Add new PWM output message

[LorenzMeier]

This new message is streamlined and ensures correct timestamping. It also has improved wording to not be specific to servos.

It fixes #153 (comment)

Please comment on the fields and make suggestions for improvements.

[amilcarlucas]

Do not add units in the description fields, it is redundant.
And ... it is non machine parseable.
And we want to encourage GCS to use the attribute field, right ?

[amilcarlucas]

This PR should also mark the old message as deprecated

[hamishwillee]

OK, I added the zero values text and removed the units from descriptions.

Just FMI, can you confirm that

• the actual mapping of motor number here to physical pins is flight stack and hardware dependent
• why are we interested in raw PWM values? Ie these would only be useful if you know what the range is, which implies the precise servo/actuator attached.

This PR should also mark the old message as deprecated.

Doesn't that happen after we've started implementing it in Autopilots?

[hamishwillee]

Can everyone re-review and confirm they are happy for this to be merged?

[WickedShell]

I know we discussed on the call making this an array instead (which allows users to easily iterate over it). Was that rejected for some reason? (I don't recall if we locked in that we wanted it or not at this point)

[WickedShell]

This is describing PWM not PPM channels. The standard description also doesn't map to a lot of exsisting hardware or actuators, and I'd probably say it's worth just removing that part.

[hamishwillee]

How would you re-write it?

[WickedShell]

I'd drop the entire sentence about standard PPM modulation. Probably not worth capitalzing the RAW part in the first sentence either.

[hamishwillee]

Makes sense to me. Done in https://github.com/mavlink/mavlink/pull/926/files#diff-ceacc1e2390186650146a45be8fcb691

[hamishwillee]

I know we discussed on the call making this an array instead (which allows users to easily iterate over it). Was that rejected for some reason? (I don't recall if we locked in that we wanted it or not at this point)

I don't recall the discussion. If this was last week, I wasn't there :-) FWIW it sounds like a good idea. Would unused (zero value) elements be truncated from the array? If so, that might well be another benefit.

[WickedShell]

I don't recall the discussion. If this was last week, I wasn't there :-) FWIW it sounds like a good idea. > Would unused (zero value) elements be truncated from the array? If so, that might well be another benefit.

I was suggesting it during the call since it allows GCS's/AutoPilots to easily just iterate over the array (which you can technically do by casting right now, but that's a dirty hack that only happens to work at the moment because of the packing). The zero truncation would happen either way, the main advantage of using 0 for the no value is that it works well with any future extensions.

[hamishwillee]

@LorenzMeier @amilcarlucas You OK with turning the individual outputs into an array? I have no reason to doubt it will be easier for GCS to process.

Otherwise this looks good to me. @amilcarlucas As above, I think deprecation happens at some future point after ArduPilot and PX4 have a chance to implement this. That was my understanding anyway.

[magicrub]

Why not make them all signed and add a format enum like UAVCAN?

[hamishwillee]

@magicrub

Why not make them all signed and add a format enum like UAVCAN?

Since I don't know UAVCAN example you are talking about:

1. Why is this a good idea?
2. What benefit does format enum give us given these are raw
   • are you proposing that we use format to show "raw" or "%" (etc) so we can use this same message for different info about servos/motors?
   • Would signed then make sense in all cases? For example, might a format allow negative values?
3. What would that look like?
4. Still an array?

[hamishwillee]

@magicrub Can you please respond to #926 (comment) -

@LorenzMeier @amilcarlucas Thoughts on #926 (comment)

[amilcarlucas]

I vote for an array. And no enum, use different message for % if needed.
Both code and documentation are easier to maintain without having the meaning of the fields depend on some enum.

[magicrub]

The UAVCAN msg I was referring to is equipment/actuator/Command which is which is of a single output:

uint8 actuator_id

# Whether the units are linear or angular depends on the actuator type.
uint8 COMMAND_TYPE_UNITLESS     = 0     # [-1, 1]
uint8 COMMAND_TYPE_POSITION     = 1     # meter or radian
uint8 COMMAND_TYPE_FORCE        = 2     # Newton or Newton metre
uint8 COMMAND_TYPE_SPEED        = 3     # meter per second or radian per second
uint8 command_type

# Value of the above type
float16 command_value

And then that's in a variable sized array of equipment/actuator/ArrayCommand

# Actuator commands.
# The system supports up to 256 actuators; up to 15 of them can be commanded with one message.

Command[<=15] commands

This allows for mixed actuator output units with variable length and since the _id is in there you can send them out-of-order or partial packets with servos that update at different rates. However, I agree it's a lot of data and in MAVLink it likely won't be used to it's potential and will just slow down the link. Also what's missing is unit type of "RC PWM" or something that would represent that 1000-2000us PWM signal that this would typically be used for in the near/medium future.

[hamishwillee]

Sorry, accidental close!

[hamishwillee]

@magicrub - thanks for the suggestion and explanation. I tend to agree with @amilcarlucas that having multiple messages provides a "cleaner" definition -and it is certainly easier for GCS to parse. We do have some cases where we do use the enum approach, but they tend to be more for internal protocol messages that won't ever have to be exposed via the UI.

That said, is it useful to expose the values as a percentage too?

[hamishwillee]

I have made this an array now.

1. Is there a defined mapping between index/servo numbers and ports on the device?

My assumption is that numbers 1-8 map must map to MAIN and 9-16 map to AUX (if present) - and we should say that.

Also that if Aux is not present that does not matter because the values will be 0 (truncated).

2. Would there ever be a case where we just want to send MAIN or just AUX ports?
3. Can there ever be more than 16 ports?

If either of these are true, then we would be better off with a field for the index of the affected port and an 8-value array.

[WickedShell]

My assumption is that numbers 1-8 map must map to MAIN and 9-16 map to AUX (if present) - and we should say that.

I think this is out of scope for us to be specifying in MAVLink. The output numbering should always be considered to be an attribute of what you are talking to. The concept of main/aux outputs is not something that should be documented here, particularly if it's just a dense array of outputs. I think this meaning will always have to be specific to the autopilot you are communicating with.

0 indicating unused works, but you have an overlap then that we also send 0 as outputs when we are sending no pulses on an otherwise valid channel, and we may start sending values later. If 0 simply means unused a GCS won't be able to differentiate between not sending pulses and not a valid channel.

[hamishwillee]

My assumption is that numbers 1-8 map must map to MAIN and 9-16 map to AUX (if present) - and we should say that.

I think this is out of scope for us to be specifying in MAVLink. The output numbering should always be considered to be an attribute of what you are talking to. The concept of main/aux outputs is not something that should be documented here, particularly if it's just a dense array of outputs. I think this meaning will always have to be specific to the autopilot you are communicating with.

@WickedShell I'm OK with that.

Still, this does assume a system with max 16 ports. Is it better to have 8 ports per message and have index for which array you're using?

0 indicating unused works, but you have an overlap then that we also send 0 as outputs when we are sending no pulses on an otherwise valid channel, and we may start sending values later. If 0 simply means unused a GCS won't be able to differentiate between not sending pulses and not a valid channel.

I guess we could add an index for the max valid array index. Or we could use UINT16_MAX for unused - but that then means we can't take advantage of zero-byte truncation. Thoughts?

[hamishwillee]

Did we decide against adding a uint32 index to allow larger numbers of actuators?

Otherwise looks as discussed?

[hamishwillee]

Replaced by #1148