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

RFC 0018 - Improved Battery Status Reporting #19

Open
wants to merge 56 commits into
base: master
Choose a base branch
from

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Mar 24, 2022

The BATTERY_STATUS has two arrays that can be used to report individual cell voltages (up to 14), or a single cumulative voltage, and which cannot be truncated.
The vast majority of consumers are only interested in the total battery voltage, and either sum the battery cells or get the cumulative voltage.

This RFC proposes two new messages, separating out the individual voltage for cells as cell fault information. The result is a simpler message to understand and use. We've also liased with UAVCAN in order to ensure that the messages are compatible with what UAVCAN is doing.

Copy link

@dakejahl dakejahl left a comment

Choose a reason for hiding this comment

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

Thank you for putting this together @hamishwillee

text/001x-battery_improvements.md Outdated Show resolved Hide resolved
text/001x-battery_improvements.md Outdated Show resolved Hide resolved
text/001x-battery_improvements.md Outdated Show resolved Hide resolved
text/001x-battery_improvements.md Outdated Show resolved Hide resolved
text/001x-battery_improvements.md Outdated Show resolved Hide resolved
text/001x-battery_improvements.md Outdated Show resolved Hide resolved
text/001x-battery_improvements.md Outdated Show resolved Hide resolved
@tridge
Copy link
Member

tridge commented Oct 7, 2022

@hamishwillee I was thinking about fuel flow, and it depends how we map the units. An idle engine may have very low fuel flow.
The same could be a concern for very low currents while a vehicle is sitting idle for a long time. For example copters that are on standby for a very long time (days) with a primary avionics battery and a flight battery. The flight battery will discharge very slowly as the power monitor itself draws current. We're getting more cases where people put vehicles in standby for days or weeks waiting for the command to fly.
If we made it float amps for current and used litres/second for fuel then we still get to support very large vehicles while flying and also support very small numbers when vehicles are sitting idle for a long time.
we do lose precision for very large numbers, but I suspect supporting very small numbers is more important

@hamishwillee
Copy link
Collaborator Author

@tridge I'm not sure the argument is compelling unless you are expecting the GCS to integrate the received current? If that's a use case, this makes sense.

Otherwise the power monitor can integrate whatever current it likes - if the vehicle happens to be sitting unused for 2 weeks and the monitor is calculating the usage accurately you'll still get the right capacity_consumed/capacity remaining.

The data size is the same so over the wire data doesn't matter. You could argue it is easier to calculate/send. I'm fine with it if @hendjoshsr71 @dakejahl all think it is worth doing. All, please confirm, or offer counter arguments.

Note the only change would be

  • BATTERY_STATUS_V2.current - currently mA in int32_t TO A in float
  • In the text we would add that if MAV_BATTERY_TYPE is a liquid fuel then the value is L.
  • no change to the consumed/capacity remaining fields

@tridge
Copy link
Member

tridge commented Oct 12, 2022

one thing we should seriously consider is adding float16 as a base type in mavlink. If we'd done the original mavlink battery msgs as float16 volts and amps then we probably wouldn't have been discussing these new msgs at all.

@tridge
Copy link
Member

tridge commented Oct 12, 2022

having this:

  • float total_voltage
  • uint16 lowest_cell_mV
  • uint16 highest_cell_mV

that costs 8 bytes and gives the GCS what it really needs to sound an alarm about a bad cell
adding uint8 higest_cell_volt_idx and lowest_cell_volt_idx would cost 2 more bytes. Is that useful?

Note that smart batteries should set the MAV_BATTERY_STATUS_FLAGS_CAPACITY_RELATIVE_TO_FULL bit to indicate that supplied capacity values are relative to a battery that is known to be full.
Power monitors would not set this bit, indicating that capacity_consumed is relative to drone power-on, and that other values are estimated based on the assumption that the battery was full on power-on.
</description>
<field type="uint8_t" name="id" instance="true">Battery ID</field>

Choose a reason for hiding this comment

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

Suggested change
<field type="uint8_t" name="id" instance="true">Battery ID</field>
<field type="uint16_t" name="id" instance="true">Battery ID</field>

Is there a need to support more than 255 batteries? @AlexKlimaj thoughts? Would you ever go bigger than 255 in your box?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hope not. That feels like excessive future proofing.

Choose a reason for hiding this comment

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

I mean .. maybe ... is it? What if someone wanted to replace autopilot with ardupilot? 🤨
https://www.google.com/search?q=how+many+batteries+in+a+tesla

see what I did there I rhymed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dakejahl And you wonder why nobody likes you :-)

Choose a reason for hiding this comment

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

haha ouch but you're wrong I am beloved. But no in DroneCAN I think it can make sense, probably not mavlink

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah yeah :-). I'm going to say no "right now". Yes this might come back and bite us, but it feels like a waste.

