AR_Motors: added MOT_REV_DELAY

[tridge]

adds an optional delay when reversing motors for ESCs that need it

this does not yet try to stop integrator windup while reversing. I suspect that is not needed for many ESCs as the required delay is quite small (eg. the ESCs I have need a 0.3s delay)
Also note that all ESCs actually do have a delay on reversal, so we already have the integrator issue, it is just that this makes the delay inherent in the signal sent to the ESC, rather than only being in the internals of the ESC

[Copilot]

Pull Request Overview

This PR adds an optional delay for motor reversal in vehicles using reversible ESCs. The key changes include:

• Adding a new parameter (_reverse_delay) to configure the motor reversal delay.
• Introducing a ReverseThrottle struct to handle delayed outputs for throttle reversal.
• Updating the output_throttle function to integrate the reversal delay logic.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
libraries/AR_Motors/AP_MotorsUGV.h	Added the _reverse_delay parameter and the ReverseThrottle structure for reversal logic.
libraries/AR_Motors/AP_MotorsUGV.cpp	Updated parameter grouping documentation and revised output_throttle to incorporate reversal delay handling.

[rmackay9]

Thanks for this.

The only functional issue I see is that the limits are not being applied. I think you'll see the effect of this when setting the REV_DELAY to a really large number (like 10 seconds). After the 10 seconds has elapsed the throttle will suddenly jump to a really high value because of I-term buildup

Stylistically I guess I would have preferred if, instead of creating a new output, the throttle was overriden just below these lines:

    // apply rate control
    throttle = get_rate_controlled_throttle(function, throttle, dt);

Still, you've been kind enough to add this important feature so I won't look a gift horse in the mouth too much :-)

[IamPete1]

This also breaks slew limiting, the slew limit will not be aware of the zero hold, so when the hold is ended throttle may exceed the set slew limit.

[tridge]

@IamPete1 we actually already have the jump and integrator build up in existing rovers and users don't seem to find it a problem. The slew limit is on the mixer input throttle, not the output throttle and there is no limit on steering rate of change, so we already jump when you steer.
Also, all ESCs have a delay when you change direction. It is an inherent part of how an ESC works. So the integrator build-up and other related issues already happens. If you set a really high MOT_REV_DELAY then it would be more noticible, but the actual values needed for ESCs is quite low. On the ESCs I did this for it is 0.3s. I suspect even if you could configure the ESC to not need the zero throttle to change then the ESC would still stop for around 0.3s. A graph of RPM vs throttle will not be a straight line through zero on any ESC.
So I'd vote for the lazy solution and see how well this works. If it turns out to be a real issue we can do something fancier

[tridge]

I would have preferred if, instead of creating a new output, the throttle was overridden just below these lines:

I think to do it properly we'd move the slew handling to use SRV_Channels::set_slew_rate()

[IamPete1]

So the integrator build-up and other related issues already happens. If you set a really high MOT_REV_DELAY then it would be more noticible, but the actual values needed for ESCs is quite low. On the ESCs I did this for it is 0.3s.

I think its fine to apply the lazy solution if you also constrain the timeout to a small number. 2 seconds maybe?

[tridge]

I think its fine to apply the lazy solution if you also constrain the timeout to a small number. 2 seconds maybe?

the documented range is 0.1 to 1.0 seconds. If the user goes outside that range then the GCS should warn. Putting an absolute restriction is I think unnecessary - I could imagine huge rovers (eg. something the size of a tank or barge) that needs a long time to reverse an engine, but where the time constant for control is so long that 2s is not an issue