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.

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

New BATTERY_STATUS_V2 #1792

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
41 changes: 41 additions & 0 deletions src/mavsdk/plugins/telemetry/telemetry_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,11 @@ void TelemetryImpl::init()
[this](const mavlink_message_t& message) { process_battery_status(message); },
this);

_parent->register_mavlink_message_handler(
MAVLINK_MSG_ID_BATTERY_STATUS_V2,
[this](const mavlink_message_t& message) { process_battery_status_v2(message); },
this);

_parent->register_mavlink_message_handler(
MAVLINK_MSG_ID_HEARTBEAT,
[this](const mavlink_message_t& message) { process_heartbeat(message); },
Expand Down Expand Up @@ -1176,6 +1181,42 @@ void TelemetryImpl::process_battery_status(const mavlink_message_t& message)
}
}

void TelemetryImpl::process_battery_status_v2(const mavlink_message_t& message)
{
mavlink_battery_status_v2_t bat_status;
mavlink_msg_battery_status_v2_decode(&message, &bat_status);

_has_bat_status = true;

Telemetry::Battery new_battery;
new_battery.id = bat_status.id;
new_battery.voltage_v = bat_status.voltage;
hamishwillee marked this conversation as resolved.
Show resolved Hide resolved
//Is it correct to set this to NaN for UINT32_MAX? HOw do you specify = NaN?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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)
  1. 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).
      

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely!

new_battery.remaining_percent = bat_status.percent_remaining; // to test * 1e-2f;

//bat_status also has:
Copy link
Collaborator Author

@hamishwillee hamishwillee May 26, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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 be ok with just adding the failure/status flags. I wouldn't necessarily add everything until someone needs/wants it.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator

@julianoes julianoes Jun 2, 2022

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

//uint32_t current; /*< [mA] Battery current (through all cells/loads). UINT32_MAX: field not provided.*/
// uint32_t current_consumed; /*< [mAh] Consumed charge (since vehicle powered on). UINT32_MAX: field not provided. Note: For power modules the expectation is that batteries are fully charged before turning on the vehicle.*/
// uint32_t current_remaining; /*< [mAh] Remaining charge (until empty). UINT32_MAX: field not provided. Note: Power monitors should not set this value.*/
// uint32_t status_flags; /*< Fault, health, and readiness status indications.*/
// int16_t temperature; /*< [cdegC] Temperature of the whole battery pack (not internal electronics). INT16_MAX field not provided.*/


set_battery(new_battery);
JonasVautherin marked this conversation as resolved.
Show resolved Hide resolved

{
std::lock_guard<std::mutex> lock(_subscription_mutex);
if (_battery_subscription) {
auto callback = _battery_subscription;
auto arg = battery();
_parent->call_user_callback([callback, arg]() { callback(arg); });
}
}
}

void TelemetryImpl::process_heartbeat(const mavlink_message_t& message)
{
if (message.compid != MAV_COMP_ID_AUTOPILOT1) {
Expand Down
1 change: 1 addition & 0 deletions src/mavsdk/plugins/telemetry/telemetry_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ class TelemetryImpl : public PluginImplBase {
void process_fixedwing_metrics(const mavlink_message_t& message);
void process_sys_status(const mavlink_message_t& message);
void process_battery_status(const mavlink_message_t& message);
void process_battery_status_v2(const mavlink_message_t& message);
void process_heartbeat(const mavlink_message_t& message);
void process_rc_channels(const mavlink_message_t& message);
void process_unix_epoch_time(const mavlink_message_t& message);
Expand Down
4 changes: 3 additions & 1 deletion third_party/mavlink/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ find_package(Python3 COMPONENTS Interpreter REQUIRED)
ExternalProject_add(
mavlink
GIT_REPOSITORY https://github.com/mavlink/mavlink
GIT_TAG 3b52eac09c2e37325e4bc49cd2667ea37bf1d7d2
GIT_TAG a5556816af29e348875cbcc26a1d36b42dc5e921
# This GIT_TAG will need to update as it is for a temporary branch containing the new battery
# https://github.com/mavlink/mavlink/commit/a5556816af29e348875cbcc26a1d36b42dc5e921
PREFIX mavlink
CONFIGURE_COMMAND Python3::Interpreter
-m pymavlink.tools.mavgen
Expand Down