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.

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

Command to allow geofence action to be set per-fence #1482

Merged
merged 4 commits into from
Apr 28, 2021

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Sep 17, 2020

MAVLink has a FENCE_ACTION that defines a number of possible actions on geofence breach. This is currently used only by ArduPilot to define the values for the system action on fence breach (it is used directly in a parameter).

This PR generalises the action, and defines a command that can be included in a geofence definition (as many times as you like) to allow the geofences in a plan to have separate actions assigned.

The message can also be used via the command protocol to set the global/system-default fence action.

@hamishwillee hamishwillee changed the title Command to allow geofence action to be set per-action Command to allow geofence action to be set per-fence Sep 17, 2020
@hamishwillee hamishwillee added the Dev Call Issues to be discussed during the Dev Call label Sep 30, 2020
@hamishwillee
Copy link
Collaborator Author

@julianoes @auturgy @DonLakeFlyer

I think this is nearly ready to go - can you give it a review?

The message sets the default system action if used in command protocol, and the action for the next fence if used in a plan. I have stated that the value in the mission plan overrides the system default even if the default action is to turn off geofences. Thoughts?

Otherwise, the FENCE_ACTION just extends the options from ArduPilot with those from PX4. A system is expected to reject a command/plan that sets a value it does not support.

@hamishwillee
Copy link
Collaborator Author

@DonLakeFlyer @julianoes Reminder - needs a sanity check.

</entry>
<entry value="2" name="FENCE_ACTION_REPORT">
<description>Report fence breach, but don't take action</description>
</entry>
<entry value="3" name="FENCE_ACTION_GUIDED_THR_PASS">
<description>Switched to guided mode to return point (fence point 0) with manual throttle control</description>
<description>Fly to MAV_CMD_NAV_FENCE_RETURN_POINT with manual throttle control in GUIDED mode.</description>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is ArduPlane keeping the concept of fence return points in the new implementation or is it switching to rally points? I'm concerned about reusing these actions for the new fence protocol and carrying over things which may not make sense any more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@DonLakeFlyer I understand that ArduPlane will not use these fence return points any more. However they will also have legacy vehicles that still support the setting.

@auturgy are you OK with us marking the cases like FENCE_ACTION_GUIDED_THR_PASS as deprecated and having adding text to description "do not use". If we can change the name to reserved as well that would be good - but I presume might break "someone".

@auturgy
Copy link
Collaborator

auturgy commented Oct 16, 2020 via email

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Oct 21, 2020

@auturgy Fair enough. Blocked/paused until ArduPilot/ardupilot#15288 merged.

FWIW commitment from ArduPilot and PX4 to implement this would be highly desirable before merging.

@DonLakeFlyer
Copy link
Contributor

FWIW commitment from ArduPilot and PX4 to implement this would be highly desirable before merging.

I would say that it should be required for at least one of them to commit. Otherwise it's just cruft in the spec.

@hamishwillee
Copy link
Collaborator Author

FWIW commitment from ArduPilot and PX4 to implement this would be highly desirable before merging.

I would say that it should be required for at least one of them to commit. Otherwise it's just cruft in the spec.

And you'd be right. This evolved from a chat in a dev call where everyone said "what a good idea" but that's not the same as saying "we'll implement it".

@joshanne
Copy link

joshanne commented Oct 22, 2020

@hamishwillee

Fair enough. Blocked/paused until ArduPilot/ardupilot#15288 merged.

RE. This point - I'm working on getting support for the new fence protocol into ArduPlane, but this is a work in progress as we encounter blocking points to be rectified. Some required changes are currently under review, so this will take some time.

@julianoes julianoes removed their request for review October 22, 2020 06:50
@stale
Copy link

stale bot commented Dec 24, 2020

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

@stale stale bot added the stale label Dec 24, 2020
@hamishwillee
Copy link
Collaborator Author

Now no longer blocked by ArduPlane, as geofence implementation went in.

@joshanne Can you confirm which fence actions from the set in this PR are/are not supported by the new implementation? (e.g. I'm guessing you might have dropped FENCE_ACTION_GUIDED_THR_PASS )
Also if there are any new ones?

Lastly, did you drop support for the MAV_CMD_NAV_FENCE_RETURN_POINT (which makes no sense now)

@hamishwillee
Copy link
Collaborator Author

We agreed in the dev call this should be "standard". I have updated to clarify that the FENCE_ACTION_GUIDED_ variants are only supported by ArduPlane. The enum remains in common.xml as it is an update. The new message command is in development.xml as per new process.

@DonLakeFlyer There is an action on @auturgy to confirm whether the new ArduPlane updates dropped support for FENCE_ACTION_GUIDED_ action variants, and for the MAV_CMD_NAV_FENCE_RETURN_POINT. The PR says that an autopilot that does not support the time of action should reject it. QGC might reasonably decide not to expose the FENCE_ACTION_GUIDED_ options directly.

@LorenzMeier @auturgy Any comments before I merge?

@hamishwillee hamishwillee merged commit 6652031 into master Apr 28, 2021
@hamishwillee hamishwillee deleted the hamishwillee-geofence_action branch April 28, 2021 00:15
@hamishwillee
Copy link
Collaborator Author

Self merged.

bkueng added a commit that referenced this pull request Apr 30, 2021
This reverts commit 6652031.

There's a problem with the C library generator:
The addition of MAV_CMD to development.xml led to the removal
of MAV_CMD in common.h. This breaks build for everyone using that enum
and including 'common.h' or 'standard.h' (and probably others).
See mavlink/c_library_v2@992871b
julianoes pushed a commit that referenced this pull request Apr 30, 2021
…#1626)

This reverts commit 6652031.

There's a problem with the C library generator:
The addition of MAV_CMD to development.xml led to the removal
of MAV_CMD in common.h. This breaks build for everyone using that enum
and including 'common.h' or 'standard.h' (and probably others).
See mavlink/c_library_v2@992871b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev Call Issues to be discussed during the Dev Call
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants