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

Allow more than 256 logging vars/parameters #337

Merged
merged 2 commits into from
Aug 7, 2018

Conversation

whoenig
Copy link
Contributor

@whoenig whoenig commented Jun 23, 2018

This addresses the firmware side of issue #313 by adding additional
CRTP commands (called V2) that uses 16 instead of 8bits for indexing.

This change keeps the old API the same, but caps the number of vars
to 255 to allow older clients to keep working. Eventually, the old
API should be removed.

This change reduced the possible length of variables by 1 byte,
which affected two variables whose name had to be shortened.

Tested with crazyflie_cpp commit 3eb3203. Other clients will need
similar changes to use more than the first 255 variables.

This addresses the firmware side of issue bitcraze#313 by adding additional
CRTP commands (called V2) that uses 16 instead of 8bits for indexing.

This change keeps the old API the same, but caps the number of vars
to 255 to allow older clients to keep working. Eventually, the old
API should be removed.

This change reduced the possible length of variables by 1 byte,
which affected two variables whose name had to be shortened.

Tested with crazyflie_cpp commit 3eb3203. Other clients will need
similar changes to use more than the first 255 variables.
@whoenig
Copy link
Contributor Author

whoenig commented Jun 23, 2018

This change intentionally has lots of code duplications, as I hope that we can simply remove the old API in a future change once the major clients have switched.
I did implement a "hybrid" mode in crazyflie_cpp, that first checks if V2 is available and falls back to V1 if not. This turns out to be pretty tricky, because there is no version information in the firmware right now, and it requires lots of code duplication (for C++ typesafety).

I tried to add similar support to crazyflie-lib-python, but couldn't get the dynamic check to work (there doesn't seem to be a timeout there anymore...). If you decide to simply require newer firmwares, the change will be straightforward.

This indicates that a client can use the new log messages to list up
to 16K variables
@ataffanel ataffanel merged commit eba8dd1 into bitcraze:master Aug 7, 2018
@ataffanel
Copy link
Member

@whoenig thanks a lot for the PR, this is a long needed functionality!

There actually is a protocol version in the Crazyflie, I added a commit to bump this version from 3 to 4, this will allow any client to discover that the new log commands are available.

With that it looks good to me so I am merging the PR and it closes #313.

@whoenig
Copy link
Contributor Author

whoenig commented Aug 8, 2018

Thanks @ataffanel! I added a new issue for crazyflie_cpp to use the protocol version to distinguish old/new firmwares, see whoenig/crazyflie_cpp#6.

ataffanel added a commit that referenced this pull request Sep 3, 2018
This fixes a bug where it was not possible to log a variable with ID 255
@krichardsson krichardsson modified the milestones: Next release, 2018.10 Oct 18, 2018
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