adding feature to prevent arming before onboard logging is ready

[catch-twenty-two]

Describe problem solved by this pull request
The goal of this pr is to keep the vehicle grounded while a heartbeat from an onboard logger has not been recieved, or has not reported as ready.

See here for the mavlink router changes as an example to enable this feature:

mavlink-router/mavlink-router#171

Describe your solution
This dis-allows arming without an on board logger being enabled and in a ready state the default setting allows the vehicle to arm without the on board logger reporting as ready via it's heartbeat

Set COM_ARM_WO_OBLOG to 1 to enable this feature.

Test data / coverage
Here is fmu output under normal operation (notice the transition of 155 an on board logger from status 3 to 4):

Got onboard heartbeat 155, 3
Got onboard heartbeat 155, 3
Got onboard heartbeat 155, 3
INFO [logger] Start file log (type: full)
INFO [logger] Opened full log file: /fs/microsd/log/2019-05-15/23_08_17.ulg
Got onboard heartbeat 155, 4
INFO [logger] Start mavlink log
Got onboard heartbeat 155, 4
Got onboard heartbeat 155, 4
Got onboard heartbeat 155, 4
Got onboard heartbeat 155, 4
Got onboard heartbeat 155, 4
Got onboard heartbeat 155, 4
INFO [logger] Stop mavlink log
Got onboard heartbeat 155, 3
INFO [logger] closed logfile, bytes written: 239363
Got onboard heartbeat 155, 3
Got onboard heartbeat 155, 3

Here is what happens when the logger fails to open a new log file (no transition):

Got onboard heartbeat 155, 3
Got onboard heartbeat 155, 3
INFO [logger] Start file log (type: full)
INFO [logger] Opened full log file: /fs/microsd/log/2019-05-15/23_05_37.ulg
Got onboard heartbeat 155, 3
Got onboard heartbeat 155, 3
Got onboard heartbeat 155, 3

Tested on a pocket beagle and laptop with a Pixhawk Cube running px4.

[dagar]

Can we drop the WO naming scheme before it goes any further?

[catch-twenty-two]

Done

[dagar]

The overall diff is a bit hard to follow, but so far this looks like an okay solution.

Bigger picture I'm questioning if we have the interactions at the right level. Commander can require receiving a HEARTBEAT from a logging component (if the single telemetry_status isn't overwhelmed), but that doesn't tell us that logging is actually going to work.

• commander arming require logging
• logger configured for mavlink, etc, logger provides status/health message for commander
• mavlink_ulog tracks remote logger health via HEARTBEAT
• logger and mavlink_ulog are in sync via ulog_stream and acknowledgements, the status of the remote logger could be passed back

Then we'd have a solution that could actually ensure logging is fully functional before arming. We could also catch problems at other levels.

[catch-twenty-two]

The overall diff is a bit hard to follow, but so far this looks like an okay solution.

Bigger picture I'm questioning if we have the interactions at the right level. Commander can require receiving a HEARTBEAT from a logging component (if the single telemetry_status isn't overwhelmed), but that doesn't tell us that logging is actually going to work.

• commander arming require logging
• logger configured for mavlink, etc, logger provides status/health message for commander
• mavlink_ulog tracks remote logger health via HEARTBEAT
• logger and mavlink_ulog are in sync via ulog_stream and acknowledgements, the status of the remote logger could be passed back

Then we'd have a solution that could actually ensure logging is fully functional before arming. We could also catch problems at other levels.

Hi @dagar Yes, agreed. I looked into adding this after remote logging is activated but it would be difficult since there isn't a great way to wait for the initial exchange for the ulog headers and other info to take place then allow arming. @bkueng any opinions about this?

[dagar]

Hi @dagar Yes, agreed. I looked into adding this after remote logging is activated but it would be difficult since there isn't a great way to wait for the initial exchange for the ulog headers and other info to take place then allow arming. @bkueng any opinions about this?

Yes, there are a number of little pieces missing. I understand the need for this as a short term bandaid that will prevent the blatant errors. As long as we're okay with the possibility of ripping it out someday with a heavy refactor. I really don't like this loose monitoring of something as a proxy for what we really want to ensure. Commander really shouldn't be micromanaging like this.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

[LorenzMeier]

Thanks for originally proposing this! I'm closing this PR as stale - if you still would like to add this, a new, rebased PR would be most welcome! Our apologies for not being more on top of it earlier. Closing stale PRs helps us to make sure that overall we can drive PRs to completion.