DroneCAN (UAVCAN) SafetyState does not communicate ARM status

mattofak commented 2 years ago

Describe the bug

I have an ESC that I'm making speak DroneCAN aka UAVCAN, and I was attempting to use the ardupilot.indication.SafetyState to determine if PX4 was armed and ready for ESCs to spin.

Unfortunately, in armed and prearmed state, PX4 claims that the safety is off:

From safety_state.cpp:

if (actuator_armed.armed || actuator_armed.prearmed) {
            cmd.status = cmd.STATUS_SAFETY_OFF;

This change was seemingly introduced in https://github.com/PX4/PX4-Autopilot/pull/16185 where someone wanted their servo to move in the pre-arm state.

I believe this change was erroneous as there is no other message that the PX4 currently emits that can indicate the arming state.

To Reproduce

I have a CubePilot, so roughly

Attach battery and RC recevier to cube pilot
Configure PX4 UAVCAN_MODE to (3) Servo and ESC
See that when QGroundControl is in the Ready to Fly state, that the ardupilot.indication.SafetyState message is reporting 255: Safety Off.

Expected behavior

The Safety Off indication should only be sent when PX4 is armed; or that there is some other indication from PX4 of the arming state.

mattofak commented 2 years ago

Assuming there's no method to configure PX4 to emit a different message, or behave differently, that I haven't found: I had a couple of ideas on how to correct the behavior so that ESCs are safe when PX4 is not armed. (Without doing some gymnastics on the ESC side looking for 0 in the raw command or something which otherwise could be confused with a valid command while in the ARM state.)

Introduce a new state into ardupilot.indication.SafetyState between STATUS_SAFETY_ON and STATUS_SAFETY_OFF -- something like STATUS_SAFETY_SURFACES_ONLY. But this might confuse servos which are not looking to see that the safety state != STATUS_SAFETY_ON
Keep the current behavior the same, but also emit the uavcan.equipment.safety.ArmingStatus message and only set STATUS_FULLY_ARMED when fully armed. But, then we have two sources of easily confused data and we still don't really capture the tristate ARM intent.
Change ardupilot.indication.SafetyState to know about the UAVCAN_MODE and when the mode is 3, only emit STATUS_SAFETY_OFF when fully armed.

mattofak commented 2 years ago

One more idea:

When not armed, the uavcan.equipment.esc.RawCommand could send a message with length 0; and then the ESC could at least know that there are no commands being sent. But again, this only implicitly communicates the Arm/Prearm/Safe states.

PetervdPerk-NXP commented 2 years ago

DroneCAN message standard is quite messy, wouldn't it be better to expose all the actuator_armed statuses using a new combined DroneCAN ArmingStatus msg based on the msgs below? https://github.com/dronecan/DSDL/blob/master/uavcan/equipment/safety/1100.ArmingStatus.uavcan https://github.com/dronecan/DSDL/blob/master/ardupilot/indication/20007.NotifyState.uavcan https://github.com/dronecan/DSDL/blob/master/ardupilot/indication/20000.SafetyState.uavcan

AlexKlimaj commented 2 years ago

See my pr with safety status. https://github.com/PX4/PX4-Autopilot/pull/19748

mattofak commented 2 years ago

Personally I'm not opposed to a new combined message that has more states than just safe and armed; but I don't know how much appetite there is for a new message vs extending an existing message vs some other approach. For me, the nice thing about extending an existing message is that it might be backwards compatible, unless the message signature changes because of the introduction of new enum values...

With respect to your PR, again I'm not opposed to having two messages sent with different interpretations of armed, but somehow the documentation has to be written clearly and in multiple places which message you should be listening to for what application (e.g. ardupilot.inidication.SafetyState for surfaces, and uavcan.equipment.safety.ArmingStatus for motors or other hazardous equipment.)

PX4 / PX4-Autopilot