fix(telemetry): stop emitting -0.001V sentinel when battery unavailable (#7958) - #10217
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
Fixes erroneous negative battery voltage telemetry by only populating DeviceMetrics.voltage when a real battery reading is available, preventing the -0.001V sentinel from leaking onto the wire.
Changes:
- Stop setting
has_voltageunconditionally in device telemetry. - Gate
voltagepopulation behindgetHasBattery()and a positive millivolt reading, and use afloatdivisor (1000.0f) to match the destination type.
| uint16_t batteryMv = powerStatus->getBatteryVoltageMv(); | ||
| if (powerStatus->getHasBattery() && batteryMv > 0) { | ||
| t.variant.device_metrics.has_voltage = true; | ||
| t.variant.device_metrics.voltage = batteryMv / 1000.0f; | ||
| } |
There was a problem hiding this comment.
powerStatus->getBatteryVoltageMv() returns an int (and can be -1 per PowerStatus ctor default). Storing it in a uint16_t will wrap -1 to 65535, making batteryMv > 0 true and causing has_voltage to be set with an incorrect ~65.5V reading. Use a signed type (e.g., int batteryMv) and gate on batteryMv > 0 (optionally also sanity-check an upper bound) before setting has_voltage/voltage.
|
@jaime-roldan can you please address the findings of the review? |
|
Hey @caveman99, sure thing, just checked and the copilot review is actually correct.
Left out the upper-bound sanity check copilot mentioned. Feels like if the power layer is returning 20000 mV then something is wrong deeper down and we shouldn't be masking it here. |
|
Validated on Nordic nRF54L15-DK — a USB-only board with no battery Before: outgoing DeviceTelemetry payloads carried After this PR: Builds clean (no footprint change). LGTM. Tested-by: cvaldess |
…le (meshtastic#7958) (meshtastic#10217) * fix(telemetry): stop emitting -0.001V sentinel when battery unavailable (meshtastic#7958) * address review: use int32_t for batteryMv to avoid uint16_t signed wrap
Summary
Fixes #7958.
Device telemetry packets currently emit
voltage = -0.0010000000474974513fornodes without a valid battery reading. That oddly-specific number is exactly the
doubleprintout of thefloatliteral-0.001f— a sentinel value that wasleaking onto the wire and being displayed to users as a real (negative) voltage.
Reporters confirmed this on TBEAM, HELTEC V2/V3, XIAO S3, XIAO NRF52, and
RAK4631 — identical bit pattern across wildly different hardware, which rules
out any measurement or hardware-specific cause.
Root cause
In
getDeviceTelemetry():has_voltage = truewas set unconditionally, on every telemetry packet.voltage = powerStatus->getBatteryVoltageMv() / 1000.0was assignedunconditionally. When
getBatteryVoltageMv()returned-1through anysigned path, dividing by 1000 produced
-0.001f.Because
has_voltagewas stampedtrueregardless, nanopb dutifully shippedthe sentinel over LoRa / BLE / MQTT. Downstream consumers (web client, Python
CLI, iOS/Android apps) had no way to know it meant "no reading" and rendered
it as a real voltage.
Fix
Only set
has_voltage = trueand assign a value when we actually have abattery and a non-zero reading. This matches the existing pattern used by
MAX17048Sensor::getMetrics()elsewhere in the tree, andDeviceMetrics.voltageis already defined as an optional field in the protobuf, so absence is a
valid wire state.
Also switched the divisor to
1000.0f(single-precision) to match thefloatdestination and avoid a pointless double round-trip.
Related cleanups (not in this PR — deliberately keeping scope small)
src/graphics/draw/UIRenderer.cpp:661has a defensivevoltage > 0.001fcheck that's a workaround for this exact bug. Worth leaving as-is for now —
it defends against older-firmware peers on the mesh still sending
-0.001,which will persist in the wild for a while. Can be simplified to just
has_voltagein a later cleanup PR.MeshPacketSerializer.cppandMeshPacketSerializer_nRF52.cppcurrentlyalways serialize
voltageinto JSON/MQTT output regardless ofhas_voltage.Those sites should probably gate on the flag too, so MQTT subscribers stop
seeing
"voltage": 0.0for nodes with no battery. Separate PR.Testing
has_voltage = trueandgates the existing assignment behind
getHasBattery() && batteryMv > 0,matching the pattern already used in
MAX17048Sensor::getMetrics().Backward compatibility
Older clients that don't check
has_voltagewill seevoltage == 0.0(thedefault for an unset
floatin zero-initialized nanopb structs) instead of-0.001. This is strictly better: no client rendered-0.001Vcorrectly,whereas
0.0is either shown as "0.0V" (acceptable) or filtered out.Recent first-party clients already read
has_voltagecorrectly.