Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AP_IOMCU: fixed an issue with double reset of IOMCU #22899

Merged
merged 3 commits into from
Feb 13, 2023

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Feb 10, 2023

if the IOMCU resets twice in quick succession then the code that restores the safety state while flying can fail, leading to the aircraft trying to continue flying with safety on

This results from two issues:

  • a race in handling the last_safety_off variable
  • the fact that plane sets the soft_armed state based on safety state

issue reported here: https://discuss.ardupilot.org/t/plane-4-3-stable-release/91727/107

we could change the behaviour of plane so it doesn't set soft armed based on safety by removing this line of code:

https://github.com/ArduPilot/ardupilot/blob/master/ArduPlane/AP_Arming.cpp#L383

we would then need to carefully check the consequences of that for takeoff delays etc when safety is on

tested on CubeOrange quadplane and in bench testing

@WickedShell
Copy link
Contributor

we could change the behaviour of plane so it doesn't set soft armed based on safety by removing this line of code:

https://github.com/ArduPilot/ardupilot/blob/master/ArduPlane/AP_Arming.cpp#L383

I have a number of use cases that have actually strongly depended upon the current behavior of that. Everything from usage of a physical button to IOMCU on a hand launch plane allowing operators to abort, scripts and features using soft armed to control ignition lines to gas engines, and then a number of essential things like quadplanes not outputting to the motors while the IOMCU isn't outputting. Basically I'd be very concerned by a change in behavior here, and think we would need to preserve the current behavior in many of the call sites (although not necessarily all of them).

@tridge
Copy link
Contributor Author

tridge commented Feb 11, 2023

I have a number of use cases that have actually strongly depended upon the current behavior of that.

indeed, and your use cases is why I didn't make that change in this PR.
What I think we can do in a future PR is to remove the line in plane, then add a AP_Arming::armed_and_safety_off() function which we would use in the place of get_soft_armed() in most (but not all?) places in ArduPlane/
I did this PR in the way it is done to make it clear it is not changing any of that bevaviour, while fixing the bug with IOMCU reset handling

Copy link
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.

We really should come up with a better name than get_soft_armed - because, frankly, I have no idea what that state is supposed to represent at this point.

libraries/AP_IOMCU/AP_IOMCU.cpp Show resolved Hide resolved
libraries/AP_IOMCU/AP_IOMCU.cpp Show resolved Hide resolved
libraries/AP_IOMCU/AP_IOMCU.cpp Show resolved Hide resolved
if the IOMCU resets twice in quick succession then the code that
restores the safety state while flying can fail, leading to the
aircraft trying to continue flying with safety on

This results from two issues:

- a race in handling the last_safety_off variable
- the fact that plane sets the soft_armed state based on safety state
this allows testing of either watchdog or hard-fault reset
if IOMCU stops responding completely or stops giving status update
then give an internal error to help with diagnostics
@tridge tridge merged commit ab07688 into ArduPilot:master Feb 13, 2023
@rmackay9 rmackay9 added this to Pending in Copter 4.3 Feb 14, 2023
@rmackay9 rmackay9 moved this from Pending to 4.3.4-rc1 in Copter 4.3 Feb 14, 2023
@tridge tridge added this to 4.3.4 in Plane 4.3 Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Copter 4.3
4.3.4-rc1
Development

Successfully merging this pull request may close these issues.

None yet

4 participants