Better Battery Messages #7
Conversation
|
|
It would be useful to align DroneCAN and MAVLink messages, as they need to partially map (DroneCAN is obviously internal, MAVLink external, but where information is common the field names and data types should match in my view). |
|
Good start overall and nice initiative Still I've got some feedback you might want to consider in the smart battery messages. Status flags
For the SmartBatteryPeriodic messages, I think it might worthwhile to explore to expose more information of the battery itself, things that came to my mind would be
|
|
|
Yes. See https://mavlink.io/en/messages/common.html#SMART_BATTERY_INFO and mavlink/rfcs#19 (in discussion) |
|
Agreed about additional flags for bad battery, under temp, and manufacture date. I disagree on weight and battery technology though. Both these fields are static and can be derived from the |
dakejahl
force-pushed
the
pr-smart_battery
branch
from
a390492
to
be885a4
Compare
|
Alright I made all data types and name match mavlink with the exception of status_flags, see mavlink rfc for battery_status_v2 I also moved |
|
I am loving this rework on the smart battery message info. If I can add my thoughts on the matter. Firstly, I agree 100% transmitting static information at a rate equal to or grater than 1Hz is useless. If it isn't used in the normal operation of a vehicle then why send it at 1Hz? I am not saying don't send it, just don't flood the bus with info that is hardly ever used. I think Manufacturing date should be separated out into more generic information that other nodes use. Secondly, I think we need to look at the necessity/criticality of data that is being sent. How often is it sampled, how often is it used/needed. If there isn't sound justification for high rate transmission then I think it should go in the 1Hz periodic message. In my mind, this is things like time_remaining, battery_remaining and maybe temperature. Lets look something like the BQ40Zxx. This BMS chip "perform voltage, current, and temperature readings every 250 ms" but will only "performs I am happy to add to or generate another PR to show my point of view |
100% agree. Some kind of request/response mechanism would be much better suited for this static information. I have not used "Services" before but perhaps we need to introduce a metadata file for CAN Nodes. This would be a one time transaction initiated by the FC and the file could be a JSON description of the value name, data type, and value. This would eliminate the Periodic message entirely in its current form. Another option would be to have a Service request a message be sent once. This would effectively convert the Periodic message into a Static message and the FC would have to request it. I think the transmission rate of data is less important, I picked 10Hz and 1Hz arbitrarily. But killing off the periodic message in favor of a metadata file and keeping the "remaining" fields in the Continuous message might be the best optimization. Another benefit of a JSON file would be extensibility without breaking the wire format. Thoughts? |
There was a problem hiding this comment.
Where possible we should have flags that tells systems whether the battery pack can provide the specified information or not. (MAVLink has these for most items. I'd use those for the most part.)
There are going to be senders of these messages that range from providing all of the info. to just cell voltages, to a mix in between. They already exist and use the existing messages.
My personal thought is that cell voltages should get their own message. (Hopefully, those could be turned on/off as parameters....) Some systems will not want this info. flooding their CAN bus. Some systems may be happy with whatever info. and flags are provided by TI ICs.
But others will want it for their own predictive health algorithms. Or EKFs.
You should take a look again at the original and copy more from the current implementations. Saving bytes, adding/deleting fields, as needed of course. But the comments, sizes, fields you didn't include, etc. in those are pretty decent.
| # Smart battery data to be sent continuously (10Hz or greater) | ||
| # | ||
|
|
||
| int16 temperature # [cdegC] |
There was a problem hiding this comment.
Should there be additional temperature fields in another message? The TI chip you mentioned has 4 inputs and there are many current products with both internal and external thermistors.
| int16 temperature # [cdegC] | |
| int16 temperature # [cdegC], not available INT16_MAX |
There was a problem hiding this comment.
IMO the only number that we really care about is the external pack-mounted thermistor
There was a problem hiding this comment.
at minimum we need to document which should be preferred here (internal, external, maximum of all sensors?). I'm no battery expert. But my uninformed self would think internal temperature could be more important since that would get hotter faster and getter colder slower?
There was a problem hiding this comment.
Sorry by internal I am referring to the temperature sensor on the ASIC and by external I am referring to the pack mounted thermistor. The pack mounted thermistor measures the temperature of the cells (sometimes it's installed between cells) and is definitely going to heat up the most/fastest. The ASIC might not be positioned as closely to the cells themselves so the temp reading from it is less important. All we really want to know is "how hot is my battery?"
There was a problem hiding this comment.
Ahh ok. To me external means mounted on outside and internal mounted in between cells. I would likely call your internal temperature: PCB temperature or chip temperature
There was a problem hiding this comment.
If you could accurately measure shunt/FET temps that would be useful too, but how are you going to do that? Mount a thermistor right next to the shunt resistor? Thermal paste them to the top of the FET package(s) or on top of the shunt itself?
Yes. This is what we do
I honestly don't know... and there are problems with both of those proposed methods. At the end of the day it's only a few bytes so it's not that big of a deal, but I think for most cases it's going to be an unused field.
But if we're going to do it we should do something like
float16 temperature_pack // Battery pack temperature float16 max_temperature // Maximum thermistor temperature on BMS float16 min_temperature // Minimum thermistor temperature on BMS
Wouldn't this make sense?
float16 max_temperature_board // Max temperature on BMS FET/shunt/board
float16 max_pack_temperature // Maximum Battery pack temperature
float16 min_pack_temperature // Minimum Battery pack temperature
There was a problem hiding this comment.
@Sypaq-MadMan Sorry, I'm not trying to be pedantic just to be clear. It is the problem with creating definitions :)
Your definition of letting the BMS do all the decisions is a narrow use case for these messages.
👍 I am definitely coming at this from a very specific use case and I probably don't fully understand or know the rest of the cases.
I'm not sure your meaning of average temperature? How would the average be defined by the battery system?
Most of my example revolve around using a BQ Series BMS. The BMS has a config parameter that can tell the battery what instantaneous temperature to spit out over SMBus, I.e Min/Max/Average over X + 1 thermistors. Where one is in the chip itself (suppose to reflect FET/PCB Temp if PCB is designed correctly) and the rest in-between the battery cells (I.e. External, but internal when holding the battery as a whole).
Now stepping away from my case, I do agree, having multiple temperatures does present more value. How much beyond what echoGee suggested? I don't know.
If you wanted more them then maybe just:
float16 max_temperature_board // Max temperature on BMS FET/Shunt/Board
float16 pack_temperature [<=10] // Battery pack temperature(s)
There was a problem hiding this comment.
Ahh! This was the key part I didn't understand: "Min/Max/Average over X + 1 thermistors" you're comments make a lot of sense now.
I do think it is better to enumerate the definition of thermistors ie first is PCB, second external, third internal.
In this way we don't need special parsing for each in the autopilot for this device or that device?
Because which thermistor is measuring the min. or max., could be changing?
There was a problem hiding this comment.
Is this resolved? Are we happy with the 3 temperatures?
There was a problem hiding this comment.
Considering this message is pretty small (30 bytes) we could add all of the temperatures we could possibly care about 🤷
float16 temperature_cells
float16 temperature_shunt
float16 temperature_fet
float16 temperature_max
float16 temperature_min
I think this would make sense
I think having a mechanism to request messages, whether that be a single shot or interval, would be super useful.
Which fields would you like to see? And could you explain the usefulness? I reviewed all of the existing messages and omitted many of the fields because I thought they were redundant or do not provide much value. |
|
@dakejahl Just as an FYI:
Cell voltages are the reason we are breaking BATTERY_STATUS we keep on getting batteries with more cells and we can't extend the message.
I'll update the MAVLink RFC with some feedback from developers later today. Again, we might not take it. Message design is a compromise. |
I agree this could be an issue. Perhaps we can have GCSs request this as well have FCs send at a very low rate. FCs could also implement a mechanism to trigger sending the message when a battery has changed, but architecturally this doesn't exist and would be more difficult to implement than simply low rate streaming (0.2Hz or something) 100% agree about selectively enabling cell voltages, I think they belong in their own message. The question is what is the cutoff for maximum number of cells? An inefficient way would be to send each cell voltage/fault_flag as its own message. As far as logging cell voltages, we could still do this within each FC log file. |
I think this is a good suggestion although does require nodes to implement parameters. It is more flexible and extensible which would allow implementations to provide only the data they deem necessary. A downside of this would be a lack of a standard for parameter names... so whoever does it first and provides handling in one or more flight stacks would effectively lock the spec, unless we continued to extend the handlers for new/different param names... double edged sword me thinks. Thoughts? |
|
In mavlink we are proposing a 12-cell message with battery id and index. If you have 24 cells, you send it twice. You might argue inefficient, but the fact is no one needs this message except for debugging - it can be sent at very low rate. I don't know how UAVCAN "treats" parameters but in MAVLink they are essentially component specific - i.e. MAVLink can set them and expose them to users, but it knows nothing about them. So if you say something is a parameter you are saying that it is not standardized and you can't rely on it being something you use unless you know the system. That makes sense for a lot of stuff, but I don't imagine battery properties fit into that category. |
dakejahl
force-pushed
the
pr-smart_battery
branch
from
554a447
to
1b33058
Compare
|
Here is my branch built from your current branch, https://github.com/hendjoshsr71/DSDL/tree/pr/battery_messages I would love to compare anything we do with a comparable messages used in auto EVs or commercial aircraft? But I;m not well versed on the standards there. I created a Cell1 message for just voltages and resistances for those who would want to monitor health fully. Presuming the rate of the message would default off. The biggest changes is pulling in more comments and fields to define the meaning. We had a case of a vendor defining charge current opposite normal convention once. Assuming we can figure out a way to do a "static" message type we should be pulling in more info. to giving others systems a fuller picture of the battery from a charging and discharge perspective. IE slow down you cant pull that much current. And the system slows itself down (ArduPilot can do this already). One area I think we see this differently is the message is labeled "smart_battery". But really do we need other battery messages for batteries or systems with varying measurement and estimation capabilities? IE some use the current message on a can node to convert analog voltage and current measurements to CAN to the autopilot. These have zero estimation capabilities and limited measurements. Most TI ICs would have middle of the road estimation capabilities. And above that would be those that go to the full modeling effort and create an EKF for SOC and SOH. |
Great idea.
Yeah I am not opposed to putting all kinds of info in the static/periodic message. If we can create a Message Request Service that would be ideal, and then we just request the message when we want it.
Yeah you're right, both of these messages are generic enough to be used for all batteries. The important piece is just the separation of high and low rate data. Let's rename to be more generic
Yeah I like the addition of the two capabilities flags. |
|
Suggestions for the new message names? SmartBatteryContinuous --> BatteryStatus Or is having "static" or "continuous" in the name helpful to understand the usage? And perhaps we add an |
The new proposed names are much better.
Yes. In MAVLink we used an index for the "SmartBatteryCells1" equivalent message for that reason. There are already batteries with far more than 24 cells. We used just 12 cells on the assumption that it is a soft spot where the vast majority of batteries will have less than that. We were originally 10 but noted that battery cells seem to increase in multiples of 6. |
|
Yup. Adding an index makes a lot of sense to simplify things. Thanks! Does that mean we would might want to add the battery id to make life easier for log analysis? I know it could be derived from battery_info and the NodeID. I like the new names. Maybe "BatteryCell" or something more generic "BatteryCellStatus" |
| # Smart battery data to be sent continuously (1 - 10Hz) | ||
| # | ||
|
|
||
| float16 temperature # [C] : Pack mounted thermistor (preferably installed between cells), NAN: field not provided |
There was a problem hiding this comment.
Kelvin is less useful. Most people want to know temperature in C or F
There was a problem hiding this comment.
FWIW this kind of argument comes up all the time in MAVLink: personally I prefer human units, while MAVLink "officially" leans towards SI units. The message space is littered with fields designed using both approaches.
The learning from this? It doesn't matter so much which unit you use for a particular message. What does matter is that the units you use are as consistent as possible across all your messages. That allows message design and interpretation to follow familiar patterns for implementers.
I wish we'd thought of that early on :-(
| float16 temperature # [C] : Pack mounted thermistor (preferably installed between cells), NAN: field not provided | ||
| float32 current # [Ampere] : Postive: defined as a discharge current. Negitive: deifined as a charging current, NAN: field not provided | ||
| float16 voltage # [Volt] | ||
| uint32 capacity_consumed # [mAh] |
There was a problem hiding this comment.
I am not sure about this one, isn't capacity_consumed = capacity_remining - full_charge_capacity?
Above rings true if its a fully charged pack, but wont for anything else. This is going to cause issue as now if I go and fly a 85% charged pack, who is calculating capacity_consumed? The micro?
Also, cant capacity_consumed just be calculated from capacity_remaining at time of armed minus current capacity_remaining?
There was a problem hiding this comment.
yeah I could go either way on it. Figured it might be useful for non-smart batteries (only coulomb counting for example) where they may not actually know the remaining capacity but do know how much has been consumed.
There was a problem hiding this comment.
Yes. For MAVLink at least we need both.
To be very precise, a power monitor can really only measure the consumed current. Systems that use this always assume full battery to start with and calculate remaining capacity from consumed current. This assumption is flawed because the power monitor resets on every reboot, and might actually not be full (as you say).
So we need this for power monitors. Probably in MAVLink we'll just highlight this issue with using that kind of hardware. Perhaps recommend only supplying capacity_remaining if that is reliable and consumed current if that is what is reliable.
| float32 current # [Ampere] : Postive: defined as a discharge current. Negitive: deifined as a charging current, NAN: field not provided | ||
| float16 voltage # [Volt] | ||
| uint32 capacity_consumed # [mAh] | ||
| uint32 capacity_remaining # [mAh] |
There was a problem hiding this comment.
I agree W-h is a better standard from a power systems perspective. But in the end most flight controllers currently use mAh all around the code meaning you would still need mAh or convert from W-h using nominal voltage.
There was a problem hiding this comment.
yup I tend to favor units that are more practical, mAh is just what everyone uses
There was a problem hiding this comment.
@dakejahl : Shouldn't this change to WH moving forward though? Many drone makers are going towards higher(12S,14S or higher) packs, and the mAh drawn isn't as intuitive/accurate as when dealing with lower number cells. I feel like the better standards should also nudge users to adopting better units.
You could check out https://rotoye.com/modelling-the-behavior-of-a-battery/ to see how the currents change with high and low SOC for different cell counts
There was a problem hiding this comment.
I am not opposed as long as I understand how we perform the conversion without losing any accuracy. How is delta mAh derived? Is this just mAh_full - mAh_current?
There was a problem hiding this comment.
Could you clarify? I didn't understand which variables are available, and which one you'd want to convert to.
Analog power monitors for eg, have instantaneous current measurements (current). I believe px4 does delta mAh = current*dt
There was a problem hiding this comment.
Wh = mAh * Voltage / 1000
This is not exactly right. To be precise, this should be
delta Wh = delta mAh * instantaneous voltage /1000
Present Wh left in battery = SOC(%) * Total Wh of battery. ButPresent mAh left in battery != SOC(%) * Total mAh of battery
Most fuel gauges that I've looked at only report RemainingCapacity in units of mAh. This PR was intended specifically for smart batteries (which use fuel gauges). So if I can understand how we would convert this mAh value into Wh without losing accuracy then I'm game, otherwise the unit conversion would defeat the purpose of the data...
@AlexKlimaj Can you comment here?
There was a problem hiding this comment.
Even if the Wh isn't calculated by the chip, it maybe close enough to take volt at A and B and average it to calculate Wh from delta mAh .
There was a problem hiding this comment.
I'm not sure how the delta mAh is helpful. Since this is smart battery specific, the fuel gauge IC's are doing coulomb counting based on current. Which I assume is why they measure everything in mAh as truth. I think generally, Watt Hours are calculated based on nominal voltage * Ah. If we want to use Wh, we would need to know the nominal voltage of the pack as well.
We already have the nominal voltage in the Periodic message. In that case, mAh makes more sense. A user's application can take those two values and multiply them for Wh if they want.
In PX4, I believe delta mAh is just a rough running accumulation of the measured current. I've never actually used it as a real measure of the battery's remaining capacity.
BatteryCellVoltages is more precise if all you are sending is voltages. I haven't looked at the message - you might be sending per-cell fault information too. |
I've added in cell resistances since that plus cell voltages equal most of the extended info. someone health monitoring might want for long term monitoring. (This assumes the rates of the message or field is configurable of course....) |
BatteryCellStatus might be a good name then. For MAVLink the way messages work means that, if resistances are useful, we would do these as a separate message. |
|
Yeah in the context of mavlink I might just wait till someone asks for it... I don't think I'd ever put those on the link with the limited space available. Even in a flight test scenario with high speed links I wouldn't add them to the link budget. I forgot about the context of companions too with mavlink. There receiving resistances might present a limited value. |
|
I'm quite keen on getting slot IDs in though I think it might be worth for DroneCAN purposes to discuss how folks could automatically setup slot IDs per battery without setting CAN IDs. Sometime maybe we should discuss a method for that. I've seen your 8 slot solution by reading an analog voltage which is cool for 8 slots. But thinking for things at a bigger scale...... and what a slot manager would look like on the hardware side. Such as using I2C or 1-wire 32-bit unique id chips that a battery could read. It might be talking what the hardware side might look like sometime? The autopilot would then know to correlate that unique slot_ID with the physical slot via a parameter setup in the SLOT_MANAGER. |
|
@dakejahl if you want to add lifetime fields to the battery_info message I'm okay with that as it is low rate. And it could make it easier for manufacturers to check on systems that are being abused beyond warranties :) And we shouldn't force SSHing into an individual battery. I would NOT add them to MAVLink. @hamishwillee In regards to all the arming stuff at this point that would be better left up to the user, vehicle, and autopilot? A slot being empty is very reasonable in some systems. Numbering of slots would be nice to a have a standard. |
|
"edit: is there any value in knowing if it's avionics/power/backup/other? Or is it better to just know that a battery is secondary/primary? Primary meaning it's used for propulsion and should be associated with low battery warnings/failsafes." I think there is value is marking these especially for VTOL aircraft. There are many systems with separate batteries for VTOL vs forward flight vs. avionics/flight surfaces. Or for a copter manufacturer I helped them setup all their vehicles models such that Of course a GCS would need to do something smart with these fields..... |
Although I agree with this design approach it means redefining the "hardware interface standard" by needing to now also route one wire or i2c (instead of boot/slot_id). It also increases cost and complexity because now you have hardware you have to talk to (onewire or i2c eeprom for example). Personally I don't care about the slot ID concept right now, but I definitely see how it is useful for other applications. I also feel like the current specification has too many details relating to the smart battery design implementation (boot pin and ID being analog voltage level defined, outlining the different battery "states" -- hotswap, flightmode, slot_id_sense etc) but I won't get into that. Perhaps in the future a new specification can be designed which includes an eeprom over I2C to allow more than 8 slot IDs as well as allow for storing of other information that a battery might want to know about a slot. If you have an eeprom, you could just directly report that you are Battery slot number N, Node ID N, connected in Seriallel and your function is SystemBlah. No need for mapping slot number to anything on the autopilot side. But you'd need to power this eeprom... 2 more signals added. This all just adds complexity... but it might be a more robust implementation.
linux on a battery? 🤨 |
|
Ignoring the overhead of dronecan this message is 240 bits which means at 10hz it's ~2.5kbit/s, just FYI. Not bad! |
@hendjoshsr71 I am happy enough with the concept of slots. To me a slot is something that holds a battery and is used to provide persistent information about the function and warning levels. Right now we have arbitrarily number batteries and as they get added and removed they can move about. That makes it hard to manage them in a GCS in a sensible way. I think it is good to know if a slots is primary because it tells me that batteries are used one after each other rather than in parallel. Essentially it's a way to say whether the warnings matter if you have multiple batteries that serve the same function and one of them is low. I am not sure about what information we need. Sorry if I'm rambling. End of day. Beginning of public holiday. |
| uint32 capacity_consumed # [mAh] : This is either the consumption since power-on or since the battery was full, depending on the value of STATUS_FLAG_CAPACITY_RELATIVE_TO_FULL, UINT32_MAX: field not provided | ||
| uint32 capacity_remaining # [mAh] : If STATUS_FLAG_CAPACITY_RELATIVE_TO_FULL is unset, this value is based on the assumption the battery was full when the system was powered, UINT32_MAX: field not provided | ||
| uint32 full_charge_capacity # [mAh] : Predicted battery capacity when fully charged (accounting for battery degradation), UINT32_MAX: field not provided |
There was a problem hiding this comment.
@hamishwillee @hendjoshsr71 integer types were chosen here to match mavlink, but the Battery Status V2 has since changed to floats. I am going to change these here as well to match
| uint8 slot_id # : The physical location of the battery on the aircraft. 0: field not provided | ||
| float32 capacity_consumed # [Ah] : This is either the consumption since power-on or since the battery was full, depending on the value of STATUS_FLAG_CAPACITY_RELATIVE_TO_FULL, NAN: field not provided | ||
| float32 capacity_remaining # [Ah] : If STATUS_FLAG_CAPACITY_RELATIVE_TO_FULL is unset, this value is based on the assumption the battery was full when the system was powered, NAN: field not provided | ||
| float32 full_charge_capacity # [Ah] : Predicted battery capacity when fully charged (accounting for battery degradation), NAN: field not provided |
There was a problem hiding this comment.
I'm sure we discussed this at some point .....
Perhaps this should be moved into BATTERY_INFO? It won't change that often.
| float32 full_charge_capacity # [Ah] : Predicted battery capacity when fully charged (accounting for battery degradation), NAN: field not provided | |
| float32 full_charge_capacity # [Ah] : Predicted battery capacity when fully charged (accounting for battery degradation), NAN: field not provided |
There was a problem hiding this comment.
Looking back at these I am now confused with these three fields.. I think one of them is not needed
float32 capacity_consumed
float32 capacity_remaining
float32 full_charge_capacity
Power monitor:
capacity_consumed = Ah consumed since power on
However with a power module we have to assume we start full anyway, so this really is just
full_charge_capacity - capacity_remaining
Smart battery
capacity_consumed = Total consumed -- again it is full_charge_capacity - capacity_remaining
So capacity_consumed is unneeded since it can be derived, and the derivation is the same in both cases.
Proposed
float32 full_charge_capacity
float32 capacity_remaining
If STATUS_FLAG_CAPACITY_RELATIVE_TO_FULL is set, then capacity_remaining assumes the battery was fully charged at boot. If unset, it is the true remaining capacity.
There was a problem hiding this comment.
While I agree with you, but....
capacity_consumed = full_charge_capacity - capacity_remaining means you would need to set full_charge_capacity on the power_monitor CAN device right? That seems rather annoying for a device that should be a plug in and then the autopilot receives the consumed_ah.
There was a problem hiding this comment.
Good point, what about
float32 full_charge_capacity
float32 capacity_consumed
power module: only reports capacity_consumed and sets full_charge_capacity=NAN
smart battery: reports capacity consumed as capacity relative to full (and the flag is set). Always reports full_charge_capacity, therefore remaining is full - consumed
There was a problem hiding this comment.
MAVLink did it like this in the battery message:
float32 capacity_consumed
float32 capacity_remaining
status flag values_relative_to_full
For a power monitor the values_relative_to_full is false and capacity_consumed is calculated since boot. The capacity remaining is a guess by the autopilot based on its assumptions about the original full state of the battery.
For a smart battery the assumption is that values_relative_to_full is true and the consumed and remaining values are relative to full. So you can always get the full batter capacity by adding them.
In addition, the SMART_BATTERY_INFO explicitly contains the capacity_full_specification and capacity_full (with degradation).
Don't know where that gets you, but it is important to remember that if power monitors are of interest the only thing you can trust is capacity consumed. Everything else is a guess. So I think that should always be sent.
There was a problem hiding this comment.
I'm not sure you could argue that capacity_remaining is redundant especially in a smart battery that can correct this value over time when in use. Reason why is that that capacity_remaining might even change while in use due to temperature changes, also a heavier load causes a battery heat up more quickly.
Furthermore for the end-user capacity_remaining is an easy to use indicator to know how much energy at this time could be retrieved even though it gets changed when in use.
Overall I think this message set looks fine to me and is a huge improvement to the older messages. And I hope we can this is in the DSDL repo sooner then later because I think not having a good battery message set in DroneCAN is hurting adoption of DroneCAN & Smart BMS at the current state.
There was a problem hiding this comment.
@PetervdPerk-NXP I'm sorry for the delayed response.
Certainly, the capacity_remaining would change over time due to heavy loads or temperature.
But can the following arithmetic still apply?
capacity_remaining = full_charge_capacity - capacity_consumed
IE should or can the smart_battery's algorithms be updating the full_charge_capacity estimates. And then place the burden on the end user (usually an autopilot) of the droneCAN message to compute capacity_remaining?
Assuming we are sending full_charge_capacity in the periodic message set every second to 5 seconds.
In mavlink, I agree having capacity_remaining in the message is important because it is easier for a human to understand. And no matter what we need to send both 2 out of the 3 items full_charge_capacity, capacity_consumed, or capacity_remaning since we are on a lossy link.
Ideally, we send fewer bytes in the messages for DroneCAN?
Of course capacity_remaining is only 4 bytes *10 Hz (320bits/sec per battery) . so I'm not opposed to keeping it in the high rate message if @dakejahl is happy to keep it there too? And the droneCAN message would closely mirror mavlink that way.
But if there are good ways to save bytes lets do it?
There was a problem hiding this comment.
I'm okay using the arithmetic you're proposing
capacity_remaining = full_charge_capacity - capacity_consumed
But we should change the description then that full_charge_capacity reflects this.
Regarding size of optimization the 20010.BatteryContinuous.uavcan message, without capacity_remaining is 22 bytes, each DroneCAN CAN Frame can transmit up to 7 bytes, which means that you need 4 frames to transmit the data.
If we could optimize 20010.BatteryContinuous.uavcan to be 21 bytes it would fit in 3 frames.
There was a problem hiding this comment.
Ahh yeah I forgot about the 7 byte payload requirement for DroneCAN. Thanks!
I was reading up here on multiframe transfers here, https://dronecan.github.io/Specification/4._CAN_bus_transport_layer/
It seems the first frame of multi-frame message has only 5 bytes for payload?
So with 21 or 22 bytes of payload we must use 4 frames total?
If that is indeed true then we have 4B to use still? I'd would increase voltage to float32 to mirror mavlink with the extra bytes. Or leave capacity_remaining in?
There was a problem hiding this comment.
I'm okay using the arithmetic you're proposing
capacity_remaining = full_charge_capacity - capacity_consumed
Note that mavlink requires this to be true if MAV_BATTERY_STATUS_FLAGS_CAPACITY_RELATIVE_TO_FULL is set - i.e. in a smart battery the capacity consumed is always relative to full.
With a power monitor that flag is not set. In this case we are indicating capacity_consumed is "since boot". We assume that the battery was full to start with, so you can use the calculation as shown. The difference though is that we know it might be wrong, and indeed will be on battery swaps etc.
|
@tridge |
|
@PetervdPerk-NXP I've been leaving @hendjoshsr71 to handle this as he has been looking at it much more closely. I'm happy to merge this when @hendjoshsr71 is happy with the new messages |
There was a problem hiding this comment.
@dakejahl can we move the full_charge_capacity to the periodic message?
@hendjoshsr71 since the request was processed. When can we expect this being merged? |
|
@tridge since @dakejahl made @hendjoshsr71 requested changes in a6de481 is this all good to be squashed and merged? |
There was a problem hiding this comment.
@dakejahl consider my one comment and then squash all the commits if you could?
|
I can't squash the commits on this repo. If you could do that I can approve it. |
|
Why don't we just merge-squash? Then it would preserve the history of the branch, maybe there's some value to that? 🤷 |
|
Ehh branch will be deleted anyway. Okay I'll squash and push 😆 |
|
I have no idea of the convention in DroneCAN DSDL ? I'm just used to the commits being clean when it goes into a main branch. If someone wants the history see GITHub for actual discussion? |
dakejahl
force-pushed
the
pr-smart_battery
branch
from
ebaf45e
to
ea9b21f
Compare
dakejahl
force-pushed
the
pr-smart_battery
branch
from
ea9b21f
to
66733ba
Compare
|
The admin who has merge authority has a "squash and merge" option when they hit the merge button. Idk sometimes I don't trust myself to manually squash, if you make a mistake and force push you can really screw yourself. Anyway I did it and it looks like I didn't screw it up this time. Almost one year later! Congrats everybody! 😆 🚀 |
|
@dakejahl thanks for all your work on this, and your patience! |
|
Thank you guys for your support getting this message implemented. |
An attempt to establish comprehensive messages for smart battery data. Your input is highly valued!