Conversation
…prototypes, definitions, and rc_override type has been changed to allow uint8_t inputs rather than boolean.
There was a problem hiding this comment.
Pull request overview
This PR enhances the RC override functionality by replacing the simple boolean override status with a detailed bitfield-based system. The change allows the system to report not just whether RC override is active, but specifically which channels are overridden and why (stick deviation, switch activation, or offboard inactive). The implementation includes comprehensive unit tests and updates the MAVLink message structure to support the extended override information.
Key changes:
- Replaced boolean
rc_override_active()withget_rc_override()returning auint16_tbitfield - Added new
RCOverrideReasonenum with specific override flags for each channel and reason - Refactored muxing logic to use the bitfield system consistently
- Updated MAVLink
rosflight_statusmessage to useuint16_tfor rc_override field
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/command_manager_test.cpp | Added comprehensive tests for RC override bitfield functionality including default state, switch activation, and various muxing scenarios |
| src/command_manager.cpp | Refactored from boolean-based to bitfield-based override tracking with cleaner separation of concerns |
| src/mixer.cpp | Updated to use bitfield-based override checks with bitmask operations |
| include/command_manager.h | Added RCOverrideReason enum, helper constants, and updated data structures for bitfield approach |
| include/comm_link.h, src/comm_manager.cpp | Changed rc_override parameter from bool to uint16_t |
| comms/mavlink/rosflight.xml | Changed rc_override field type from uint8_t to uint16_t |
| comms/mavlink/v1.0/* | Regenerated MAVLink headers (autogenerated, primarily formatting/style changes) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@JMoore5353, The nature of the autogenerated files occurred in commit 62892a1 where the code generation instructions went from using the https://github.com/rosflight/mavlink.git repository to using https://github.com/mavlink/mavlink.git. You probably generated the mavlink code with the old repository. |
|
@avtoku That makes sense. Thanks for pointing that out. Turns out I was following the instructions in the note included in the I updated the instructions to fix some issues.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (1)
src/mixer.cpp:606
- The logic for selecting primary vs secondary mixer is inverted. When
rc_throttle_override_active == 0(meaning throttle override is NOT active), the code selects the primary mixer for Q channels. However, when override is active, it should use the secondary mixer.
The condition should be inverted:
- When
rc_throttle_override_active != 0(override IS active), use secondary mixer - When
rc_throttle_override_active == 0(override NOT active), use primary mixer
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
df8d782 to
451fd9d
Compare
This PR closes #443
@avtoku It looks like a lot of the autogenerated mavlink headers were changed when I regenerated them. I'm not sure why all these differences happened... I wondered if it was an errant git commit in the ROSflight mavlink repo, but there haven't been any changes since May, and we last regenerated the headers in June. Do you have any idea why this could be different? The CRC method has changed, as well as what seems to be a bunch of boilerplate code.
Sorry that is such an open-ended question. I generated the headers on Python 3.8, 2.7, checked out previous commits of the mavlink repo and the rosflight_firmware repo, but I'm not can't find out why the generated files are so different.
It all works (tested in sim), so there's that at least.