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

Skydio add parachute protocol #546

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vivian-zhou-skydio
Copy link
Contributor

@vivian-zhou-skydio vivian-zhou-skydio commented Sep 11, 2024

adding parachute protocol microservice document and test suite

The script `parachute.py` emulates a parachute module that has MAVLink v2 UDP connections.
The script `test_parachute.py` runs a standard test suite and can be used to test the emulator with its default settings.

Instructions:
1. Run simple parachute emulator `python3 parachute.py`
2. Run test `MAVLINK20=1 python3 test_parachute.py` (or `MAVLINK20=1 python3 -m unittest -v test_parachute.py` for more verbosity)

To use the test suite on other parachute modules, set the relevant env variables `MAVLINK_DIALECT`, `MAVLINK_UDPIN`, and `MAVLINK_UDPOUT`.
Example: `MAVLINK20=1 MAVLINK_DIALECT=dialect MAVLINK_UDPIN=udpin MAVLINK_UDPOUT=udpout python3 test_parachute.py`

Topic: parachute_testsuite
@vivian-zhou-skydio
Copy link
Contributor Author

Hi @hamishwillee sorry for the delay - just want to add you as a reviewer for this! :)

@@ -0,0 +1,182 @@
import datetime
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we have bit of comment on the top explaining what this is for. Can point to the protocol doc. Ditto for the following one.

@hamishwillee
Copy link
Collaborator

hamishwillee commented Sep 12, 2024

@vivian-zhou-skydio Thank you.

  1. Your branch is not allowing me to push subedits. Can you please either allow that, or fetch my branch https://github.com/mavlink/mavlink-devguide/tree/skydio-add-parachute-protocol and cherry-pick these two changes into it:

    pick 8832c63 prettier + add to sidebar
    pick d76fd51 Reduce
    

    What these do is cut back this document quite significantly. There is nothing wrong with it, except there is no point duplicating the docs in the messages - so this makes it much more high level.

  2. Further, reading this has made me question some aspects of the naming in the mavlink PR, and the particular case of the drone trigger. Sorry to kick this off again, but that's what you get when you document stuff!

  3. The first "flaw" in this is that it ignores the existing MAVLink command to enable auto triggering - https://mavlink.io/en/messages/common.html#MAV_CMD_DO_PARACHUTE

    We need to clearly outline the way they interact, and what you've done here can't break the expectations on MAV_CMD_DO_PARACHUTE implementations.

    • If this is sent to disable ATS, what should that do for your parachute? Ditto if it triggers parachute immediately, but your parachute is configured to ignore the offboard control (IMO it should obey this anyway).
    • How do we know if this API is supported on a particular system? Perhaps it just supports MAV_CMD_DO_PARACHUTE, perhaps it supports both? Do we need a protocol bit to query this?
    • Perhaps this is actually two protocols - simple and advanced. That wouldn't change any of these questions, but it might change how we present it.
    • We know that a parachute might be attached to an FC via PWM, so connection should also consider the flight stack. If we have a flight stack then it might need to emit the message instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants