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

development.xml: RADIO_RC_CHANNELS message #343

Merged
merged 10 commits into from
Feb 27, 2024
Merged

development.xml: RADIO_RC_CHANNELS message #343

Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8865e68
development.xml: RADIO_RC_CHANNELS message
jlpoltrack Dec 6, 2023
ad21308
add target fields
jlpoltrack Dec 28, 2023
5a59ee3
address feedback
jlpoltrack Jan 26, 2024
4cb1e2b
further updates
jlpoltrack Feb 14, 2024
5d5cea1
further updates 2
jlpoltrack Feb 14, 2024
7d96b50
further updates 3
jlpoltrack Feb 14, 2024
88cdc26
further updates 4
jlpoltrack Feb 15, 2024
9e0907d
Update message_definitions/v1.0/development.xml
jlpoltrack Feb 21, 2024
af01ec5
Update message_definitions/v1.0/development.xml
jlpoltrack Feb 21, 2024
0dc3b40
Update message_definitions/v1.0/development.xml
jlpoltrack Feb 22, 2024
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
33 changes: 33 additions & 0 deletions message_definitions/v1.0/development.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@
<description>True if the data from this sensor is being actively used by the flight controller for guidance, navigation or control.</description>
</entry>
</enum>
<enum name="RADIO_RC_CHANNELS_FLAGS" bitmask="true">
<description>RADIO_RC_CHANNELS flags (bitmask).</description>
<entry value="1" name="RADIO_RC_CHANNELS_FLAGS_FAILSAFE">
<description>Failsafe is active. The content of the RC channels data in the RADIO_RC_CHANNELS message is implementation dependent.</description>
</entry>
<entry value="2" name="RADIO_RC_CHANNELS_FLAGS_OUTDATED">
<description>Channel data may be out of date. This is set when the receiver is unable to parse incoming data from the transmitter and has therefore resent the last data it was able to process.</description>
jlpoltrack marked this conversation as resolved.
Show resolved Hide resolved
</entry>
</enum>
</enums>
<messages>
<message id="53" name="MISSION_CHECKSUM">
Expand All @@ -38,5 +47,29 @@
<field type="float" name="raw_press" units="hPa">Raw differential pressure. NaN for value unknown/not supplied.</field>
<field type="uint8_t" name="flags" enum="AIRSPEED_SENSOR_FLAGS">Airspeed sensor flags.</field>
</message>
<message id="420" name="RADIO_RC_CHANNELS">
<description>RC channel outputs from a MAVLink RC receiver for input to a flight controller or other components (allows an RC receiver to connect via MAVLink instead of some other protocol such as PPM-Sum or S.BUS).
Note that this is not intended to be an over-the-air format, and does not replace RC_CHANNELS and similar messages reported by the flight controller.
The target_system field should normally be set to the system id of the system to control, typically the flight controller.
The target_component field can normally be set to 0, so that all components of the system can receive the message.
Copy link

@hamishwillee hamishwillee Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Worth saying perhaps that this might allow targeting of what is effectively a second RC controller to command a a specific mavlink component - i.e. allow having separate RC for gimbal and autopilot flight control?

What if we want to target a vehicle attached gimbal using a separate RC?

Copy link

Choose a reason for hiding this comment

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

IMHO that's what targets are generally for in a MAVLink system

Copy link

Choose a reason for hiding this comment

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

Right, but since you've explicitly said it can normally go everywhere, my thought was that it makes sense to say what case would make you not send it everywhere. Had you said nothing I would probably have left it. I do like spelling things out.

