Battery message - extend with status, current, and fix percentage #290
Conversation
protos/telemetry/telemetry.proto
Outdated
| BATTERY_STATUS_FLAGS_FAULT_INCOMPATIBLE_FIRMWARE = 65536; // Battery firmware is not compatible with current autopilot firmware. | ||
| BATTERY_STATUS_FLAGS_FAULT_INCOMPATIBLE_CELLS_CONFIGURATION = 131072; // Battery is not compatible due to cell configuration. | ||
| BATTERY_STATUS_FLAGS_CAPACITY_RELATIVE_TO_FULL = 262144; // Battery capacity values are relative to a known-full battery (smart battery) and not estimated. | ||
| BATTERY_STATUS_FLAGS_EXTENDED = 4294967295; // Reserved for future use. |
There was a problem hiding this comment.
This ID number is too high for protobuf (hence the prototool failure in the CI) 😁
There was a problem hiding this comment.
We generally don't support enums used as bitflags in MAVSDK, right @JonasVautherin?
So what we can do is add boolean values for it and then convert between the flags and bools.
I think this makes using the API easier for other languages, where it's not as common to do bitwise checks.
There was a problem hiding this comment.
Well as long as the number is in bounds, it's fine. Usually we just increment from 0, but well... I don't really mind if we want to use weird ids 😅.
However the protobuf field number is only a protobuf thing, it's completely independent from MAVLink. So using powers of two has no meaning here 👍
There was a problem hiding this comment.
So that's the thing, we don't support it because the numbers are removed in the C++ API.
There was a problem hiding this comment.
Sorry I did not get that. In the C++ implementation, the code will have to translate this enum into the mavlink message. But using powers of 2 here doesn't do anything, so it would probably be cleaner to just increment from 0.
There was a problem hiding this comment.
No, the enum is fine. As a MAVSDK user, I'd rather deal with BATTERY_STATUS_FLAGS_EXTENDED than 4294967295. I don't care how the enum is encoded on the wire, be it with a bitmask, with booleans, little-endian or not, UTF8 or not, etc.
@JonasVautherin I don't think this would work. The problem is that the MAVLink spec is not really an enum but it's bitflags.
So in our conversion the enum becomes a normal enum incremented with 1 (instead of doubled for bits), so you can only ever set one flag at a time. In the MAVLink world you can set multiple bits.
Therefore, I'm suggesting to use boolean flags for each flag, and then encode it to bits internally.
There was a problem hiding this comment.
Ooooooh, I get it now 👍. Yeah then a boolean makes total sense 💯. Actually that's what a flag represents, it's just that under the hood mavlink represents it as a bitset. But those are booleans indeed 👍
There was a problem hiding this comment.
So is the format I suggested right?
bool status_not_ready_to_use= 5 [(mavsdk.options.default_value)="0"]; // Battery status: not ready to use
bool status_charging= 6 [(mavsdk.options.default_value)="0"]; // Battery status: charging
...
There was a problem hiding this comment.
I would try with [(mavsdk.options.default_value)="false"], I think that should work 😇
| // Battery type. | ||
| message Battery { | ||
| uint32 id = 3 [(mavsdk.options.default_value)="0"]; // Battery ID, for systems with multiple batteries | ||
| float voltage_v = 1 [(mavsdk.options.default_value)="NaN"]; // Voltage in volts | ||
| float remaining_percent = 2 [(mavsdk.options.default_value)="NaN"]; // Estimated battery remaining (range: 0.0 to 1.0) | ||
| float remaining_percent = 2 [(mavsdk.options.default_value)="NaN"]; // Estimated battery remaining (range: 0 to 100) | ||
| float current_ma = 4 [(mavsdk.options.default_value)="NaN"]; // Current in mA |
There was a problem hiding this comment.
Could also be A instead of mA but then we are quite different from the MAVLink message.
There was a problem hiding this comment.
I can change if you want. Just let me know.
|
@hamishwillee what's the status here? Is this good to go in? |
|
@julianoes Ha ha ha ha :-) No. The battery messages are still a little in flux. Since MAVSDK takes common rather than development, I don't want this in yet, |
|
@hamishwillee FYI the v1 PR is in. |
This is proposed proto to match the updated battery status updates in mavlink/MAVSDK#1792
It fixes the percentage remaining to be 0-100 (docs only) and adds current and status flags as suggested by @julianoes
However, in draft because the status flags are still in flux. This shouldn't be merged!
But would like review to sanity check it isn't wrong/broken (then I can try next update step)