navigator: fix edge case with valid idle setpoint #14134
Conversation
|
Where are you seeing 1.5 seconds spent in navigator? RTL safe points or geofence? In vehicle_status we carry the timestamp of last nav_state change. What about using this downstream in flight tasks to prevent processing of a stale position setpoint triplet? |
There was a problem hiding this comment.
I did some SITL testing on this and didn't see anything break but it's really hard to verify since I can't find any way to guarantee what's inside the triplet also to see if there are other such cases.
Note that the IDLE setpoint type is presumably deprecated since there's takeoff logic in the position controller and when the drone is armed and landed it stays idle until the flight task is requesting/allowing a takeoff. In mission the condition is that the output of the jerk optimized trajectory generator has a velocity setpoint that tries to go up with more than 0.3m/s: https://github.com/PX4/Firmware/blob/0d36e5094b4655c960846d481f62361d40cf1d9f/src/lib/flight_tasks/tasks/AutoLineSmoothVel/FlightTaskAutoLineSmoothVel.cpp#L315 This should work fine according to my understanding but we need proper outdoor testing before merging.
There was a proposition for that workaround before but IMHO it overcomplicates things even more. Also it doesn't solve the real problem we're experiencing in this case where Naviator is publishing valid triplet setpoints while there's no navigator mode running. I would like to make sure Navigator publishes what it wants to do when a navigator mode is running and leave the triplet invalid whenever it's not considered valid anymore which also was the point of the original fix #13534. |
It would be in addition to that, and as far as I know it's the only thing that actually enables us to begin addressing the race condition between navigator and flight_tasks. There are other ways to do this, but they'd be quite a lot more rework. For example splitting out flight_tasks from mc_pos_control and running both flight_tasks and navigator in the same work queue with a simple priority mechanism so that navigator runs first on vehicle_status change. |
|
Tested on pixhawk4 v5 f-450 Procedure Notes Log |
That's just the worst case numbers I remember from doing dm_read on cheap SD cards. |
Tested on PixRacer V4Indoor Flight Test Procedure Notes Log https://review.px4.io/plot_app?log=cd74017f-76b0-4bd9-a628-120f0b3f0563 Tested on CUAV nano V5Indoor Flight Test Procedure Notes Log https://review.px4.io/plot_app?log=f6955de2-c20e-4aa5-a8b9-dfc2c7f73166 |
|
@PX4/testflights is there a chance that you could also test this with fixedwing? We would especially be interested around the behaviour right after arming. Best would be a takeoff in manual mode but also an auto-takeoff. |
|
Tested on Fixed Wing Pixhawk 1 V2 Procedure Notes Log: https://review.px4.io/plot_app?log=bf04da6f-e692-4d86-b0ac-c60ec159823d @julianoes we don't know if the crash has any relation to the PR. We will replace the frame, let us know what you see. Thanks. |
|
@Tony3dr sorry about the crash! Have you been flying this airframe regularly with master? From the log I can see that there was a software hardfault. I will investigate it. Edit: It also looks like it was correctly following the setpoints and a turn was commanded by RC. |
|
@dagar this looks like the likely cause: And here is some symbols that were in the hardfault log: |
@julianoes, yes we fly the vehicle on and off and no issues in the past. We are replacing the frame and should have the vehicle ready in the next few days. |
@Tony3dr ok that's good. The issue came because "sideslip fusion failure" was triggered somehow, and that lead to the stack overflow, so it really was a special case which accidentally happened in this flight. The failure did not happen because of this PR here but it would still be good to test this one again. |
This is an attempt to fix an edge case in the triplet publication which can lead to crashes on autopilots with slow SD cards. The sequence of events before this patch is: 1. Switch to POSCTL when disarmed. At this point current valid with setpoint idle is published. 2. Arm, takeoff, and fly using joystick/RC. 3. Switch to RTL (or trigger RTL using RC loss). At this point the setpoint is valid but still idle and the motors will shut off. 4. Once navigator has published the new setpoint (which can take up to 1.5 seconds on slow SD cards) we will hopefully recover. With this patch we omit this edge case, so we never publish this idle setpoint when landed. The assumption is that this idle setpoint is no longer required with the current flight task code, however, that needs to be further verified.
julianoes
force-pushed
the
pr-fix-idle-sp
branch
from
6d17302 to
c8f81e6
Compare
|
@Tony3dr this is rebased on master now and can be tested right after you have successfully flown master. |
|
@PX4/testflights please test this one with fixedwing in stabilized and in a mission, thanks. Make sure first without propeller if auto-takeoff and things like that work as expected. |
|
@Tony3dr do you think you'll be able to test it this week? |
@julianoes, unfortunately, we have rain all week. I'm guessing Monday we can get that tested. |
|
@Tony3dr how's the weather? :) |
The weather continues to limit us from Test Flying. We are monitoring closely and hopefully, we get to squeeze this PR and others we have pending. |
|
@Tony3dr this is now merged but it would still be good to do the test with master, thx! |
|
Tested on Pixhawk 1 v3 fixed-wing Flight modes: Master daily Notes: |
|
I see the only possible failure cases without an idle setpoint to be arming in Hold mode on the ground or automatic takeoff and landing but I have honestly no idea how that works with fixed-wing. I'm always hand-launching my plane in stabilized mode to avoid this uncertainty. |
This is an attempt to fix an edge case in the triplet publication which can lead to crashes on autopilots with slow SD cards.
The sequence of events before this patch is:
With this patch we omit this edge case, so we never publish this idle setpoint when landed. The assumption is that this idle setpoint is no longer required with the current flight task code, however, that needs to be further verified.