Conversation
krichardsson
approved these changes
Mar 30, 2023
Contributor
krichardsson
left a comment
There was a problem hiding this comment.
To me this is OK to merge, especially since it is a direct re-write of the C-code and you want to keep the 1 to 1 mapping.
There are room for improvements though, some general ideas would be:
- Use constants for numbers to describe what they might mean
- Use math.pi instead of 3.14
- Use math.radians() and math.degrees() when converting between radians and degrees
- Add a function for cleaning up multiranger values instead of repeating code
- A more liberal use of variables that describes the meaning of a calculation. There are some cases where calls to
logic_is_close_to(), contain a fair amount of code. Readability would improve by first assigning the calculation to a variable with a descriptive name, and secondly using the variable when callinglogic_is_close_to()
Contributor
Author
|
Thanks for the tips! I've implemented a few already but I'll add issues to both the crazyflie-firmware and crazyflie-lib-python to implement these in the future. It would be good to keep the two versions synced up. |
This was referenced Mar 31, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We have had the wall following app for a while in the crazyflie firmware. There used to be a python version made back when the SGBA work was done, but at one point all development was done using only the c version of the wall following algorithm.
I've now ported the c code back to python, so that we can have a cflib example of the wall following demo.