Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow individual fences to be enabled and disabled #349

Merged
merged 1 commit into from
Feb 28, 2024
Merged

Allow individual fences to be enabled and disabled #349

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion message_definitions/v1.0/common.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,23 @@
<description>Velocity limiting active to prevent breach</description>
</entry>
</enum>
<enum name="FENCE_TYPE" bitmask="true">
<entry value="0" name="FENCE_TYPE_ALL">
andyp1per marked this conversation as resolved.
Show resolved Hide resolved
<description>All fence types</description>
</entry>
<entry value="1" name="FENCE_TYPE_ALT_MAX">
<description>Maximum altitude fence</description>
</entry>
<entry value="2" name="FENCE_TYPE_CIRCLE">
<description>Circle fence</description>
</entry>
<entry value="4" name="FENCE_TYPE_POLYGON">
<description>Polygon fence</description>
</entry>
<entry value="8" name="FENCE_TYPE_ALT_MIN">
<description>Minimum altitude fence</description>
</entry>
</enum>
<!-- Camera Mount mode Enumeration -->
<enum name="MAV_MOUNT_MODE">
<description>Enumeration of possible mount operation modes</description>
Expand Down Expand Up @@ -1305,7 +1322,7 @@
<entry value="207" name="MAV_CMD_DO_FENCE_ENABLE" hasLocation="false" isDestination="false">
<description>Mission command to enable the geofence</description>
<param index="1" label="Enable" minValue="0" maxValue="2" increment="1">enable? (0=disable, 1=enable, 2=disable_floor_only)</param>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth converting this to an enum while we're here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I quite like that idea. It is more inline with how I think of bitmasks. Also it ompletely separates the old behaviour from the new behaviour so the bitmasks are only used with that new param 1.
We'd have to check no one is doing something annoying like treating any non-zero value of param 1 as enabled.

Is it worth converting this to an enum while we're here?

Probably. If we do this it makes it easier to more clearly deprecate particular options, such as param1 option "2" over time.

But let's see what @andyp1per and others think.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, but I think you need both - otherwise you need to make a call to find out what the current bitmask is.
So I think setting enable/disable with a bitmask should still be valid and then a new option which overwrites everything

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this ^^^. Why do you need to know the current bitmask? Whatever you set will become the enabled/disabled state with the new approach?

The other thing I like about using a bitmask in this way is that we could make it a proper bitmask - ie. not use 0 to mean "set all" but instead set nothing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets say I only want to change the max altitude fence. I need to know whether any of the other fences are enabled because I want to preserve their state - I have to find that out from somewhere or stash it. With the enable/disable thing I don't need to remember this because I know I am only changing one specific fence - which TBH is the whole point.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyp1per Doh - thanks.

So to be clear, the only use case you really need here is to be able to disable the altitude geofence on takeoff, enable it again when flying, then disable it for landing?

If so, why can't you:

  • Before takeoff send command with param1 = 2 - disables the floor check so you can take off
  • When flying send command with param=1 - enable all
  • Before landing send command with param1 = 2 - disables the floor check

I appreciate this new approach is more flexible but I don't see why you'd ever want to selectively turn on/off the other case.

Sorry if I'm being dumb here. We can skype if you like - hamishgw

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately I want this to be finely controlled from scripting. Individually enabling and disabling notches is a requirement here. We also have tests that test individual fences in the air - I don't want to break that behaviour.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"I want to because I have requirements and tests" doesn't sound like a use case - it is easy to create a bogus requirement and then test against it". Can you give me one real case where you're flying and you want to disable all circle fences but not all polygon fences, and visa versa? I'm asking because the change should add value.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am at a race track. I have a polygon fence around the track and a circle fence around the landing area. I want the polygon fence to always be active but I want the landing area to be deactivated on a switch when I am ready to come in to land.

Copy link

@hamishwillee hamishwillee Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's pretty inflexible. I only have to add one more fence of any kind to mean that this no longer works.

