New BATTERY_STATUS_V2 #1792

hamishwillee · 2022-05-26T08:04:10Z

This is PR for a new battery status message BATTERY_STATUS_V2 that is being added in (under discussion) RFC: mavlink/rfcs#19

This message has been added in mavlink/mavlink#1846 into development.xml

Needs to be configured using cmake -Bbuild/default -DMAVLINK_DIALECT=development -H.

Note, first version just allows easy testing.

It pulls in the mavlink git_tag in a commit that can later be removed.
adds handler for new message. Does not update proto with new fields.

Need to agree proto changes and fixes. Also whole RFC needs to be agreed before this can be merged.

hamishwillee · 2022-05-26T08:08:27Z

src/mavsdk/plugins/telemetry/telemetry_impl.cpp

+    Telemetry::Battery new_battery;
+    new_battery.id = bat_status.id;
+    new_battery.voltage_v = bat_status.voltage; 
+    //Is it correct to set this to NaN for UINT32_MAX? HOw do you specify = NaN?


@julianoes The old version seems to ignore possibility that these values might be arriving as UINT32_MAX or whatever.
The doc says

float mavsdk::Telemetry::Battery::voltage_v {float(NAN)}

Does that mean we need to check for a UINT32_MAX and set to NaN? Hints?

Yes, good point. We should check for it and set NAN if it's std::limits<uint32_t>::max, the C++ way or UINT32_MAX 😂.

Cool. voltage fixed above. But leaving battery remaining until I understand if we can/should make that an int

hamishwillee · 2022-05-26T08:09:10Z

src/mavsdk/plugins/telemetry/telemetry_impl.cpp

+    //Is it correct to set this to NaN for UINT32_MAX? HOw do you specify = NaN?
+
+    // FIXME: it is strange calling it percent when the range goes from 0 to 1.
+    // Can we fix this? What about if value not provided?


See this fixme? Would be good to break this and send a percentage at some point. How are you managing those kinds of changes?

Yes, there are a few that are wrong like that. Thanks for pointing it out. Version 2 is coming soon, so now is the time to break/fix it.

Here is where it needs fixing first:
https://github.com/mavlink/MAVSDK-Proto/blob/e8526d1fd8505fa3bc47eb1ea5edb31e58cb3be0/protos/telemetry/telemetry.proto#L571

@julianoes Thanks, but I think I need a bit more info.
This is what it looks like now:

float remaining_percent = 2 [(mavsdk.options.default_value)="NaN"]; // Estimated battery remaining (range: 0.0 to 1.0)

As I understand it, to allow us to break this, first I change the name. So
float percent_remaining = 2 [(mavsdk.options.default_value)="NaN"]; // Estimated battery percentage remaining (range: 0 to 100).

What is the value after =? In this case the value is 2. My guess would be the default initialisation, but then we afterwards state mavsdk.options.default_value=NaN. That would imply to me that the default is NaN?

This is sent as a uint8. It's an integer with an invalid value of UINT8_MAX. Can we do something like (?):
uint8 percent_remaining = 255 [(mavsdk.options.default_value)="UINT8_MAX"]; // Estimated battery percentage remaining (range: 0 to 100).

