-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Mellinger: support to change gains in bindings #1180
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually never noticed the CRAZYFLIE_FW define before. Is this to separate functionality for the python bindings build and the firmware build? Will there be an build error if you don't use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this logic from collision_avoidance.c - might have been introduced there by @jpreiss. We want it here to avoid that these functions end up in the bindings, since they are "internal". A cleaner way could be to have separate files, but that makes code navigation more annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered both choices. IIRC, I didn't have particularly strong motivation for choosing one file with
#ifdef
over two files. I probably just did it to reduce the number of files. I don't see any discussion in #567 either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also the option to use the
UNIT_TEST_MODE
define that is used in unit tests to show that some part of the code is added/removed to make the code testable. I just added it to the python tests in PR #1183 that I'm working on. Maybe it should be renamed though?I prefer a solution where there are as few
#if
s and buts as possible in the code, especially modifications related to test. I vote for separating the generic functionality and the global state into two files, even though it is a bit trickier to navigate.As an extra input to this I also would like to add that we have (sort of) wanted to separate code that depends on FreeRTOS and the platform in
src/modules
, while generic code that could be used on other platforms goes intosrc/utils
(I can see that this is not completely true at the moment...). I'm not sure it has been communicated outside the office but has been part of internal discussions. For the mellinger controller this would translate to maybe moving the generic file toutils
while the file with the global state stays inmodules
. I think this is outside the scope of this PR but I just wanted to put it out there. To me this is related to cleaning up dependencies and make it easier to re-use/test code.