<param index="2">Empty</param>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an aside, what's your understanding the conditions under which the fences revert to their system defaults (presumably "all enabled").
For missions I think it is pretty straightforward - generally settings in missions should revert to system defaults when the mode changes out of mission mode - although this is problematic for for flight stacks that implement mission pause as a mode change, since the current setting would be lost.

I'm not sure about what happens if this is sent in a command, and the docs don't say.

  • It could be that when sent as a command this permanently changes the system default - i.e. it turns off the fences even if defined in the mission. If so, we'd need a way to know whether they are on or off.
  • It could be that when sent as a command the change is temporary for the current mode. So (thinking in PX4 as more familiar for me) you'd set takeoff mode, send command to disable the altitude fence, arm and takeoff. The fence would then turn on as you switch from takeoff to hold. Or similar.
  • Something else.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the things I am trying to fix is that mavlink currently makes permanent changes to the vehicle defaults - I want to support not doing this. My design assumption was that with this, whatever was sent from mavlink should override whatever is currently configured. We also have parameters relating to auto-enablement that are not reflected in mavlink - not permanently updating makes this a bit easier.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see benefits to either approach - setting the system defaults or a temporary change. It's not my call which is used, and I don't "care" provided the behaviour is specified, and the specification does not immediately make either ArduPilot or PX4 non-compliant (as that needs to be agreed/managed).

Assuming all are OK with this this case the description might change from:

<description>Mission command to enable the geofence</description>

to

<description>
  Enable the geofence.
  This can be used in a mission or via the command protocol.
  The effects persist while the mission is being executed, until superseded by another mission item or the mission completes.
  When used as a command (outside of mission) the value persists until superseded by another command, a mode change (which reverts system back to system defaults), or reboot.
  When used as a command while a mission is running the value is treated as though it were a mission item, and will change the current mission value.</description>

The points of interest being that:

  • mission settings persist in the mission context, even on flight stacks that pause by switching mode to "Hold": on return to the mission the mission setting would be used.
  • As a command the value only persists until mode change or reboot.
  • A command can affect a mission

All of that is arguable, but IMO should be the default thinking for commands used in both missions and command protocol.

If you think this is reasonable I'll create an upstream PR to discuss.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me

<param index="2" label="Types" enum="FENCE_TYPE">Fence types to enable or disable as a bitmask. A value of 0 indicates that all fences should be enabled or disabled. This parameter is ignored if param 1 has the value 2</param>
<param index="3">Empty</param>
Comment on lines +1325 to 1326
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyp1per For discussion, there is another way that this can be done, which might be better - two enums, one being a bitmask that says what you're enabling.

This allows you to specify any combination of flags to be enabled/disabled. It means, for example, that you don't have do know whether a particular fence type is enabled or disabled to start with, you can just use the second bitmask to specify that you're only interested in affecting the altitude floor fence (say).

We'd enable this completely separate of the original behaviour using param1=4

So something like:

Suggested change
<param index="2" label="Types" enum="FENCE_TYPE">Fence types to enable or disable as a bitmask. A value of 0 indicates that all fences should be enabled or disabled. This parameter is ignored if param 1 has the value 2</param>
<param index="3">Empty</param>
<param index="2" label="Types" enum="FENCE_TYPE">A bitmask of the fence types to enable and disable (i.e. a value of 0 indicates that all fences should disabled). Values are only changed if the corresponding type in param 3 is set. This param is ignored unless param1=4.</param>
<param index="3" label="Types bitmask" enum="FENCE_TYPE">Bitmask indicating the fence types in param2 that are affected. If 0, no fences are affected. This parameter is ignored unless param1=4.</param>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tomato, tomato - I think my way is better, but as long as I can get the functionality I am after - enabling and disabling of individual fences I don't really care. I'm not quite clear what happens to current calls with your scheme and the default values - does it really work?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should.

I'm not quite clear what happens to current calls with your scheme and the default values - does it really work?

I'm not certain what you mean, but the new stuff would all be conditional on param1 = 4. Existing systems should reject a command with this value.

<param index="4">Empty</param>
<param index="5">Empty</param>
Expand Down
Loading