Choose a reason for hiding this comment

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

I've thought about that one before too.....
But i guess one could create their own message at that point. :)

For mavlink no reason to waste space.

@dakejahl
Copy link

dakejahl commented Oct 12, 2022

uint16 lowest_cell_mV
uint16 highest_cell_mV
that costs 8 bytes and gives the GCS what it really needs to sound an alarm about a bad cell
adding uint8 higest_cell_volt_idx and lowest_cell_volt_idx would cost 2 more bytes. Is that useful?

yeah this actually makes a lot of sense because the SMART_BATTERY_INFO message doesn't indicate anything about cells being bad. I don't think streaming the cell voltages all the time is a good idea either and the GCS shouldn't be forced to do that. You guys know more about message design than I do, is 10 more bytes breaking the bank? We're at 217 right now with v2 frame overhead. So that's a 4.6% increase in size... seems worth it to me

edit: there are the status flags for sounding an alarm in general, but knowing if a cell is going bad before it actually does could be useful

@hamishwillee
Copy link
Collaborator Author

@dakejahl SO current proposal is that this is in its own message. Its a low rate message so ideally not on Battery_status_v2 message. It could go on the SMART_BATTERY_INFO maybe, but I'd prefer not to.

@dakejahl
Copy link

dakejahl commented Oct 12, 2022

oh wait I also think I counted wrong we're only at 49 bytes

@dakejahl SO current proposal is that this is in its own message. Its a low rate message so ideally not on Battery_status_v2 message. It could go on the SMART_BATTERY_INFO maybe, but I'd prefer not to.

Frame overhead is 25 bytes, for a 10 byte message? Does it make more sense to stuff it somewhere else where it's already getting polled or low rate streamed? Is the overhead less of a worry with low rate messages?

@tridge
Copy link
Member

tridge commented Oct 12, 2022

for the max/min cell proposal, we also need cell count so we can calculate avg to get deviation from avg for the warning trigger

@hamishwillee
Copy link
Collaborator Author

@tridge @hendjoshsr71 @dakejahl @auturgy

  1. BATTERY_STATUS_V2 seems to have stabilized. I've copied into into development.xml in BATTERY_STATUS_V2 - add to development.xml mavlink#1846 so that people can start to prototype more easily, if they so wish. Can you take one last look at that before I press the merge button? (depends on schema - Add Ampere-hours units ArduPilot/pymavlink#739)
  2. The above does not include the battery voltages. I'd rather @WickedShell or @tridge put that into this doc as a "Suggestion" so we are not playing Chinese whispers.

@hamishwillee
Copy link
Collaborator Author

@tridge @dakejahl @hendjoshsr71 I have merged the BATTERY_STATUS_V2 into development.xml, unchanged since my last comment above #19 (comment). A couple of issues came up that we decided did not warrant changes - see at bottom

Let's get some testing. Is anyone "in particular" committing to implementing these, and in what timeframes? I ask because we're trying to make sure that things don't live in development.xml forever. if no one else, I will start the process of doing this for PX4.

@WickedShell @tridge - the voltage cell message has not gone in, as that is still waiting on a precise definition from "someone".


The changes discussed for BATTERY_STATUS_V2 that did not change anything:

  1. Possible removal of capacity_remaining:
    • The argument was that this is always a synthesize value from full battery capacity and capacity consumed, so this is redundant/wasted bytes. However if you don't have a smart battery then SMART_BATTERY_INFO is optional, and all the recipient will get is this message. Therefore this is the only way that an emitter can effectively give the GCS an estimate of full battery capacity, and that is a useful thing.
    • Also it is minimal churn on existing consumers of the BATTERY_STATUS.
  2. Supporting the concept of battery slots.
    • There was no "key stakeholder" on this and no clarity on requriements or clear solutions. But in discussion it seems likely that this could be added as a field to SMART_BATTERY_INFO later if needed.
    • In the dev call we agreed that if you want slots then you're committing to sending SMART_BATTERY_INFO (whether or not your battery is smart).

@julianoes
Copy link

Is this still relevant? I'm just looking at mavlink/MAVSDK#1792...

<entry value="262144" name="MAV_BATTERY_STATUS_FLAGS_CAPACITY_RELATIVE_TO_FULL">
<description>
Battery capacity_consumed and capacity_remaining values are relative to a full battery (they sum to the total capacity of the battery).
This flag would be set for a smart battery that can accurately determine its remaining charge across vehicle reboots and discharge/recharge cycles.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
This flag would be set for a smart battery that can accurately determine its remaining charge across vehicle reboots and discharge/recharge cycles.
This flag would be set for a battery that can accurately determine its remaining charge across vehicle reboots and discharge/recharge cycles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants