Add parameter for requiring Geofence for arming

[okalachev]

For autonomous systems GeoFence can do much job, even rescuing some property from damage (if system's GPS functioning properly).

I propose adding a parameter (COM_ARM_GEOFENCE), which makes GeoFence required for arming.

The options are to require GeoFence only for mission mode or for all the modes. The alternative may be requiring it for all the position assisted modes (OFFBOARD, POSCTL, TAKEOFF, ...), which may be more complicated.

Tested in SITL.

[dagar]

Overall this looks functional, although we've been slowly trying to consolidate the pre-arm logic with all of the arming requirements rolled into a single state machine helper with arming requirements bitfield.

https://github.com/PX4/Firmware/blob/7ec6f0dca6ec604e5a62707e487ad754a23ded7a/src/modules/commander/state_machine_helper.cpp#L913-L1011

The existing commander geofence logic could populate something like status_flags.condition_geofence_available, then the pre-arm logic would simply block arming or not based on COM_ARM_GEOFENCE.

[Antiheavy]

great feature! We will definitely enable it for our systems in the future.

[dagar]

Thinking about this a bit more, could we simply use the existing GF_ACTION to require a valid geofence at arming if it's set to anything other than none?
https://github.com/PX4/Firmware/compare/master...dagar:pr-geofence_check?expand=1

[okalachev]

@dagar, yes, makes sense!

[okalachev]

The only little problem it that you would need to change this parameter if you want to test your drone in STABILIZED mode, even indoor, if you don't have GeoFence.

And when you go out, it's easier to forget to change GF_ACTION back.

Maybe disable this check in "positionless" modes, like STABILIZED...

[dagar]

I think we've hit on something that may be ambiguous or open to interpretation. What's the desired geofence behavior with respect to different modes? @Antiheavy @okalachev

Manual mode being able to ignore position dependent things like a geofence makes sense to me, but potentially introduces holes in the user experience and someone will inevitably want it regardless.

[okalachev]

@dagar

There is a wish to enforce the testers and the autonomous systems' operators to not forget to set GeoFence before flight, most importantly autonomous flights in mission mode. Sometimes OFFBOARD and POSCTL modes need GeoFence as well.

But, in most cases, this requirement should not disturb to test the drone in manual modes (STABILIZED, ...), especially, when GPS is not available at all.

[okalachev]

If this setting is linked to the GF_ACTION parameter itself, then it's again easy to forget to set it after indoor testing.

[bys1123]

@okalachev I think you are right, saw people forgot to add geofence before.

[LorenzMeier]

If you violate a geofence and you enabled enforcement you should not be allowed to fly in it, no matter which mode the vehicle is in. A geofence doesn’t prevent any failures in an autonomous mode - it enforces merely location-specific compliance.

So it should indeed be based on GF_ACTION for all flight modes. We can’t justify a geofence implementation in front of a regulator that can be circumvented by something as basic as changing flight modes.

[okalachev]

@LorenzMeier, @dagar, what do you think of this:

The vehicle can be armed, when:

• the GF_ACTION is None, OR
• the geofence is set, OR
• global position is not present AND the mode does not require the position.

Also, now I tested and found an issue: the vehicle is getting armed, even when it's outside of the fence. I think, this check also should be on the pre-arm stage, and it should not get armed, when outside the fence.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Antiheavy]

Still seems like a valid topic

[stale]

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

[MikeCharikov]

Hi! Does this PR just stuck or this functionality was done in some other way?

From my point of view this issue is important from security reasons.

[stale]

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