Fix BATTERY_STATUS voltage reporting #22009
Conversation
The extension fields need to be 0 by default according to the MAVLink spec. This is because extensions are 0 by default and need to be 0 when unknown/unused for backwards compatibility. The patch also simplifies the flow slightly in that it doesn't create a temporary array but just fills in the cell voltages directly. Signed-off-by: Julian Oes <julian@oes.ch>
If the measured voltage is more than 65v we need to split the voltage over multiple cells in order to avoid overflowing the uint16. This is according to the MAVLink spec. Signed-off-by: Julian Oes <julian@oes.ch>
There was a problem hiding this comment.
You thought about every case I went through 👍
It's good that you brought up the requirement for the zero-filled extension for compatibility. The original extension implementation did not follow this https://github.com/PX4/PX4-Autopilot/pull/17704/files#diff-a2164adeb58db340808a3233f23393127401db63695e200f4e0ba3beaada7d17R135 and I never changed that when reworking the logic. Also, the "distributed single cell" voltage support to work around MAVLink not defining a total voltage field makes sense.
Co-authored-by: Matthias Grob <maetugr@gmail.com>
|
Nice, it's working ok for me now. Just tested using the combination of PX4 master on a Cube Orange with MAVSDK main works ok. Thanks a lot. How about back porting to release/1.13 and release/1.14? Is that still required or am I missing something? The whole issue started with release/1.13 and MAVSDK main for me. |
|
Backport into v1.14 yes, v1.13 probably not. |
|
Ok, clear. Fwiw, I tried out your recent change for MAVSDK main in combination with 1.13 and reported voltage is ok. Same for 1.14 btw but indeed better to backport. |
|
My PR with almost the same changes was open since July and unreviewed: #21827 😕 Thanks for fixing that problem anyway! I'm glad it's in the main now. |
|
Oh, I'm sorry @pedorich-n. PX4 maintenance, issue triaging and PR reviewing is a big job, and there are only so many helping with that day by day. |
This fixes two issues with voltage reporting in BATTERY_STATUS:
The extension fields need to be 0 by default according to the MAVLink spec. This is because extensions are 0 by default and need to be 0 when unknown/unused for backwards compatibility.
If the measured voltage is more than 65v we need to split the voltage over multiple cells in order to avoid overflowing the uint16. This is according to the MAVLink spec.
Needs backporting to v1.14.
Probably best reviewed commit by commit.
Tested using a Pixhawk 6C with power module and playing with the voltage divider.
This was found by @reedev after mavlink/MAVSDK#2114.