Skip to content

Comments

Skydio add parachute protocol#546

Open
vivian-zhou-skydio wants to merge 2 commits intomavlink:masterfrom
Skydio:skydio-add-parachute-protocol
Open

Skydio add parachute protocol#546
vivian-zhou-skydio wants to merge 2 commits intomavlink:masterfrom
Skydio:skydio-add-parachute-protocol

Conversation

@vivian-zhou-skydio
Copy link
Contributor

@vivian-zhou-skydio vivian-zhou-skydio commented Sep 11, 2024 •
edited by hamishwillee
Loading

adding parachute protocol microservice document and test suite

vivian-zhou-skydio and others added 2 commits September 11, 2024 15:40
@vivian-zhou-skydio
add microservice document
f45a195
@amy-chen-skydio @vivian-zhou-skydio
[parachute] Parachute testsuite
4ba3baf 
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

vivian-zhou-skydio commented Sep 11, 2024

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

@amy-chen-skydio amy-chen-skydio mentioned this pull request Sep 11, 2024
Open
@hamishwillee hamishwillee self-requested a review September 11, 2024 23:56
hamishwillee
hamishwillee reviewed Sep 12, 2024
View reviewed changes
assets/services/parachute/parachute.py
@@ -0,0 +1,182 @@
import datetime
Copy link
Collaborator

@hamishwillee hamishwillee Sep 12, 2024

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 •
edited
Loading

@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.

@hamishwillee hamishwillee self-requested a review November 27, 2024 23:23
@hamishwillee
Copy link
Collaborator

hamishwillee commented Dec 11, 2024

HI @vivian-zhou-skydio

Just a gentle ping that it would be great to progress this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hamishwillee hamishwillee Awaiting requested review from hamishwillee

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vivian-zhou-skydio @hamishwillee @amy-chen-skydio