-
Notifications
You must be signed in to change notification settings - Fork 744
mavproxy_param: allow setting multiple bitmask bits simultaneously #1549
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
Conversation
joshanne
left a comment
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.
First, I love seeing that people are using this feature already!
I would like to test this and see how it behaves with a vehicle.
MAVProxy/modules/mavproxy_param.py
Outdated
|
|
||
| if bit_index is None: | ||
| print(f"Invalid bit index: {arg_bit_index}") | ||
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.
prune the whitespace.
joshanne
left a comment
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.
@peterbarker only comment I have is above regarding informing of errors.
The functionality appears to work as expected, and doesn't break any workflows with the single bit index.
I feel like there could be a more intuitive way to do the same thing - When i do this, I do each bit at a time, ensuring I don't make a mistake at each step. Not saying it's the perfect way, though.
|
I've added the suggested changes, I can squash the commits if you approve |
|
@be-ska looks like it's all working fine, i'd be happy if you pruned the whitespace and squash the commits. Really only needs to be the one commit. This gets my thumbs up. @peterbarker leaving you for final approval, but all seems fine to me. Curious if you've made use of the bitmask manipulation yourself? |
da2fd8e to
d836aa4
Compare
Yep, I'll merge this once CI passes. And yes, I do use the new command a bit - particularly when combined with the detailed-which-bits-are-set parameter view it's pretty damned handy! |
|
Merged, thanks! |
Sometimes it's very handy to set multiple bits with just one command, ie: setting ARMING_CHECK or LOG_BITMASK.
With this very simple PR user can provide more bit arguments, so
change ARMING_CHECK from ALL (=1) to ALL but CAMERA (=982526)