Skip to content

Sub: transition to manual_override, deprecate roll-pitch toggle#32639

Open
ES-Alexander wants to merge 5 commits intoArduPilot:masterfrom
ES-Alexander:sub-manual-control-improvements
Open

Sub: transition to manual_override, deprecate roll-pitch toggle#32639
ES-Alexander wants to merge 5 commits intoArduPilot:masterfrom
ES-Alexander:sub-manual-control-improvements

Conversation

@ES-Alexander
Copy link
Copy Markdown
Contributor

@ES-Alexander ES-Alexander commented Mar 31, 2026

Summary

Refactors ArduSub MAVLink MANUAL_CONTROL handling to allow ignoring axes and support channel ranges, and deprecates the roll-pitch toggle joystick button function to aid in doing so.

Testing (more checks increases chance of being merged)

  • Checked by a human programmer
  • Tested in SITL
    • Builds, but not yet tested
  • Tested on hardware
  • Logs attached
  • Logs available on request
  • Autotest included

Description

  1. Refactors MAVLink MANUAL_CONTROL handling to (generally) ignore axes when specified as INT16_MAX, per the spec
    • Switches from direct channel overrides to using the GCS_MAVLINK::manual_override method, per @peterbarker's suggestion
      • To make this work I had to add value constraining to the manual_override implementation, and friend the Sub class
        • Not sure whether either of those changes will be considered problematic 🤷‍♂️
  2. To simplify 1., deprecates the joystick button functionality that toggles between forward+vertical <> roll+pitch axis control
    • Philosophically, joystick axis input configuration is better handled in the joystick hardware and/or in the GCS, which can then send consistent and reasonable values through MAVLink to the autopilot
    • I believe this feature is minimally used - Sub users are more likely to use the roll/pitch trim joystick button functions, and/or the s and t axes with a 6-axis joystick
    • Accidentally switching into this mode has been a source of confusion for various users over the years
    • Maintaining this code is rather unpleasant, and makes other functionality hard to implement
    • Preferably not merged until joystick: migrate MANUAL_CONTROL axes to data lake bluerobotics/cockpit#2476 (so there's some kind of viable GCS alternative for users without extra axes or dynamic axis remapping functionality in their controller)
    • Ideally not merged until joystick: make axis selection/remapping dynamic bluerobotics/cockpit#2524 is resolved, so there's at least one nice reference implementation of a GCS alternative, but not critical

If merged, closes #32348, though I'm currently unsure which is the lesser evil...

Copy link
Copy Markdown
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method is now mis-named :-)

Good improvement.

Obviously some behavioural changes - did you want to add warnings if people try things that aren't supported now?

Comment thread libraries/GCS_MAVLink/GCS.h Outdated
MAV_RESULT set_message_interval(uint32_t msg_id, int32_t interval_us);

// Helper function to override the input to a RC channel
void manual_override(class RC_Channel *c, int16_t value_in, uint16_t offset, float scaler, const uint32_t tnow, bool reversed = false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be tempted to friend that joystick class instead, but it's not a big thing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now friended Sub in GCS_MAVLINK instead, which I believe is at least marginally nicer than making a function public just for Sub usage.

I wanted to do GCS_MAVLINK_Sub instead, or GCS_Sub, but the joystick.cpp file uses so many top level Sub features that it became impractical to write sub. everywhere.

I also wanted to guard the friending with #if APM_BUILD_TYPE(APM_BUILD_ArduSub), but that was apparently an invalid usage of APM_BUILD_TYPE, and I couldn't find a reasonable alternative guard flag, so I had to just include it in all the firmware 🤷‍♂️

Comment thread libraries/GCS_MAVLink/GCS_Common.cpp Outdated
@ES-Alexander ES-Alexander force-pushed the sub-manual-control-improvements branch from 45eb0c5 to 03ba7f5 Compare April 5, 2026 20:06
@ES-Alexander
Copy link
Copy Markdown
Contributor Author

The method is now mis-named :-)

@peterbarker not sure what you mean by this. As far as I can tell the manual_override function still converts its input into an RC input channel override, so transform_manual_control_to_rc_override still seems correct? Perhaps you're referring to a different method...

Good improvement.

Thanks - it wasn't very fun to implement 😅
I did at least learn a fair amount, so I'll take the win for what it's worth :-)

Did you want to add warnings if people try things that aren't supported now?

Added a warning about the removed roll-pitch toggle behaviour, so it's at least not a silent failure. Was tricky to get something meaningful in 50 characters, but hopefully it's still clear enough to Sub users 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants