-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
common: extend MANUAL_CONTROL with auxiliary continuous inputs #2031
common: extend MANUAL_CONTROL with auxiliary continuous inputs #2031
Conversation
@auturgy Is this the straw that breaks the camel's back with respect to MANUAL_CONTROL? i.e. every time we extend it we say "this is now too big", but hey, let's live with it. FYI @MaEtUgR we've talked about a new message that doesn't have the buttons. Instead whatever is connected to the joystick (i.e. QGC) provides MAVLink commands to perform button mappings. I think it was Martina suggested another component information based API for controlling mappings in a "mavlink enabled" joystick. I'll look at this next week. Based on history this will probably be fine. |
@hamishwillee Thanks for the background. I expected there were already related discussions. Doesn't the extension paragraph basically say: "prefer MAVLink commands to trigger autopilot functionality rather than streaming button positions and map them autopilot side"? PX4 currently only supports this workflow for joysticks. The For continuous inputs MAVLink commands are unsuited since they usually don't trigger discrete events but should be streamed. So the options are:
I'm convinced we need separate messages for common use cases like gimbal control but would like to enable the generic inputs for the long tail of features and cases where the same knob does different things on a specific vehicle depending on the mode. |
I See the actual Manual_Control very limited and lets say, ridiculous. MAVLink can cook coffe and made toasts, but can manually only control 4 axis. As @MaEtUgR evidenced, there are use cases that needs more axis. Same is for us, we develop a MAVLink native IP based rugged remote controller with up to 8 axis and 64 buttons. Ok, 64 buttons are for sure too much, but 8 axis is the least Manual_Control should support. Can leave with 16 buttons. Internally in the remote controller we work with our own C2 standard, this could be a good starting point to copy. Our intention is to substitute the classic hobby grade ICs with a IP based, such as HereLink. The controller using Manual_Control should be seen in the Vehicle Setup under "Radio" as a standard RC input, nothing else. |
I will think on this tomorrow. FWIW in passing, @MaEtUgR the "generic inputs" point is compelling. |
@relaxibus Thanks for your comments. I completely agree with you that the MANUAL_CONTROL has shortcomings. We should be able to support more axis, and different types of buttons and switches.
The question is whether we try to address that using modifications of I don't know how much experience you have with MAVLink, so please excuse me if the following is already known/obvious to you:
I'm not sure of the right way to do this long term. You sound like you might be a good person to act as a stakeholder in the discussion as we move forward. |
@MaEtUgR IMO the concept of having a way to map an arbitrary variable controller seems a good thing. Should we add more than one? I've added this to next dev call and will see if I can get comment from @auturgy before then. FWIW
|
There is a spectrum between:
I'm convinced there will always be people who want both or a combination and I'd myself at least enable a compromise where the interface clearly says which stick is in what (calibrated) position but not what it does in which mode so similar to the What I do not see is that the ground station based on the mode maps the stick to a certain setpoint and sends that. Also, I'd avoid forcing all inputs to plain old RC_CHANNELS and not allowing the ground station to define or specify anything. This pull request is part of allowing a compromise. If nothing comes in between I'll join the MAVLink call. |
@hamishwillee thanks for your explanations. I‘m not a MAVLink expert, but quite fit. Regarding the bandwidth, the uplink, usually doesn‘t need as much bandwidth as the downlink. This why I developed the SERaero uplink protocoll to minimize bandwidth usage. The Downlink is the same as CAN Aerospace, just modified for serial use. |
I'll raise in the call this evening (in 3.5 hours). @MaEtUgR Thanks for that perspective. There is a lot to be said for a MAVLink joystick to be plug'n'play for at least some core features (I say "mavlink joystick" because that's the role QGC is performing for you). The problem is that everyone thinks that support for their own thing is worth paying the extra bandwidth. So I think am personally happy with this suggestion, but I know this same conversation will come up again, and probably sooner rather than later. Anyhow, let's see in the meeting. |
message_definitions/v1.0/common.xml
Outdated
<field type="uint8_t" name="enabled_extensions">Set bits to 1 to indicate which of the following extension fields contain valid data: bit 0: pitch, bit 1: roll, bit 2: aux</field> | ||
<field type="int16_t" name="s">Pitch-only-axis, normalized to the range [-1000,1000]. Generally corresponds to pitch on vehicles with additional degrees of freedom. Valid if bit 0 of enabled_extensions field is set. Set to 0 if invalid.</field> | ||
<field type="int16_t" name="t">Roll-only-axis, normalized to the range [-1000,1000]. Generally corresponds to roll on vehicles with additional degrees of freedom. Valid if bit 1 of enabled_extensions field is set. Set to 0 if invalid.</field> | ||
<field type="int16_t[6]" name="aux">Auxiliary continuous input fields normalized in the range [-1000,1000] which are mapped to use case specific effects. Considered if bit 2 of enabled_extensions field is set. A value of INT16_MAX indicates that a particular input is invalid.</field> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On further thought, the trade off of 6 bits for potentially 5 * 2 wasted bytes on every message, whether used or not, seems unreasonable. If we have to extend this and we pay an extra byte for more extension fields we're still way ahead "for most users".
I've also split this into individual items rather than an array on the assumption that it is easier for the recipient to read/map them like this, because the pattern for extending is obvious. If that is untrue they might be combined into an array again.
Thoughts? Am I missing something about why the other way makes more sense?
<field type="uint8_t" name="enabled_extensions">Set bits to 1 to indicate which of the following extension fields contain valid data: bit 0: pitch, bit 1: roll, bit 2: aux</field> | |
<field type="int16_t" name="s">Pitch-only-axis, normalized to the range [-1000,1000]. Generally corresponds to pitch on vehicles with additional degrees of freedom. Valid if bit 0 of enabled_extensions field is set. Set to 0 if invalid.</field> | |
<field type="int16_t" name="t">Roll-only-axis, normalized to the range [-1000,1000]. Generally corresponds to roll on vehicles with additional degrees of freedom. Valid if bit 1 of enabled_extensions field is set. Set to 0 if invalid.</field> | |
<field type="int16_t[6]" name="aux">Auxiliary continuous input fields normalized in the range [-1000,1000] which are mapped to use case specific effects. Considered if bit 2 of enabled_extensions field is set. A value of INT16_MAX indicates that a particular input is invalid.</field> | |
<field type="uint8_t" name="enabled_extensions">Set bits to 1 to indicate which of the following extension fields contain valid data: bit 0: pitch, bit 1: roll, bit 2: aux</field> | |
<field type="int16_t" name="s">Pitch-only-axis, normalized to the range [-1000,1000]. Generally corresponds to pitch on vehicles with additional degrees of freedom. Valid if bit 0 of enabled_extensions field is set. Set to 0 if invalid.</field> | |
<field type="int16_t" name="t">Roll-only-axis, normalized to the range [-1000,1000]. Generally corresponds to roll on vehicles with additional degrees of freedom. Valid if bit 1 of enabled_extensions field is set. Set to 0 if invalid.</field> | |
<field type="int16_t" name="aux1">Aux continuous input field 1. Normalized in the range [-1000,1000]. Purpose defined by recipient. Valid data if bit 2 of enabled_extensions field is set. 0 if bit 2 is unset.</field> | |
<field type="int16_t" name="aux2">Aux continuous input field 2. Normalized in the range [-1000,1000]. Purpose defined by recipient. Valid data if bit 3 of enabled_extensions field is set. 0 if bit 3 is unset.</field> | |
<field type="int16_t" name="aux3">Aux continuous input field 3. Normalized in the range [-1000,1000]. Purpose defined by recipient. Valid data if bit 4 of enabled_extensions field is set. 0 if bit 4 is unset.</field> | |
<field type="int16_t" name="aux4">Aux continuous input field 4. Normalized in the range [-1000,1000]. Purpose defined by recipient. Valid data if bit 5 of enabled_extensions field is set. 0 if bit 5 is unset.</field> | |
<field type="int16_t" name="aux5">Aux continuous input field 5. Normalized in the range [-1000,1000]. Purpose defined by recipient. Valid data if bit 6 of enabled_extensions field is set. 0 if bit 6 is unset.</field> | |
<field type="int16_t" name="aux6">Aux continuous input field 6. Normalized in the range [-1000,1000]. Purpose defined by recipient. Valid data if bit 7 of enabled_extensions field is set. 0 if bit 7 is unset.</field> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with the "enable extensions" you "activate" the extra "aux" fields? So now we have up to 8 analog channels?
How many buttons? 16?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with the "enable extensions" you "activate" the extra "aux" fields?
In MAVLink there is a feature called zero-byte truncation which means that any zero bytes at the end of the packet in send order can be truncated and get reconstructed at the other end. So what that means is that we want to send zero for any fields that are not going to be used, so they are truncated and the message is more efficient. The enable_extensions bit tells us whether the other fields are zero because the value is zero (of the input) or because they are not being used.
Upshot, if you want to use a particular axis you set the bit associated with it. Note that this is a suggestion - what it may replace is a single bit that enables all 6 axis - this is wasteful of bandwidth.
So now we have up to 8 analog channels?
6 fields are defined as aux that can be mapped to any purpose - ie. they are like RC channels in that you have to map them to something in the flight stack/recipient. I wouldn't quite call them analog. They are a 16 bit int that you must set to a value between -1000 and 1000. But no less analog than the RC_OVERRIDES :-)
Hope that helps.
How many buttons?
We haven't added explicitly added any additional buttons. However if someone in a flight stack wants to use one of the aux as a 16 button mapping they could. I think that would be a good idea but would require discussion/flight stack implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many buttons?
Just to clarify:
The buttons
field offers 16 bits so 16 buttons that have an on/off state.
The buttons2
extension field offers and additional 16 bits so you can cover a total of 32 buttons with the extension. This was already the case and does not change with this pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the trade off of 6 bits for potentially 5 * 2 wasted bytes on every message, whether used or not, seems unreasonable
You are right. I can change it to use the bitfield up for what it was intended for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also split this into individual items rather than an array on the assumption that it is easier for the recipient
I don't know. I thought the array is easier to fill/read but I'm also fine with separate fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. I thought the array is easier to fill/read but I'm also fine with separate fields.
Missed this sorry. I never had to read from these so that was a question. But I kind of like having them individual because if we extend even more, the pattern for using the field in code stays the same - i.e. not mixing accessing values in arrays vs values.
W.r.t. buttons it is true that nothing has changed. FYI only, note however that button implementations are (or at least historically have been) a bit inconsistent. The definition basically says that these buttons are hard coded buttons 1 - 32 on the controller - so if you plug in a joystick and it has button 10 (say) you'd expect button 10 to be set in the appropriate bit. However
- on some flight stacks the positions are mapped to functions - so bit 10 in MANUAL_CONTROL button array might always be kill switch and if a kill switch is assigned to button 3 then QGC does the mapping to actually set bit 10 (I "think" this is how PX4 used to do it).
- Others don't set the bits at all, but directly map the function of the set button in QGC to a mavlink command - so if button 3 is mapped to kills switch the button field is not set in MANUAL_CONTROL, but instead a kill command is sent (I think this is what PX4 does now.
- On ArduPilot I think the button positions are mapped via parameters. So if button 4 is pressed, the corresponding bit in manual control is set, and the assigned function is executed. This is the correct implementation IMO as documented.
914ab5b
to
0c45d8a
Compare
@hamishwillee Thanks for your input. I rebased the pr content on MAVLink master and added a second commit that changes to using separate aux fields instead of an array and explicitly listing all bits in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this. @auturgy Still OK with this to merge?
Thanks @MaEtUgR - we're all good with this. Merging! Thank you for your patience and all the design guidance. We're going to have to look at this again at some point, but this does make things pretty flexible. |
…nk#2031) * common: extend MANUAL_CONTROL with auxiliary continuous inputs * common: separate aux fields instead of array with separate validity bits
For various use cases additional knobs on the remote can be mapped to change the behavior of the vehicle on the fly e.g. change a tuning parameter, change the speed in one axis of the vehicle, move an actuator or payload, change the radius of the flown arc and similar. Since we cannot cover every single combination of such effect in any mode that might ever come up through the ground station sending a specific MAVLink message I suggest we introduce auxiliary continuous (aka analog) inputs which can then be mapped to have a certain effect depending on the operation mode on the receiving side.
Initial applications:
PX4 already allows mapping auxiliary RC channels to change a parameter on the fly or map certain payload functionality. The use case I'm specifically requesting this for is a mode that allows a custom mapping of knobs on the remote to continuously slow down certain movements of a drone.
To keep consistency with the existing message content I defined the new fields with the exact same range as
x
,y
,z
,r
normalized [-1000,1000] withINT16_MAX
representing an invalid/unused value.To cover the case where the extension is unknown I added a corresponding bit in the
enabled_extensions
mask which if the extension is unknown defaults to zero and makes the receiver not parse the new fields. This mechanism was introduced in #1674.