The channels array field can publish up to 32 channels; the number of channel items used in the array is specified in the count field.
The time_last_update_ms field contains the timestamp of the last received valid channels data in the receiver's time domain.
The count field indicates the first index of the channel array that holds invalid data.
jlpoltrack marked this conversation as resolved.
Show resolved Hide resolved
The RADIO_RC_CHANNELS_FLAGS_OUTDATED flag is set by the receiver if the channels data is not up-to-date (for example, if new data from the transmitter could not be decoded so the previous data is resent).
jlpoltrack marked this conversation as resolved.
Show resolved Hide resolved
The RADIO_RC_CHANNELS_FLAGS_FAILSAFE failsafe flag is set by the receiver if the receiver's failsafe condition is met (implementation dependent, e.g., connection to the RC radio is lost).
In this case time_last_update_ms still contains the timestamp of the last valid channels data, but the content of the channels data is not defined by the protocol (it is up to the implementation of the receiver).
For instance, the channels data could contain failsafe values configured in the receiver; the default is to carry the last valid data.
Note: The RC channels fields are extensions to ensure that they are located at the end of the serialized payload and subject to MAVLink's trailing-zero trimming.
</description>
<field type="uint8_t" name="target_system">System ID (ID of target system, normally flight controller).</field>
<field type="uint8_t" name="target_component">Component ID (normally 0 for broadcast).</field>
<field type="uint32_t" name="time_last_update_ms" units="ms">Time when the data in the channels field were last updated (time since boot in the receiver's time domain).</field>
<field type="uint16_t" name="flags" enum="RADIO_RC_CHANNELS_FLAGS" display="bitmask">Radio RC channels status flags.</field>
<field type="uint8_t" name="count">Total number of RC channels being received. This can be larger than 32, indicating that more channels are available but not given in this message.</field>
Copy link

@hamishwillee hamishwillee Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we can conceivably want more than 32 channels, would we be better to use an index and a count for this message? And in that case you could have a smaller array if you wanted.

Or if we're agreeing that truncation is actually important enough here to force reordering and never extending, why don't we make this a full length array (i.e use up the rest of the packet).

Copy link

Choose a reason for hiding this comment

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

I thought about this. We could add a field with the first channel in the message, which would allow sending several. Also, sure one could make it as large as possible (and I increased already from 24 to 32).
But: I really think we should not overdo it ...

My first impulse: REALLY? I would argue if really more than 32 RC channles are needed really something is wrong with the setup and what this person wants to do. There should then be a more appropriate "microservoce". I.e., the only reason I can see one may want more than 32 channels is to missuse it for something which is not existing. Just put 32 switches on your radio ... I won't believe you that you will know them all just 2 weeks later.

This message can be emitted by the receiver at relatively high rate, let's say 100 Hz (depending on the receiver)! Again, I doubt one ever will need that many rc channels at high rate, and all it tells is that oen wants to do something this message isn't for.

I VERY STRONGLY advice to not make this a message which is supposed to do everything and cure all issues in this world.

Copy link

Choose a reason for hiding this comment

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

All very logical, but we've said "that'll be all we need" so many times I don't believe it any more. We recently had someone come up with a joystick with more than 32 widgets on it, and history tells me that this will grow.

But anyhow, the main reason I want this it to use up all the remaining bits so that no one can extend it - that's for the same reason you have - to stop it being reused and extended at will.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think more than 32 R/C channels is useful

<extensions/>
Copy link

Choose a reason for hiding this comment

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

Suggested change
<extensions/>

No reason to make these extensions AFAICS.

trailing-zero-in-payload-trimming happens regardless of extension/not extension.

Copy link

Choose a reason for hiding this comment

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

IMHO incorrect. The sequence of fields is reordered, such that the larger fields come first. So the unit8 fields would be at the back, and zero trim would not work for the channel fields

Copy link

Choose a reason for hiding this comment

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

Good call!

Could we get a comment instead, please?

Copy link

Choose a reason for hiding this comment

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

done

Copy link

Choose a reason for hiding this comment

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

If we do this we're saying we will never extend this message. Is that a shared understanding?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes

<field type="int16_t[32]" name="channels" minValue="-4096" maxValue="4096">RC channels.
Channel values are in centered 13 bit format. Range is -4096 to 4096, center is 0. Conversion to PWM is x * 5/32 + 1500.
Channels with indexes equal or above count should be set to 0, to benefit from MAVLink's trailing-zero trimming.</field>
</message>
</messages>
</mavlink>