Not exactly 🙂. I believe that the only thing to change there is the docs (((range: 0.0 to 1.0) to (range: 0 to 100)). Let me explain:

remaining_percent follows the convention that the "unit" goes at the end, like velocity_m_s and time_s. I don't have a strong opinion here so if you insist, we can rename it 👍.

The number after = is for protobuf. In protobuf, each field has an "id", which is used in the binary representation. Something like the msgid in MAVLink, I guess. We never want to change it.

[(mavsdk.options.default_value)="NaN"] This is not used by protobuf, just by our MAVSDK code generators. The thing is that "UINT8_MAX" is a C definition, and this value could be used by the templates in all languages. We could have the templates translate that to the right value for each language, but... why would we set a float default value to uint8_max? Feels like NaN makes more sense for an undefined float 🤔.

I would say _percent should go from 0..100. If there is no unit, it should go from 1.0. I know this might not be inline with what I have implemented earlier (case and point here) but I'd say that's the most intuitive.

I agree with @JonasVautherin that we can't have magic numbers in the API except NAN.

@julianoes @JonasVautherin So the reason I wanted to change the name was that Julian said that we change the name on API break - I presume the assumption being that it fails a compile and users check what's changed.

So I am happy to change (range: 0.0 to 1.0) to (range: 0 to 100) but don't we need to rename it something? I could call it charge_percent if you prefer?

that we change the name on API break

We can change the name on API break if we want to. It's just an opportunity, it's not mandatory. I think there was a misunderstanding there, right? 😁

It's just that for semantic versioning, every time we break the API, we have to increase the major version. So we don't want to be on MAVSDK v503 just because we rename stuff 😁. However, we will soon move to MAVSDK v2 (we had a real need to break the API), so that's an opportunity to rename small stuff (and break more of the API on this occasion 😆).

Does that make sense?

Absolutely!

hamishwillee · 2022-05-26T08:10:05Z

src/mavsdk/plugins/telemetry/telemetry_impl.cpp

+    // Can we fix this? What about if value not provided?
+    new_battery.remaining_percent = bat_status.percent_remaining; // to test * 1e-2f;
+
+    //bat_status also has:


Other fields are shown below. I think it worth supplying everything. Thoughts?

My understanding is I'd update the proto and rebuild to get appropriate headers.

Even more fields are in https://mavlink.io/en/messages/common.html#SMART_BATTERY_INFO . I think we need to catch some of these too - at least the battery function.

What else would you want as a user?

But these are static/low rate. I guess we just get the battery info when it comes in once. Otherwise those fields are NAN.

The current seems like a good indicator, and status flags, and temperature is also important.
I would want to ignore the mAh fields as they are somewhat battery specific/internal, at least for smart batteries.

Really depends on what you expect your consumers to be able to do with the info. Right now, we're saying that the only thing of interest to a user is the % battery remaining. [technically we're also giving them the voltage, but this is of no use without context provided by SMART_BATTERY_INFO].

With the update I think you're saying that we might also give them current. But what does that actually tell them - without knowing capacity and consumption, you can't do anything with it. So we really need to supply "all or nothing".

The same thing is true of temperature. If we just want to know that we're over temp we might as well just pass through the status bits, which tell you that the system thinks the battery has a high/low temperature. I guess passing through the actual temperature is useful if we want someone to be able to do something to control it - i.e. know it is not just faulting, but is really about to catch fire.

So I think I need more direction about how you see the users and exactly what fields you would be happy with.

One option might be just a little more - status bits and temperature.

If we're not providing all the extra info then we probably also don't need https://mavlink.io/en/messages/common.html#SMART_BATTERY_INFO, except perhaps for "function".

I would be ok with just adding the failure/status flags. I wouldn't necessarily add everything until someone needs/wants it.

Hi,
Sounds like a nice improvement, I was thinking about an extended parameter set too.

What I would like to have, as an end user, is how much capacity of my battery I have used.

Also, current is another one. Maybe not as end user parameter but as power consumption, since it's a hint how healthy a setup is.

cheers

OK,

Current consumed, and total current is something that needs to be kept internal to a smart battery, in my opinion, because otherwise it will - more often than not - be wrong.

A smart battery can report consumption (since full) and remaining battery capacity reliably -they will not be wrong, even after many power cycles and for aged batteries.
Power monitors only measure consumption since power on, and estimate power remaining based on assumption of a full battery at power on.

We are going to allow a consumer to determine which case they have using a new flag. If the flag is set (by a smart battery) the values are assumed to be accurate and consumed is "relative to a full battery". If not set (default, or by a power module) only the current consumed can be trusted.

