Added function to query SYS_STATUS present/enabled/health flags

[dakejahl]

Added function to query SYS_STATUS present/enabled/health. This allows applications to look at specific sensor statuses, such as logloader which looks at the MAV_SYS_STATUS_LOGGING flag

[dakejahl]

Not sure what the check for diff check is doing or why it's failing

[JonasVautherin]

Thanks!

I guess the question is whether it should expose an uint32_t or some list of booleans for all (or a subset) of the sensors in MAV_SYS_STATUS_SENSOR.

Does it always make sense to expose all those values, or are some values fundamentally "internal"? I could imagine for instance that if SENSOR_BATTERY=false means "there is no battery", maybe it should go together with the battery update (e.g. an is_present field in https://github.com/mavlink/MAVSDK-Proto/blob/main/protos/telemetry/telemetry.proto#L566-L573?). Same for the GPS, maybe if a GPS sensor is not "present", we should have a FixType saying "NO_GPS_SENSOR" (if there is a difference between "no GPS connected" and "no GPS present").

What do you think?

Not sure what the check for diff check is doing or why it's failing

I think it would be the style check. Try running ./tools/fix_style.sh 👍

[dakejahl]

I did run fix_style.sh, it's only failing on 24.04 🤔

Does it always make sense to expose all those values, or are some values fundamentally "internal"?

I thought about this but decided to expose them all since PX4 currently doesn't implement all of them but could at any point in the future. It would just be a pain to have to updated mavsdk again if for example I decided to add the flags for a VIO system, or optical flow, or precision landing. So I figured just expose the raw 3 flags and we get everything in one go.

[julianoes]

I usually don't want to expose bitflags like this but make the API clearer. I know that's a pain but otherwise we might as well expose all over MAVLink directly and we're back to square one.

So generally the idea is to have boolean flags that actually mean something instead and can be queried.

For logloader specifically, could you use passthrough instead?

[julianoes]

I did run fix_style.sh, it's only failing on 24.04 🤔

You need to fix the style on 24.04 or use the docker helper.

[dakejahl]

I could use passthrough but then my code becomes disorderly. Okay if I changed it to a boolean flag for each of those mentioned sensors, what pattern should I follow?

This isn't doing anything more

tools/run-docker-clang-format.sh tools/fix_style.sh .

[dakejahl]

if there is a difference between "no GPS connected" and "no GPS present"

also part of why I was avoiding the specific flags.. PX4 makes it hard to use the sys_status_sensor flags directly since they're coupled to the HealthAndArmingChecks Report. I can't remember exact details, but when I implemented the loggerCheck I wasn't able to set the present/enabled flags directly, and as a result they toggle true/false together. I think the expectation would be that present means the vehicle has logging functionality (sd card) and enabled would mean it's actively logging.

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[JonasVautherin]

Okay if I changed it to a boolean flag for each of those mentioned sensors, what pattern should I follow?

I would think something like the winch status flags? https://github.com/mavlink/MAVSDK-Proto/blob/517fef5a3a673e9b50f65a56dff350c6af515564/protos/winch/winch.proto#L77-L101:

/*
 * Winch Status Flags.
 *
 * The status flags are defined in mavlink
 * https://mavlink.io/en/messages/common.html#MAV_WINCH_STATUS_FLAG.
 *
 * Multiple status fields can be set simultaneously. Mavlink does
 * not specify which states are mutually exclusive.
 */
message StatusFlags {
    bool healthy = 1;           // Winch is healthy
    bool fully_retracted = 2;   // Winch line is fully retracted
    bool moving = 3;            // Winch motor is moving
    bool clutch_engaged = 4;    // Winch clutch is engaged allowing motor to move freely
    bool locked = 5;            // Winch is locked by locking mechanism
    bool dropping = 6;          // Winch is gravity dropping payload
    bool arresting = 7;         // Winch is arresting payload descent
    bool ground_sense = 8;      // Winch is using torque measurements to sense the ground
    bool retracting = 9;        // Winch is returning to the fully retracted position
    bool redeliver = 10;        // Winch is redelivering the payload. This is a failover state if the line tension goes above a threshold during RETRACTING.
    bool abandon_line = 11;     // Winch is abandoning the line and possibly payload. Winch unspools the entire calculated line length. This is a failover state from REDELIVER if the number of attempts exceeds a threshold.
    bool locking = 12;          // Winch is engaging the locking mechanism
    bool load_line = 13;        // Winch is spooling on line
    bool load_payload = 14;     // Winch is loading a payload
}

[julianoes]

I think it will either be a Status struct per component:

message StatusFlags {
   bool present = 1;
   bool enabled = 2;
   bool healthy = 3;
}

Or a flat enum:

enum Status {
    STATUS_NA = 0; // Not present, not enabled, not healthy
    STATUS_PRESENT = 1; // Not enabled, not healthy
    STATUS_ENABLED = 2; // Not present, not healthy
    STATUS_PRESENT_ENABLED = 3; // Not healthy
    // STATUS_HEALTHY = 4; // -> invalid
    STATUS_PRESENT_HEALTHY = 5; // Not enabled 
    // STATUS_ENABLED_HEALTHY = 6; -> invalid
    STATUS_PRESENT_ENABLED_HEALTHY = 7; -> All ok!
}

[JonasVautherin]

I think it will either be a Status struct per component:

The StatusFlags struct with 3 booleans is a lot more intuitive to me, I would say 🙈