fix(development): BATTERY_STATUS_V2 proposed changes#2471
fix(development): BATTERY_STATUS_V2 proposed changes#2471hamishwillee wants to merge 4 commits intomasterfrom
Conversation
| <entry value="524288" name="MAV_BATTERY_STATUS_FLAGS_CHARGE_STATE_LOW"> | ||
| <description>Battery state is low, warn and monitor close</description> | ||
| </entry> | ||
| <entry value="1048576" name="MAV_BATTERY_STATUS_FLAGS_CHARGE_STATE_CRITICAL"> | ||
| <description>Battery state is critical, return / abort immediately</description> | ||
| </entry> | ||
| <entry value="2097152" name="MAV_BATTERY_STATUS_FLAGS_CHARGE_STATE_EMERGENCY"> | ||
| <description>Battery state is too low for ordinary abortion, fastest possible emergency stop preventing damage</description> | ||
| </entry> |
There was a problem hiding this comment.
This is proposed addition of charging state flags for comment.
From @MaEtUgR in https://github.com/mavlink/mavlink/pull/1846/changes#r3164851127
This new message does not contain information about the low, critical, emegency state of depletion of the battery based on the use case. That means the UI has no way to show that and would need to infer based on additional information like non-standardized internal state of charge thresholds. So it's missing this functionality: https://github.com/mavlink/mavlink/pull/875/changes
@MaEtUgR The removal of those was intentional from my inference of the RFC
The state is almost all inferred from the battery remaining. Traditionally the GCS sets appropriate levels for warning - the battery has a less accurate view on appropriate warning levels for a particular system, making this redundant. The exception is charging status.
In other words, the battery doesn't know whether it is critical or not for the vehicle - there might be several batteries and whether it is a problem depends on how they are wired. So with a MAVLink battery MAV_BATTERY_CHARGE_STATE_CRITICAL couldn't be set by the battery alone - the GCS would have to set that level right?
I could be wrong. Perhaps a MAVLink battery would not set these, but if emitted by a flight stack, the flight stack would provide the information?
Thoughts?
|
@hendjoshsr71 I was wondering if you would like to comment on the proposed changes to BATTERY_STATUS_V2. I am inclined to accept them - in particular state-of-charge instead of battery remaining as it does appear more industry standard. |
| @@ -516,7 +525,7 @@ | |||
| <field type="float" name="current" units="A" invalid="NaN">Battery current (through all cells/loads). Positive value when discharging and negative if charging. NaN: field not provided.</field> | |||
| <field type="float" name="capacity_consumed" units="Ah" invalid="NaN">Consumed charge. NaN: field not provided. This is either the consumption since power-on or since the battery was full, depending on the value of MAV_BATTERY_STATUS_FLAGS_CAPACITY_RELATIVE_TO_FULL.</field> | |||
There was a problem hiding this comment.
Note: I should propose SI unit for energy Wh such that we can disucss that as well.
There was a problem hiding this comment.
IMO common convention > SI units. Every battery is labelled with Ah.
There was a problem hiding this comment.
The use of SI vs common convention has been an argument that has gone back and forth over the years. I prefer common conventions in most cases because as presentation values they are are more intuitive to users, and thereby avoid a conversion. Similarly, I prefer SI values if to use them you will likely have a conversion to the SI value anyway.
I have a slight leaning towards Wh as a measure as it is independent of voltage and battery chemistry.
I'm not sure it matters though - for display user only care about the % left or the available flight time. If you're using a particular vehicle it is likely the same battery will be used all the time, so if you were to look at the value you'd become familar with Ah or Wh that was shown to you.
If there is a technical reason why it is better as Wh in real scenarios then we should switch.
There was a problem hiding this comment.
optional:
In case you want opinions from the crowd, I like this as-is (capacity_consumed, Ah, Charge consumed). I'd even prefer in the description "time-integral of measured current" but that might be too mathy for average users. As-written, I concluded that's what's meant. (In case that conclusion is wrong, that might prompt further consideration.)
IMO if you switch to Wh, should also rename like "energy_consumed". And description 'Consumed charge' isn't quite right, but I would be ok with it as a known abuse of terminology.
By "optional" I mean that I don't expect response nor changes. Use or ignore my input as best suits you! And keep up the great work.
There was a problem hiding this comment.
@hendjoshsr71 is ArduPilot go-to-guy for battery commentry. So if he agrees it is likely that changes will be accepted by the broader ArduPilot community. @MaEtUgR serves the same purpose in PX4 land. So I need their opinions as a proxy for their broader community.
That said, opinions from the crows are very very welcome. Note though that this message has already been through many rounds of review, so I'm more likely to accept changes that don't change anything too significant :-)
To propose a specific change to a field in any way, please add a github suggestion to the line, preceded by your reasoning. You almost certainly know better than me about the terminology
- and yes, I prefer industry standard terminology.
- It is the time integral of measured current as I understand it.
- We can be fairly technical in the description, but I prefer that the initial description uses simple wording "consumed/remaining/available charge/enery/etc."
There was a problem hiding this comment.
(Ok with me to resolve anytime.)
There was a problem hiding this comment.
Cool. Let's see what Josh has to say.
| <field type="float" name="capacity_consumed" units="Ah" invalid="NaN">Consumed charge. NaN: field not provided. This is either the consumption since power-on or since the battery was full, depending on the value of MAV_BATTERY_STATUS_FLAGS_CAPACITY_RELATIVE_TO_FULL.</field> | ||
| <field type="float" name="capacity_remaining" units="Ah" invalid="NaN">Remaining charge (until empty). NaN: field not provided. Note: If MAV_BATTERY_STATUS_FLAGS_CAPACITY_RELATIVE_TO_FULL is unset, this value is based on the assumption the battery was full when the system was powered.</field> | ||
| <field type="uint8_t" name="percent_remaining" units="%" invalid="UINT8_MAX">Remaining battery energy. Values: [0-100], UINT8_MAX: field not provided.</field> | ||
| <field type="float" name="state_of_charge" units="%" invalid="NaN">State of Charge (SoC). Remaining capacity relative to current maximum capacity as a percentage (designed max capacity can be used if current max capacity is not known). Values: [0-100], NaN: field not provided.</field> |
There was a problem hiding this comment.
Note, float for consistency, and considered useful for very large batteries.
4 participants
This proposes updates for discussion in BATTERY_STATUS_V2 following review of first implementation by @MaEtUgR in PX4/PX4-Autopilot#25347 (review)
See comments inline for discussion start points. There are no live implementations so we can still udpate if we want.
@peterbarker @auturgy Can you suggest who in ArduPilot might like to comment. FWIW I tend to think if SoC is a standard term we should use it. Rest need more discussion.