Skip to content

Comments

Add support for generic payloads#3

Open
vivian-zhou-skydio wants to merge 1 commit intomasterfrom
vivian.zhou/revup/master/generic_payload
Open

Add support for generic payloads#3
vivian-zhou-skydio wants to merge 1 commit intomasterfrom
vivian.zhou/revup/master/generic_payload

Conversation

@vivian-zhou-skydio
Copy link

@vivian-zhou-skydio vivian-zhou-skydio commented Nov 16, 2024

  • Add MAV_CMD_GENERIC_PAYLOAD_FUNCTIONS_CONTROL, GENERIC_PAYLOAD_STATUS,GENERIC_PAYLOAD_FUNCTION_STATUS, and GENERIC_PAYLOAD_ERROR_FLAGS to development.xml

GENERIC_PAYLOAD_STATUS is the status for the generic payload, and GENERIC_PAYLOAD_FUNCTION_STATUS is the status for individual function(s) of the generic payload that can be controlled via MAV_CMD_GENERIC_PAYLOAD_FUNCTIONS_CONTROL.
MAV_CMD_GENERIC_PAYLOAD_FUNCTIONS_CONTROL is used to control payload functions. It can be used for simple control such as ON/OFF of a flashlight. It can also be used to control multiple functions of a component such as arm/disarm and deployment of a parachute.

Topic: generic_payload

@vivian-zhou-skydio
Copy link
Author

Reviews in this chain:
#3 Add support for generic payloads

@vivian-zhou-skydio
Copy link
Author

vivian-zhou-skydio commented Nov 16, 2024

# head base diff date summary
0 112e1ab4 01e0cc2f diff Nov 15 17:32 PM 1 file changed, 10 insertions(+)
1 91451e44 01e0cc2f diff Nov 19 13:53 PM 1 file changed, 20 insertions(+), 2 deletions(-)
2 c5f7ed27 01e0cc2f diff Dec 18 14:22 PM 1 file changed, 10 insertions(+), 9 deletions(-)
3 7b83b105 01e0cc2f diff Dec 19 14:15 PM 1 file changed, 10 insertions(+), 2 deletions(-)

@vivian-zhou-skydio vivian-zhou-skydio force-pushed the vivian.zhou/revup/master/generic_payload branch from 91451e4 to c5f7ed2 Compare December 18, 2024 22:22
- Add `MAV_CMD_GENERIC_PAYLOAD_FUNCTIONS_CONTROL`, `GENERIC_PAYLOAD_STATUS`,`GENERIC_PAYLOAD_FUNCTION_STATUS`, and  `GENERIC_PAYLOAD_ERROR_FLAGS` to development.xml

`GENERIC_PAYLOAD_STATUS` is the status for the generic payload, and `GENERIC_PAYLOAD_FUNCTION_STATUS` is the status for individual function(s) of the generic payload that can be controlled via `MAV_CMD_GENERIC_PAYLOAD_FUNCTIONS_CONTROL`.
`MAV_CMD_GENERIC_PAYLOAD_FUNCTIONS_CONTROL` is used to control payload functions. It can be used for simple control such as ON/OFF of a flashlight. It can also be used to control multiple functions of a component such as arm/disarm and deployment of a parachute.

Topic: generic_payload
@vivian-zhou-skydio vivian-zhou-skydio force-pushed the vivian.zhou/revup/master/generic_payload branch from c5f7ed2 to 7b83b10 Compare December 19, 2024 22:15
Copy link

@amy-chen-skydio amy-chen-skydio left a comment

Choose a reason for hiding this comment

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

lgtm!

<message id="398" name="GENERIC_PAYLOAD_STATUS">
<description>Generic payload status.</description>
<field type="uint32_t" name="uptime_ms" units="ms">Time since the start-up of the generic payload in ms</field>
<field type="char[32]" name="name">Generic payload name to be used in UI. This is a NULL terminated string. If it is exactly 32 characters long, add a terminating NULL. If this string is empty, a generic name is shown to the user.</field>

Choose a reason for hiding this comment

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

"If it is exactly 32 characters long, add a terminating NULL" is confusing. imo it's clearer to say something like "stores null terminated string if less than 32 characters, or 32 characters without null termination." Similarly to the language used elsewhere in this file

Choose a reason for hiding this comment

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

I think this was copied from existing example STORAGE_TYPE enum

Copy link

@thomas-watters-skydio thomas-watters-skydio Jan 24, 2025

Choose a reason for hiding this comment

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

