Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
New BATTERY_STATUS_V2 #1792
Changes from 2 commits
773e9fc
2e5344b
e80d2e1
8bc6b6e
731b9d5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
Does that mean we need to check for a UINT32_MAX and set to NaN? Hints?
There was a problem hiding this comment.
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 😂.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
=
? In this case the value is 2. My guess would be the default initialisation, but then we afterwards statemavsdk.options.default_value
=NaN. That would imply to me that the default is NaN?There was a problem hiding this comment.
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, likevelocity_m_s
andtime_s
. I don't have a strong opinion here so if you insist, we can rename it 👍.=
is for protobuf. In protobuf, each field has an "id", which is used in the binary representation. Something like themsgid
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 likeNaN
makes more sense for an undefined float 🤔.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 itcharge_percent
if you prefer?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK,
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.