So there is the option to supply this information only if it is known to be accurate, or to supply it and state that it might not be accurate.

Just FYI!

When you have a smart battery you don't need consumption since full and capacity, that's why a smart battery gives you percentage. So I really don't like the approach of using different values based on different settings, hence I suggest to have percentage (as estimated by PX4), voltage, current for dumb batteries, and the failure flags for smart batteries.

I agree with only exposing the current. I am not sure how QGC handles the 'current consumed' but that's not something for an FC to keep track with. Current is a raw value and works well. Until now I was peeking the mavlink message itself to calculate the power consumption and I find it very valuable.

I am looking forward to a proper smart battery. I've once purchased a BatMon but never got into it seriously. Are their changes in the PX4?

The battery message updates are "in discussion". There are no changes to PX4 yet, but this is part of what might end up happening.

OK, proto proposed in mavlink/MAVSDK-Proto#290 - don't think it can merge until things settled with the message either.

src/mavsdk/plugins/telemetry/telemetry_impl.cpp

hamishwillee added 2 commits May 26, 2022 17:22

TEMP GIT_TAG update to get mavlink battery status v2 branch

773e9fc

battery_status_v2 message handling

2e5344b

hamishwillee commented May 26, 2022

View reviewed changes

JonasVautherin reviewed May 27, 2022

View reviewed changes

src/mavsdk/plugins/telemetry/telemetry_impl.cpp Show resolved Hide resolved

hamishwillee commented May 29, 2022

View reviewed changes

src/mavsdk/plugins/telemetry/telemetry_impl.cpp Outdated Show resolved Hide resolved

hamishwillee added 3 commits June 2, 2022 18:07

Update the CMakeList with update changelist

e80d2e1

Update src/mavsdk/plugins/telemetry/telemetry_impl.cpp

8bc6b6e

Fix up remaining_percent if empty

731b9d5

hamishwillee mentioned this pull request Jun 2, 2022

Battery message - extend with status, current, and fix percentage mavlink/MAVSDK-Proto#290

Draft

julianoes changed the title ~~PR for BATTERY_STATUS_V2~~ New BATTERY_STATUS_V2 Sep 9, 2022

julianoes added the feature label Sep 9, 2022

Katawann mentioned this pull request Sep 14, 2022

PR Add battery proto status message for v1 mavlink/MAVSDK-Proto#297

Merged

julianoes mentioned this pull request Dec 4, 2023

RFC 0018 - Improved Battery Status Reporting mavlink/rfcs#19

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New BATTERY_STATUS_V2 #1792

New BATTERY_STATUS_V2 #1792

hamishwillee commented May 26, 2022 •

edited

Loading

hamishwillee May 26, 2022

julianoes May 26, 2022

hamishwillee May 29, 2022

hamishwillee May 26, 2022

julianoes May 26, 2022

hamishwillee May 29, 2022

JonasVautherin May 29, 2022

julianoes Jun 1, 2022

julianoes Jun 1, 2022

hamishwillee Jun 1, 2022

JonasVautherin Jun 1, 2022

hamishwillee Jun 2, 2022

hamishwillee May 26, 2022 •

edited

Loading

julianoes May 26, 2022

hamishwillee May 29, 2022

julianoes May 30, 2022

reedev May 31, 2022

hamishwillee Jun 2, 2022

julianoes Jun 2, 2022 •

edited

Loading

reedev Jun 2, 2022

hamishwillee Jun 2, 2022

hamishwillee Jun 2, 2022

New BATTERY_STATUS_V2 #1792

Are you sure you want to change the base?

New BATTERY_STATUS_V2 #1792

Conversation

hamishwillee commented May 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamishwillee May 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianoes Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamishwillee commented May 26, 2022 •

edited

Loading

hamishwillee May 26, 2022 •

edited

Loading

julianoes Jun 2, 2022 •

edited

Loading