Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Battery message - extend with status, current, and fix percentage #290

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Battery message - extend with status, current, and fix percentage #290

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
64c4a9b
Battery message - extend with status, current, and fix percentage
hamishwillee Jun 2, 2022
c138772
Change flag to indicate not ready to fly
hamishwillee Jun 3, 2022
834099d
Add all the fault/status flags
hamishwillee Jun 12, 2022
79e6aeb
Update protos/telemetry/telemetry.proto
hamishwillee Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion protos/telemetry/telemetry.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,11 +564,37 @@ enum FixType {
FIX_TYPE_RTK_FIXED = 6; // RTK Fixed, 3D position
}


enum BatteryStatusFlags {
BATTERY_STATUS_FLAGS_READY_TO_USE = 1; // The battery is ready to use (fly).
BATTERY_STATUS_FLAGS_CHARGING = 2; // Battery is charging.
BATTERY_STATUS_FLAGS_CELL_BALANCING = 4; // Battery is cell balancing (during charging).
BATTERY_STATUS_FLAGS_FAULT_CELL_IMBALANCE = 8; // Battery cells are not balanced.
BATTERY_STATUS_FLAGS_AUTO_DISCHARGING = 16; // Battery is auto discharging (towards storage level).
BATTERY_STATUS_FLAGS_REQUIRES_SERVICE = 32; // Battery requires service (not safe to fly).
BATTERY_STATUS_FLAGS_BAD_BATTERY = 64; // Battery is faulty and cannot be repaired (not safe to fly).
BATTERY_STATUS_FLAGS_PROTECTIONS_ENABLED = 128; // Automatic battery protection monitoring is enabled.
BATTERY_STATUS_FLAGS_FAULT_PROTECTION_SYSTEM = 256; // The battery fault protection system had detected a fault and cut all power from the battery.
BATTERY_STATUS_FLAGS_FAULT_OVER_VOLT = 512; // One or more cells are above their maximum voltage rating.
BATTERY_STATUS_FLAGS_FAULT_UNDER_VOLT = 1024; // One or more cells are below their minimum voltage rating.
BATTERY_STATUS_FLAGS_FAULT_OVER_TEMPERATURE = 2048; // Over-temperature fault.
BATTERY_STATUS_FLAGS_FAULT_UNDER_TEMPERATURE = 4096; // Under-temperature fault.
BATTERY_STATUS_FLAGS_FAULT_OVER_CURRENT = 8192; // Over-current fault.
BATTERY_STATUS_FLAGS_FAULT_SHORT_CIRCUIT = 16384; // Short circuit event detected.
BATTERY_STATUS_FLAGS_FAULT_INCOMPATIBLE_VOLTAGE = 32768; // Voltage not compatible with power rail voltage (batteries on same power rail should have similar voltage).
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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ID number is too high for protobuf (hence the prototool failure in the CI) 😁

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that's the thing, we don't support it because the numbers are removed in the C++ API.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@JonasVautherin JonasVautherin Jun 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try with [(mavsdk.options.default_value)="false"], I think that should work 😇

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that looks right.

}

// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also be A instead of mA but then we are quite different from the MAVLink message.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change if you want. Just let me know.

BatteryStatusFlags status = 5; // Battery status flags
}

/*
Expand Down