Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure we can access the hash of the ledringtimingsmem #1303

Closed
wants to merge 1 commit into from

Conversation

dolfje
Copy link
Contributor

@dolfje dolfje commented Jul 18, 2023

It was always the intention that you can check if crazyflie still has the latest ledring timings. That is a hash that you can upload and check afterwards. But current code only allows updating the timings itself.

It was always the intention that you can check if crazyflie still has the latest ledring timings. That is a hash that you can upload and check afterwards. But current code only allows updating the timings itself.
@dolfje dolfje changed the title Make sure we can access the has of the ledringtimingsmem Make sure we can access the hash of the ledringtimingsmem Jul 18, 2023
@ataffanel
Copy link
Member

Thanks for the PR! I agree on principle: the hash was made to be read to verify if the led timing memory needs to be re-uploaded.

Though, while changing the read part is no problem, it is not implemented in the lib, the write part would be a breaking change and is a little bit more problematic. Mostly that the spirit was to get the hash calculated by the Crazyflie and not by the client writing the data.

I have a couple of alternate solution that would work:

  1. Make reading and writing asymetrical: we write timings and read hash-then-timing. This does not feel right though since it will not emulate a memory anymore
  2. Make the hash accessible on read at address 0xFFFC. This would work fine for me.
  3. Make the hash accessible as a log variable instead. A bit odd but it would work.

In all those 3 cases, the hash is calculated in the handleWrite function.

What do you think?

@knmcguire
Copy link
Member

knmcguire commented Sep 27, 2023

@dolfje any comments on this? We don't like to keep pull request hanging so if you are not able to address these we will close the pull request if that is okay? Then it might be better to start up a issue and discuss the implementation further.

@knmcguire
Copy link
Member

closing due to inactivity. Please reopen if you'd like to implement this.

@knmcguire knmcguire closed this Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants