search

ArduPilot / mavlink

MAVLink micro air vehicle marshalling / communication library
http://qgroundcontrol.org/mavlink/start
Other
63 stars 287 forks source link

Allow individual fences to be enabled and disabled #349

Closed andyp1per closed 9 months ago

andyp1per commented 10 months ago

Allows a bitmask to be passed to the fence enable function to specify individual fences to enable or disable

This has been done so that by not providing the second arg you get the current behaviour which is to enable and disable all fences

auturgy commented 10 months ago

@hamishwillee can you have a quick look - I think this is good to go, if you agree I'll pull it across to mavlink/mavlink

hamishwillee commented 10 months ago
  1. So just to clear, the intent is that if this appears in a mission, all subsequent geofences of the specified type(s) will be enabled/disabled until the next occurrence of the mission item, or the mission ends?
  2. What's the use case you're trying to solve?
  3. It is a pity we can't get rid of that param1 value of 2 and have a more consistent command.
  4. Not for this PR but we should probably change the description to remove the explicit mention of "mission command". YOu could reasonably use this in a command to enable or disable the default system geofence too.

@auturgy Looks good to me. Once this is in mavlink/mavlink there might be differing opinions.

andyp1per commented 10 months ago

@hamishwillee

So just to clear, the intent is that if this appears in a mission, all subsequent geofences of the specified type(s) will be enabled/disabled until the next occurrence of the mission item, or the mission ends?

Yes, although that's not actually the problem I am trying to solve

What's the use case you're trying to solve?

The main issue is that ALT_MIN fences cannot be simply enabled and disabled at the same time as all the others. If you are on the ground you only want to enable the fences that make sense (for instance). In the air you might want something different. A single big fat enable flag makes all of this logic extremely convoluted - you have to have all kinds of exceptions to things. This allows the mavlink commands to actually DWIS as well as DWIM

It is a pity we can't get rid of that param1 value of 2 and have a more consistent command.

Yes I agree, but history

Not for this PR but we should probably change the description to remove the explicit mention of "mission command". YOu could reasonably use this in a command to enable or disable the default system geofence too.

hamishwillee commented 10 months ago

Ah, so geofences are all default on. Your first mission item might be to disable the minimum alt fence. Then once flying enable all, and disable just the minimum fence again to land?

For takeoff I guess you might have some rule that says minimum fences don't apply until you have first exceeded them, but you'd still need some way to turn them off again in order to land. So this makes sense.

@auturgy I'm good with this.

nickexton commented 10 months ago

So if we want to change the fence type (e.g. turn off CIRCLE and turn on POLYGON), we need to send two messages?

Should we also add a new value to p1 called e.g. SET_FROM_BITMASK such that we could combine the above into:

hamishwillee commented 9 months ago

@auturgy @andyp1per I don't believe this particular implementation to address the stated use case is necessary.

  1. The specific example Andy needs can be addressed is to turn off altitude fences on takeoff/landing, and that can be addressed using the existing API as in https://github.com/ArduPilot/mavlink/pull/349#discussion_r1472046521
  2. It is possible to construct use cases where it is useful to also turn off a circular or polygon fence too .This feels very contrived, but it is also inflexible - you can turn off the one fence type but if you'll jump around hoops if you have multiple areas.

Something more useful would be an approach for enabling/disabling an arbitrary set of inclusion and/or exclusion fences. We might do this using an approach similar to inclusion groups. Essentially you could give any type of fence you want to enable/disable a group number like MAV_CMD_NAV_FENCE_POLYGON_VERTEX_INCLUSION and then send a command to enable/disable that particular group.

nexton-winjeel commented 9 months ago

I don't believe this particular implementation to address the stated use case is necessary.

Disagree. We would use something similar to https://github.com/ArduPilot/mavlink/pull/349#discussion_r1489982474 during flight testing is this was available:

Something more useful would be an approach for enabling/disabling an arbitrary set of inclusion and/or exclusion fences.

Yes, that would be more useful, but that capability doesn't exist now. I don't think you should reject a solution that works because there might be a better solution in the future.

auturgy commented 9 months ago

Thanks. I'll make the upstream PR unless someone else beats me to it.

andyp1per commented 9 months ago

Thanks all!