Sub: When disarmed and in manual mode, set throttle out to neutral value

[amarburg]

Fixes #32056

We are working to confirm the issue and this change with our other BlueROVs.

Testing on a BlueROV2 Heavy running BlueOS-1.4.2 with Ardupilot 4.5.7 (also tested with 4.5.5).

Under a very specific set of circumstances:

1. Vehicle in the water, armed, running in alt hold
2. Disarm vehicle
3. Set to manual mode
4. Arm vehicle (now in manual mode)
5. Set to alt hold mode

The vehicle will descend suddenly by ~1m, then stabilize at a depth slightly below its original depth. The second and third steps are interchangeable (manual then disarm or disarm then manual).

This patch sets the vertical throttle to 0.5 (neutral, no vertical thrust) when the vehicle is disabled in any of these modes:

• Acro
• Auto
• Circle
• Guided
• Manual
• PosHold
• Stabilize
• Surface

Expected behavior

On all transitions into alt hold mode, the vehicle should hold the current vehicle depth.

[ES-Alexander]

Hi @amarburg, thanks for submitting this fix (and raising the underlying issue).

Given your PR is to the master branch, have you confirmed the functionality occurs in a recent Dev release of the firmware? The current ArduSub stable (4.5) is quite far behind master (4.7), so it would provide some extra peace of mind to know the issue is in fact still present. That said, from the code you're changing I suspect the issue does indeed exist, and would likely be fixed by the suggestion of properly factoring in the bidirectional throttle support in ArduSub.

I'd personally prefer if the throttle functions were refactored to use 0 as the neutral point rather than using 0.5 as a hardcoded value in multiple places without explanation, but I believe the neutral point is determined by motor library code that also affects other firmware, so that may not be straightforward to implement and confirm (and/or may involve some hassle with the required broader conversations). Perhaps a reasonable compromise would be to define a NEUTRAL_THROTTLE constant (e.g. in Sub.h), and use that instead of hardcoded values? That at least explains what the intent of the value is, and avoids reader confusion when wondering why the code looks to potentially be setting the throttle to 50% with no explanation.

It would likely also be helpful if you can squash your two commits together, and (for future reference) update the PR description to reflect that it's not just manual mode being modified, and change the first two words to "Fixes", to make use of GitHub's keywords to auto-close the Issue when the PR gets merged.

I have requested a review from the primary ArduSub maintainer, who will hopefully be able to properly test the changes, and merge the code when relevant :-)

[amarburg]

@ES-Alexander my testing thus far has been with my rebase of these changes onto Sub-4.5, so I'll need to do a separate test with master / 4.7 to confirm the behavior and the fix.

I'll squash / clean the MR when I have a moment. I agree, the 0.5-means-no-vertical-thrust convention is very confusing, but it's pervasive across Ardusub and, I think, would require either an unsatisfying piecemeal change or an intensive refactoring. I can put together a proposal for a NEUTRAL_THROTTLE constant, tho.

[amarburg]

Per @ES-Alexander , I've added a constant NEUTRAL_THROTTLE in config.h. Caught a few other cases where throttle needs to be set in other modes.

I don't have a positioning system, so my ability to test the more complex modes (guided, auto, circle) is limited.

And squashed.

[ES-Alexander]

I've added a constant NEUTRAL_THROTTLE in config.h. Caught a few other cases where throttle needs to be set in other modes.

Nice!!

I don't have a positioning system, so my ability to test the more complex modes (guided, auto, circle) is limited.

You could likely test in SITL, with an autotest (or several)? Positioning is enabled in SITL simulations by default.

If you make a test that seems useful for preventing similar issues in future, you could contribute it as part of the PR so it runs when others try to contribute code.

[ES-Alexander]

This makes sense to me, and the implementation cleans up a bunch of hardcoded values (always a plus).

@Williangalvani would be great if you could test+review this when you can make some time for it :-)

[Williangalvani]

That is a nice cleanup + bugfix. Thanks!