-
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
Add messages for power regulation and temperature status reporting. #1234
Conversation
message_definitions/v1.0/common.xml
Outdated
@@ -3472,6 +3473,28 @@ | |||
<description>The location of the remote pilot is a fixed location.</description> | |||
</entry> | |||
</enum> | |||
<enum name="REGULATOR_STATUS_FLAG"> | |||
<entry value="1" name="REGULATOR_STATUS_HEALTHY"> | |||
<description>The regulator channel is operating normally.</description> |
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.
Do you think a normal reader will know what a regulator is? Should this be voltage regulator?
What what is a regulator channel? (note, I don't know what the architecture is that makes channel the right word here).
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'm happy to change the description to describe as 'voltage regulator' (technically, our aircraft do have some constant current regulator devices as well, but we could deal with the description being technically inaccurate).
I don't know, "channel" just seemed like a word to describe what the different regulated things handled by a single MAVLink component might be called. 'Buses' or 'rails'? Don't really mind.
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.
We have a dev call meeting this week weds, so if we don't get general feeling before then I'll see what others think.
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.
Actually I got it wrong. Meeting is this week sorry. Will make sure it gets looked at.
message_definitions/v1.0/common.xml
Outdated
<description>Status from a power regulator.</description> | ||
<field type="uint8_t" name="channel">ID number for the regulator channel.</field> | ||
<field type="char[16]" name="name">Descriptive name for the regulator channel; terminated by NULL if length less than 16 human-readable chars, and without if length is exactly 16.</field> | ||
<field type="uint16_t" name="v_nom">Nominal voltage of the regulator channel, in mV. If unknown, set to 65535.</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.
Any particular reason not to have as voltage_nominal
(etc for all cases). Might make the generated API easier to use.
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.
Only because my personnel preference when I have a set of otherwise identical variables holding nominal, actual, maximum and minimum type data is to keep the variable names all the same lengths! Can change if it's preferred differently.
message_definitions/v1.0/common.xml
Outdated
<description>Status from a power regulator.</description> | ||
<field type="uint8_t" name="channel">ID number for the regulator channel.</field> | ||
<field type="char[16]" name="name">Descriptive name for the regulator channel; terminated by NULL if length less than 16 human-readable chars, and without if length is exactly 16.</field> | ||
<field type="uint16_t" name="v_nom">Nominal voltage of the regulator channel, in mV. If unknown, set to 65535.</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.
For all cases, remove the units from the description and put in units="mV"
etc.
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.
wilco
message_definitions/v1.0/common.xml
Outdated
<field type="uint16_t" name="v_min">Minimum permissible voltage, in mV. Set to 65535 for no limit.</field> | ||
<field type="uint16_t" name="i_act">Measured current of the regulator channel, in mA. If unmeasured, set to 65535.</field> | ||
<field type="uint16_t" name="i_max">Maximum permissible current, in mA. Set to 65535 for no limit.</field> | ||
<field type="uint16_t" name="flags">Bitmap of regulator status flags.</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.
Bitmaps normally done like this:
<field type="uint16_t" name="flags" enum="REGULATOR_STATUS_FLAG" display="bitmask">Bitmap of regulator status flags.</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.
wilco
message_definitions/v1.0/common.xml
Outdated
<field type="uint8_t" name="channel">ID number for the regulator channel.</field> | ||
<field type="char[16]" name="name">Descriptive name for the regulator channel; terminated by NULL if length less than 16 human-readable chars, and without if length is exactly 16.</field> | ||
<field type="uint16_t" name="v_nom">Nominal voltage of the regulator channel, in mV. If unknown, set to 65535.</field> | ||
<field type="uint16_t" name="v_act">Measured voltage of the regulator channel, in mV. If unmeasured, set to 65535.</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.
Usually specify UINT16_MAX rather than the value 65535.
So in this case
<field type="uint16_t" name="v_act" units="mV">Measured voltage of the regulator channel. UINT16_MAX: measured voltage not sent.</field>
Note logically it would be sent if it were measured, but as a recipient we don't care - all we know is we didn't get it.
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.
wilco
message_definitions/v1.0/common.xml
Outdated
<field type="char[16]" name="name">Descriptive name for the regulator channel; terminated by NULL if length less than 16 human-readable chars, and without if length is exactly 16.</field> | ||
<field type="uint16_t" name="v_nom">Nominal voltage of the regulator channel, in mV. If unknown, set to 65535.</field> | ||
<field type="uint16_t" name="v_act">Measured voltage of the regulator channel, in mV. If unmeasured, set to 65535.</field> | ||
<field type="uint16_t" name="v_max">Maximum permissible voltage, in mV. Set to 65535 for no limit.</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.
As above.
message_definitions/v1.0/common.xml
Outdated
@@ -5469,6 +5492,31 @@ | |||
<field type="uint16_t" name="payload_type" enum="MAV_TUNNEL_PAYLOAD_TYPE">A code that identifies the content of the payload (0 for unknown, which is the default). If this code is less than 32768, it is a 'registered' payload type and the corresponding code should be added to the MAV_TUNNEL_PAYLOAD_TYPE enum, and the entry possibly to https://github.com/mavlink/mavlink/tunnel-message-payload-types.xml. Software creators can register blocks of types as needed. Codes greater than 32767 are considered local experiments and should not be checked in to any widely distributed codebase.</field> | |||
<field type="uint8_t[128]" name="payload">Variable length payload. The payload length is defined by the remaining message length when subtracting the header and other fields. The entire content of this block is opaque unless you understand the encoding specified by payload_type.</field> | |||
</message> | |||
<message id="5008" name="REGULATOR_STATUS"> |
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.
As above, should this be "POWER_REGULATOR_STATUS". I don't know enough to make assumptions on what a normal user might assume this means.
@auturgy @WickedShell @MaEtUgR Anything to add? |
just for inspiration: UAVCAN (v0) typically uses "xxxx_id", e.g. sensor_id, gimbal_id, actuator_id, circuit_id ... and device_id in the temperature measurement message ... just for inspiration |
<field type="uint8_t" name="channel">ID number of the thermometer channel.</field> | ||
<field type="char[16]" name="name">Descriptive name for the thermometer channel; terminated by NULL if length less than 16 human-readable chars, and without if length is exactly 16.</field> | ||
<field type="uint16_t" name="t_act" units="Kelvin">Measured temperature from the thermometer channel.</field> | ||
<field type="uint16_t" name="t_max" units="Kelvin">Maximum permissible temperature for this channel. Set to UINT16_MAX for no limit.</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.
Why Kelvin. centiKelvin makes a bit more sense.
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.
Why Kelvin at all (just FMI)? Yes you can use an int16, but with the ranges we're talking might we be better of just using a float and degress or centidegrees?
@julianoes Any comments. @auturgy Anything to add. |
@amilcarlucus - Ah yes, good point regarding temp units. uint16, so I guess that would mean a max representable temperature of 300 degrees Celsius, which is probably fine for most purposes. Hmm, I guess a gas engine could get that high? uint32 kinda blows up the message size. Could do deci-Kelvin? Most temp sensors aren't accurate better than that anyway I don't think. |
@olliw42 - Regarding "channel": yeah I know - the motivation for my creating these messages is literally to convert the UAVCAN messages you're referring to into a MAVLink representation! Perhaps I should just call the field "id" instead of "channel" then? |
many messages already use int16_t and cdegC or float and degC. On the quick I couldn't find any with Kelvin but plenty with C. Maybe one would want to follow the majority. Kelvin is not a very practical unit anyways. Besides not liking "channel" I have not really a suggestion. Having no idea myself, I thought one could just follow what is done elsewhere, which would suggest "regulator ID" and "thermometer ID", or similar. shouldn't this be more uppercase, like V_nom, I_act, T_max, etc |
<wip/> | ||
<!-- This message is work-in-progress it can therefore change, and should NOT be used in stable production environments --> | ||
<description>Status from a device which measures temperature.</description> | ||
<field type="uint8_t" name="channel">ID number of the thermometer channel.</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.
does "channel" refer to instance? If so, perhaps "instance" might be more consistent with other messages
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.
Yes, essentially: the avionics in question has a single component monitoring about seven or eight different temp sensors across the aircraft.
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.
Yes, IMO change to instance (terminology seen more commonly)
<field type="uint16_t" name="flags" enum="VOLTAGE_REGULATOR_STATUS_FLAGS" display="bitmask">Bitmap of regulator status flags.</field> | ||
</message> | ||
<message id="5009" name="TEMPERATURE_STATUS"> | ||
<wip/> |
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.
General comment: unit16_max is 65.535 volts which sounds a lot (~16S) - but if we are introducing a new message, should we consider tether systems? We might think about sacrificing resolution for the benefit in range (I'm not convinced either way, but worth the discussion - not blocking).
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.
Yeah, tether systems, or bigger air-taxi type aircraft could be about that; 65V was fine for my purposes so I didn't put any additional consideration into it.
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.
@auturgy Seems reasonable. What's the maximum voltage we know of on a tethered system?
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.
Most commercially available systems are ~400VDC, although there may be some that go higher.
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.
Should we consider centiVolts then ?
Another thought. What rate are you planning on sending this at? If it is low, then the message size probably not such a big deal. If it is high you might want to split this like we did for the Battery Status - low rate for invariant information, and smaller high rate message if you need regular updates. |
@hamishwillee - In our current implementation, I think was sending at 2Hz? Hmm, no maybe it was 0.5Hz. Yes, the reason I started with smaller fixed point values rather than int64 for everything was in consideration of keeping the wire size sane. Anyway, I've now mostly stepped away from my role at Aeronavics, so I'm not going to be able to drive this PR any longer. I'll tag in @tilaktilak (and @AntonSlooten) from here. |
@edwinhayes Thanks for letting me know - sorry this dragged on so long. @tilaktilak (and @AntonSlooten) , FYI typically issues move slower or not at all without a champion. Any opinions/can you drive this? |
Yep I'll follow up on that |
I'm closing this as "has no stakeholder". @tilaktilak / @AntonSlooten - feel free to reopen when you're ready to work on this. |
This PR introduces two new messages, for reporting status of onboard voltage regulators, and onboard temperature sensors respectively.
The existing
POWER_STATUS
message is extremely specific to Pixhawk family flight controllers. Similarly, temperature is only ever conveyed as part of another data type mapped to a specific sensor, e.g. IMU. There are no messages suitable for reporting data from an aircraft which has a bunch of independent sensors and power regulator channels.In the long term, I'd propose that these new messages replace the existing hardware specific messages, but for now they can coexist happily.