ardupilot icon indicating copy to clipboard operation
ardupilot copied to clipboard

Plane: NAV_ALTITUDE_WAIT apply wiggle to all control surfaces

Open magicrub opened this issue 1 year ago • 6 comments

This is two commits. First is an NFC refactor to bring NAV_ALTITUDE_WAIT mission and logic code paths to line up with NAV_DELAY. It is now owned by ModeAuto instead of global access via plane.auto_state. Second is to wiggle all control surfaces instead of just the 3 main ones. I wrote this in the Holybro booth at AUVSI Exponential 2024 /w inspiration from @ohitstarik to improve his balloon drop servo wiggles.

This code hasn't been changed in many years so I thought it deserved a refactor before anyone added new functionality to it.

magicrub avatar Apr 29 '24 02:04 magicrub

I've compiled and bench tested to verify the above changes as well:

https://www.youtube.com/watch?v=uB6nyUTwIfc

ohitstarik avatar Apr 30 '24 19:04 ohitstarik

We do have a open issue in this area: https://github.com/ArduPilot/ardupilot/issues/25851

I do like the refactor. I don't understand why the fix works. Your outputting to all control surfaces, but the existing output mixers are still running, so all of those surfaces other than aileron, elevator, rudder and flap will just be overwritten with values derived from those four when the output mixers run later in the loop.

@ohitstarik I would have expected the existing wiggle function to work for the right elevon but not left, this is not show in your testing?

IamPete1 avatar May 01 '24 17:05 IamPete1

This is blocked by PR https://github.com/ArduPilot/ardupilot/pull/26946

magicrub avatar May 01 '24 19:05 magicrub

@IamPete1 I only tested with the Left Elevon, so yes theres a real possibility that the right elevon wouldve worked fine.

I noted that the control surfaces had less throws while wiggling marked as an elevon. They had noticeably more throw while wiggling marked as Ailerons. This might have something to do with the old output mixers still running.

ohitstarik avatar May 01 '24 22:05 ohitstarik

PR https://github.com/ArduPilot/ardupilot/pull/26946 has merged so this PR is just the control surface change

magicrub avatar May 02 '24 17:05 magicrub

@IamPete1 I see what you mean about the mixer handling many of the flight surfaces, and the mixer also cancelling some of the movements in your Issue https://github.com/ArduPilot/ardupilot/issues/25851. I'm open to suggestions on how to do this, lets discuss in the next devcall

magicrub avatar May 02 '24 17:05 magicrub

Possible fix concept https://github.com/ArduPilot/ardupilot/compare/master...ohitstarik:ardupilot:wiggle_staging_fix

magicrub avatar May 06 '24 23:05 magicrub

result from devcal:

  • The change from @ohitstarik looks OK but it's extending the existing messy code so a better way-forward is to do it right with the modern functions we have today. The existing code is 8 years old... many helpers have been added to the Servo library since. A better solution is a time-based thing that is not using the magic 4500 type numbers
  • This is generally better handled in scripting so I'll add a Vehicle::is_wiggling_servo() flag that has LUA hooks.
  • Nice-to-have: add support for MAV_CMD_NAV_ALTITUDE_WAIT to all vehicles

magicrub avatar May 07 '24 00:05 magicrub

I missed the original dev call, apologies. I've created a separate PR for this fix: #27275

I agree that this could be fixed better but the situation is:

1.) something doesnt work as intended in the code 2.) we have a fix for it

i propose that we merge this as an interim fix until someone picks up the more intensive fix in the future. Otherwise, we'll knowingly be leaving the feature broken until whenever that fix happens. happy to talk about this on the next dev call.

ohitstarik avatar Jun 08 '24 16:06 ohitstarik

replaced by https://github.com/ArduPilot/ardupilot/pull/27275

IamPete1 avatar Jun 10 '24 18:06 IamPete1