Textual storage name to be used in UI (microSD 1, Internal Memory, etc.) This is a NULL terminated string. If it is exactly 32 characters long, add a terminating NULL. If this string is empty, the generic type is shown to the user.

from STORAGE_INFORMATION

it is a bit weirdly worded though. because it sounds like they want 33bytes for a string that is exactly 32chars long.. but that would not work in the packing. unless oddly the packing doesn't check the length but simply looks for a null character, and then verifies the length afterwards

</entry>
<entry value="404" name="MAV_CMD_GENERIC_PAYLOAD_FUNCTIONS_CONTROL" hasLocation="false" isDestination="false">
<description>Allows for enable/disable control over function(s) of a generic payload.</description>
<param index="1" label="Index" minValue="0" increment="1">Index of Function</param>

Choose a reason for hiding this comment

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

Should there also be a maxValue of 65535? function_count is a uint16_t

Choose a reason for hiding this comment

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

Good point! Parameters are sent as float and could have a value bigger than 65535

<field type="char[32]" name="name">Generic payload function name to be used in UI. This is a NULL terminated string. If it is exactly 32 characters long, add a terminating NULL. If this string is empty, a generic name is shown to the user.</field>
<field type="uint16_t" name="index">Index of this function on the generic payload.</field>
<field type="uint8_t" name="enable">0: Disable, 1: Enable</field>
<field type="uint16_t" name="function_count">Total number of functions on the generic payload that can be controlled.</field>

Choose a reason for hiding this comment

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

Do we need this in both messages?

Choose a reason for hiding this comment

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

@vivian-zhou-skydio what was the reasoning here, i feel like I had convinced myself it was useful

<field type="uint32_t" name="error_flags" enum="GENERIC_PAYLOAD_ERROR_FLAGS" display="bitmask">Errors</field>
<field type="uint32_t" name="custom_error_flags" display="bitmask">Bitmap used to show custom error flags.</field>
<field type="float" name="power_draw" units="W" invalid="NaN">The power draw of the generic payload. NaN: field not provided</field>
<field type="uint16_t" name="weight" units="g" invalid="0">Generic payload weight. 0: field not provided.</field>

Choose a reason for hiding this comment

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

Is this expected to change dynamically for things like droppers? If not, should this then reflect the max weight of the payload?

<field type="float" name="temp_c" invalid="NaN">Temperature in Celsius. NaN: field not provided.</field>
</message>
<message id="399" name="GENERIC_PAYLOAD_FUNCTION_STATUS">
<description>Generic payload function status.</description>

Choose a reason for hiding this comment

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

Suggested change
<description>Generic payload function status.</description>
<description>Generic payload function status. Use [MAV_CMD_REQUEST_MESSAGE](https://mavlink.io/en/messages/common.html#MAV_CMD_REQUEST_MESSAGE).param2 to indicate the index/id of requested function: 0 for all, 1 for first, 2 for second, etc.</description>

<entry value="404" name="MAV_CMD_GENERIC_PAYLOAD_FUNCTIONS_CONTROL" hasLocation="false" isDestination="false">
<description>Allows for enable/disable control over function(s) of a generic payload.</description>
<param index="1" label="Index" minValue="0" increment="1">Index of Function</param>
<param index="2" label="Enable" minValue="0" maxValue="1" increment="1">0: Disable, 1: Enable</param>

Choose a reason for hiding this comment

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

Suggested change
<param index="2" label="Enable" minValue="0" maxValue="1" increment="1">0: Disable, 1: Enable</param>
<param index="2" label="Activate" minValue="0" maxValue="1" increment="1">0: Disable, 1: Activate</param>

<field type="uint16_t" name="weight" units="g" invalid="0">Generic payload weight. 0: field not provided.</field>
<field type="float" name="temp_c" invalid="NaN">Temperature in Celsius. NaN: field not provided.</field>
</message>
<message id="399" name="GENERIC_PAYLOAD_FUNCTION_STATUS">

Choose a reason for hiding this comment

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

I wonder if there's reason to expand this to include something that specifies the control scheme ie :

enum ControlSchemeFlags {
  LATCHING = 1,
  NON_LATCHING = 2,
  BINARY = 4, 
  ANALOG = 8, 
  PWM = 16,
};

The non-latching idea was motivated by the dropper needing to "reset" after actuation. However in this case the current UI only sends on USER_ACTION to deploy, not reset. (Could be updated to send RESET maybe).

But non-latching would still be interpreted by attachment, ie it could latch for a time period, then reset on its own (in the case of the dropper), or it fires and resets immediately (one shot gpio0 on/off)

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.

4 participants