Commander datalinks lost failsafe handing #11550

cmic0 · 2019-02-27T10:10:21Z

This PR takes over the original one #11454.
In order to test the original PR some fixes have been added here.

Tests

data link loss & regain (GCS HEARTBEAT)
onboard computer link loss & regain (HEARTBEAT)
iridium loss & regain -> PENDING

The above points have been tested in SITL.
The fixes performed in this PR prevent mavlink messages flooding when you lose ONBOARD Controller:

After the fix:

NEXT STEPS

Unify datalink/OA status flags in vehicle_status_flags msg, e.g:
- condition_data_link_valid
- condition_avoidance_system_valid
- condition_manual_control_valid
Adapt [WIP] Added obstacle avoidance healthiness topic in vehicle status msg. #11456 and avoidance failsafe in commander #11457 to the new messages.

REMARKS
At the moment NAV_DLL_ACT and NAV_RCL_ACT are brought into commander_params.c but still has the old naming convention.

@dagar @MaEtUgR @mrivi FYI

MaEtUgR · 2019-02-27T13:48:46Z

I now reviewed #11454, honstely it's a bit overwhelming how many changes are proposed at the same time. Part of it is probably duplicate because it's just what the other pr contains. Changing the base branch of the pr would probably help a lot.

mrivi

I would propose to try to bring in this PR as it is now and handle #11456 in a separate PR.
As for #11457, it requires more discussion on the overall architecture so I would leave it for another PR.

mrivi · 2019-02-27T13:55:40Z

src/modules/commander/Commander.cpp

-			} else {
-				mavlink_log_critical(&mavlink_log_pub, "ACTIVATING AVAILABLE HIGH LATENCY LINK");
+			if (_datalink_last_status_avoidance_system == telemetry_status_s::MAV_STATE_CRITICAL) {
+				mavlink_log_info(&mavlink_log_pub, "AVOIDANCE SYSTEM TIMEOUT");


I would prefer to keep this mavlink_log_critical

I have had an offline talk with @baumanta about avoidance timeouts..
We came to the conclusion that a time out is less important than an avoidance loss, thus I opted for removing the warning (which I find annoying in QGC in terms of UX), and replaced with an info.

the vehicle starts loitering when there is a timeout https://github.com/PX4/avoidance/blob/master/local_planner/src/nodes/local_planner_node_main.cpp#L75-L76

The pilot should be informed on why that's happening clearly. Since critical warning are also audible I would prefer to have it critical.

I think it is ok it leave it as an info. The timeout state is something very temporary and in most cases only perists over a fraction of a second. The point in time when a timeout is triggered is also somewhat random, as it can be tuned through a parameter in the avoidance repo. And if the sensor data is not recovered in a few seconds the planner will send abort anyway and you will see that as a critical message.

cmic0 · 2019-02-27T14:12:45Z

I would propose to try to bring in this PR as it is now and handle #11456 in a separate PR.
As for #11457, it requires more discussion on the overall architecture so I would leave it for another PR.

Totally agree, I think that this should be the starting point as @baumanta commits are required to tests onboard controller lost/avoidance lost.

The vehicle_status_flag message modification can be handled in next PRs and then #11456 can be adapted on top of that.

cmic0 · 2019-02-27T14:36:47Z

@MaEtUgR Major changes in this PR are the same as #11454 performed by @dagar.
Since to continue testing #11454 we needed @baumanta commits and other minor fixes we thought it could make sense to bring all those small fixes together and create new PR.

cmic0 · 2019-02-27T15:26:49Z

Rebased on latest upstream.

MaEtUgR · 2019-02-28T14:47:35Z

make sense to bring all those small fixes together and create new PR

ok, just as a general hint it's easier to have iterations when making big changes. #11454 is already quite big to review and it's clear that you need your adjustments for testing but try to avoid loading more and more changes in the pr you like to get merged since it will take longer to be reviewed, tested, rebased and it all gets slower in the end.

thomasgubler · 2019-03-05T05:54:39Z

@MaEtUgR we want to bring this in now. Do you have objections on the changes that claudio added on top of #11454

@cmic0 I see some open review comments from @MaEtUgR in #11454 ? Did you adress @MaEtUgR s reciew comments that he made in #11454 here? Please point that out to him by commenting to his comments with e.g. "fixed in [commit link]"

cmic0 · 2019-03-05T08:16:20Z

@thomasgubler not yet, some of them are related on the general behavior (and not strictly related to the datalink failsafe handling) thus I preferred not to take actions.
I will start bringing in the quickest ones and discuss the others.

bkueng

Reviewed, and remarked what I found.

boards/stm/32f4discovery/default.cmake

msg/telemetry_status.msg

src/modules/mavlink/mavlink_receiver.cpp

src/modules/commander/state_machine_helper.cpp

src/modules/commander/commander_params.c

src/modules/commander/Commander.cpp

src/modules/commander/Commander.hpp

src/modules/mavlink/mavlink_receiver.cpp

…on to commander.

Major changes: - Prevent "HIGH LATENCY DATA LINK LOST" message to appear if iridium telemetry is not used. - Prevent "DATA LINK LOSS" mavlink messages flooding when QGC is open and then closed. - Changed "DATA LINK REGAINED" condition (use _datalink_last_heartbeat_gcs insthead of _datalink_lost) Signed-off-by: Claudio Micheli <claudio@auterion.com>