Conversation
| uint16 esc_errorcount # Number of reported errors by ESC - if supported | ||
| int16 esc_rpm # Motor RPM, negative for reverse rotation [RPM] - if supported |
There was a problem hiding this comment.
Would be good to at least update the fields you touch to current docs standard https://docs.px4.io/main/en/uorb/uorb_documentation#uorb-documentation-standard
Also if you wouldn't mind a short description and long description up the top, though that is much more optional
Is the invalid value NaN as I have indicated?
| uint16 esc_errorcount # Number of reported errors by ESC - if supported | |
| int16 esc_rpm # Motor RPM, negative for reverse rotation [RPM] - if supported | |
| uint16 esc_errorcount # [-] [@invalid NaN] Number of reported errors by ESC - if supported | |
| int16 esc_rpm # [rpm] [@invalid NaN] Motor RPM, negative for reverse rotation - if supported |
| float32 esc_voltage # Voltage measured from current ESC [V] - if supported | ||
| float32 esc_current # Current measured from current ESC [A] - if supported | ||
| float32 esc_temperature # Temperature measured from current ESC [degC] - if supported | ||
| int16 esc_temperature # Temperature measured from current ESC [degC] - if supported |
There was a problem hiding this comment.
| int16 esc_temperature # Temperature measured from current ESC [degC] - if supported | |
| int16 esc_temperature # [degC] [@invalid NaN] Temperature measured from current ESC - if supported |
There was a problem hiding this comment.
Hey @ttechnick, thanks for working on this! I looked into this since the Cyphal builds are failing in CI and wanted to share some findings that might help.
CI build error: The Cyphal targets fail because src/drivers/cyphal/Actuators/EscClient.hpp:249 assigns NAN to esc_temperature, which can't be represented in int16_t:
EscClient.hpp:249:39: error: overflow in conversion from 'float' to 'int16_t'
changes value from '+QNaNf' to '0' [-Werror=overflow]
249 | ref.esc_temperature = NAN;
Are you still working through the remaining consumers of esc_temperature? In case it helps, here are the spots I found that still need updating:
Writers:
src/drivers/cyphal/Actuators/EscClient.hpp:249— assignsNAN(the build failure)src/drivers/uavcan/actuators/esc.cpp:117— float arithmetic with-273.15fsrc/drivers/actuators/vertiq_io/vertiq_telemetry_manager.cpp:123—mcu_temp * 0.01produces a floatsrc/modules/simulation/simulator_mavlink/SimulatorMavlink.cpp:160— double expressionboards/modalai/voxl2-slpi/src/drivers/dsp_hitl/dsp_hitl.cpp:506— same pattern
Readers:
src/modules/mavlink/streams/ESC_INFO.hpp:115— multiplies by100.f, so whatever sentinel replacesNANwould need to be handled here to avoid sending a bogus value over MAVLinksrc/drivers/uavcannode/Publishers/ESCStatus.hpp:84— copies to a UAVCAN float field
You'll also want to settle on a consistent sentinel value (looks like you're going with INT16_MAX based on the EscBattery.cpp change) and make sure all writers use it instead of NAN, and all readers check for it instead of PX4_ISFINITE().
One concern on esc_rpm: I did a pass through the drivers and int16 (max 32,767) might be too narrow for RPM. A few examples:
- DShot — a 14-pole motor at ~2500 eRPM gives ~35,714 RPM (common real-world scenario)
- UAVCAN/DroneCAN — source field is 18-bit, up to 131,071 RPM
- HoTT —
raw * 10, so anything above 3276 raw overflows - VOXL ESC — extended mode supports up to 65,530 RPM
- Cyphal — 13-bit velocity field × 9.55 ≈ up to ~39,104 RPM
Only the simulators (max 6000) and TAP ESC (already int16 source) are safe. This would silently truncate/wrap on real hardware without any warning, which could be confusing for users checking telemetry. Might be worth keeping esc_rpm as int32 and focusing the size savings on the other fields?
Let me know if any of this is helpful or if you'd already spotted these.
dakejahl
left a comment
There was a problem hiding this comment.
Are you sure this captures all of the uses of EscStatus and EscReport?
| uint32 esc_errorcount # Number of reported errors by ESC - if supported | ||
| int32 esc_rpm # Motor RPM, negative for reverse rotation [RPM] - if supported | ||
| uint16 esc_errorcount # Number of reported errors by ESC - if supported | ||
| int16 esc_rpm # Motor RPM, negative for reverse rotation [RPM] - if supported |
There was a problem hiding this comment.
singed 16 bit has a max of 32767 - too low to cover all vehicle types. Also ESC_STATUS uses 32 bit.
| esc_status.esc[telemetry_index].esc_voltage = static_cast<float>(data.voltage) * 0.01f; | ||
| esc_status.esc[telemetry_index].esc_current = static_cast<float>(data.current) * 0.01f; | ||
| esc_status.esc[telemetry_index].esc_temperature = static_cast<float>(data.temperature); | ||
| esc_status.esc[telemetry_index].esc_temperature = static_cast<int>(data.temperature); |
There was a problem hiding this comment.
| esc_status.esc[telemetry_index].esc_temperature = static_cast<int>(data.temperature); | |
| esc_status.esc[telemetry_index].esc_temperature = static_cast<int16_t>(data.temperature); |
and throughout
| total_current_a += esc_status.esc[i].esc_current; | ||
|
|
||
| if (PX4_ISFINITE(esc_status.esc[i].esc_temperature)) { | ||
| if (esc_status.esc[i].esc_temperature != INT16_MAX) { |
There was a problem hiding this comment.
Did you check that ESC temp gets properly init to INT16_MAX?
There was a problem hiding this comment.
Wait, why is EscBattery using the ESC temperature as battery temperature? This seems completely wrong.
| float average_voltage_v = 0.0f; | ||
| float total_current_a = 0.0f; | ||
| float average_temperature_c = 0.0f; | ||
| int average_temperature_c = 0; |
There was a problem hiding this comment.
you can keep as float, since Battery::updateTemperature(float temp) function signature has float param
|
Some context: Question: How important is this? Is communication bandwidth an issue currently? |
Solved Problem
Fixes #26416
This pull request updates the data types used for ESC (Electronic Speed Controller) telemetry fields to use smaller types. The main focus is on reducing memory usage.
Message and Data Structure Updates:
esc_errorcountfromuint32touint16esc_rpmfromint32toint16esc_temperaturefromfloat32toint16in
EscReport.msgto reduce memory usage and standardize data types.