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

DSDL Suggestion: GenericBatteryInfo #33

Closed
olliw42 opened this issue Aug 13, 2017 · 8 comments
Closed

DSDL Suggestion: GenericBatteryInfo #33

olliw42 opened this issue Aug 13, 2017 · 8 comments

Comments

@olliw42
Copy link

olliw42 commented Aug 13, 2017

I suggest adding this message to the standard

uavcan.equipment.power.GenericBatteryInfo
id = 1093

#
# Generic battery info.
#

uint16 battery_id #can be freely assigned by the user, but must be unique within a system

float16 voltage
float16 current
float16 charge_consumed_mAh

uint8 STATUS_FLAG_ERROR_OVERVOLTAGE  = 1
uint8 STATUS_FLAG_ERROR_UNDERVOLTAGE = 2
uint8 STATUS_FLAG_ERROR_OVERCURRENT  = 4
uint8 STATUS_FLAG_ERROR_UNDERCURRENT = 8
uint8 status_flags

Here, battery_id should not match battery_id of the BatteryInfo message. That is, every battery should have it's own, unique battery_id, but every battery is free to decide which message, GenericBatteryInfo or BatteryInfo, it wants to emit (or both).

Reasons for that additional message: 13th post here ArduPilot/ardupilot#6527

In principle, I would think that the existing BatteryInfo message could need a revision. First, I think it would be better named SmartBatteryInfo. Also, I think the suggested use of battery_id isn't suitable. It should be unique within each system, of course, but otherwise the user may assign it as she/he wants. That is, she/he may number ALL her/his batteries once and for all, more along the lines of a GUID. Anything else would not be very practical. E.g., if the primary battery would have to have battery_id = 0, as currently suggested, every change of a battery would need fumbling around with the id's. How nasty. In that light, a range of unit8 for battery_id might be quite narrow.

@kjetilkjeka
Copy link
Contributor

Also, I think the suggested use of battery_id isn't suitable. It should be unique within each system, of course, but otherwise the user may assign it as she/he wants. That is, she/he may number ALL her/his batteries once and for all, more along the lines of a GUID.

If I understand the intention correctly, the model_instance_id (together with model name) is a sort of GUID, While battery_id is some sort of UUID.

Anything else would not be very practical. E.g., if the primary battery would have to have battery_id = 0, as currently suggested, every change of a battery would need fumbling around with the id's. How nasty. In that light, a range of unit8 for battery_id might be quite narrow.

Would you mind to elaborate a bit on this?

Are you using one or several batteries at the same time? If you're using several batteries are you not required to identify them in the system?

If you're using a single battery wouldn't it be practical to set battery_id to the same constant (e.g. 0) for all of them? You should then be able to swap them without having to reconfigure anything. You could still identify them by using model_instance_id.

@pavel-kirienko
Copy link
Member

battery_id in the existing message is supposed to identify the battery within the system, it doesn't need to be globally unique. I'm going to add some more information about the usage of ID fields to the specification.

@olliw42
Copy link
Author

olliw42 commented Aug 14, 2017

I was thinking that my writing was sufficiently clear, but obviously not LOL.

@ kjetilkjeka:
obviously, as also explicitly mentioned in my text, the primary purpose of battery_id is to distinguish different "batteries" in the system

however:
My concern is that situation can occur there the user would have to adjust the battery_id with each swap of a "battery". In probably the simplest scenario of a UAVCAN power node to which a physical battery is attached to that's not a problem. You have the power node permanently installed in the system, and connect whatever battery you want to it. No problem here.
But a UAVCAN power node (most likely) will have it's own micro controller, and one can think of scenarios where this microcontroller is permanently attached to a battery, so that each swap of a battery would also come with a swap of the microcontroller. This is still no problem as long as one uses only one battery in the system, but if one uses two, one would be seriously limited since some would be 0 and others 1. Here the user would want to give each "battery" a id, which is unique among her/his set of batteries (maybe it's useful to think of that scenarios as a "rudimentary smart battery").
This argument directly extends to smart batteries. My understanding (assumption) is that a smart battery is emitting the BatteryInfo message. I really don't understand how this should work to the user's satisfaction if with any swap of a battery she/he would have to adjust the batter_ids anew.

I also think that a "hardwired" id is not very convenient setup-wise. For instance, the current ArduCopter code (or a PR to that) requires the user to set the id explicitly. That is, a user has to set the id in the UAVCAN node correctly as well as in the flight controller firmware. I don't see any real need for that, it could autodetect the ids. And if we are at that point, to accept an autodetect, then there is really no reason for not relaxing the definition to "It should be unique within each system, of course, but otherwise the user may assign it as she/he wants.", and to be friendly to her/him and give her/him a sufficiently large range of values. I just think it should work similar as for smart batteries.

@ pavel:
I didn't meant "globally unique" in the sense of global = universe. In my answer to kjetilkjeka I've described it now as "unique among her/his set of batteries".

btw: a similar line of reasoning does, IMHO, apply also to CircuitStatus. Interestingly, it's id is uint16. I would be surprised to hear that this is so because one realistically expects to have more than 256 such circuits in the system.

btwII: Maybe one should extend GenericBatteryInfo by a variable field at the end holding the individual cell voltages. Also, the status field needs discussion. E.g. UNDERVOLTAGE, is this supposed to indicate a malfunction, or that a voltage fell below a certain level in the sense of a lipo-saver function? Should one have two flags for each situation?

@kjetilkjeka
Copy link
Contributor

My concern is that situation can occur there the user would have to adjust the battery_id with each swap of a "battery". In probably the simplest scenario of a UAVCAN power node to which a physical battery is attached to that's not a problem. You have the power node permanently installed in the system, and connect whatever battery you want to it. No problem here.
But a UAVCAN power node (most likely) will have it's own micro controller, and one can think of scenarios where this microcontroller is permanently attached to a battery, so that each swap of a battery would also come with a swap of the microcontroller. This is still no problem as long as one uses only one battery in the system, but if one uses two, one would be seriously limited since some would be 0 and others 1. Here the user would want to give each "battery" a id, which is unique among her/his set of batteries (maybe it's useful to think of that scenarios as a "rudimentary smart battery").
This argument directly extends to smart batteries. My understanding (assumption) is that a smart battery is emitting the BatteryInfo message. I really don't understand how this should work to the user's satisfaction if with any swap of a battery she/he would have to adjust the batter_ids anew.

Ok, i undestand now. You're talking about the situation where you use several batteries but there is no need to identify them within the system. Thanks for clarifying.

there is really no reason for not relaxing the definition to "It should be unique within each system, of course, but otherwise the user may assign it as she/he wants."

This seems reasonable. I can't see any reason to disallow components in a system to not care about the battery ID. Would this remove the need for the new message type?

I also think that a "hardwired" id is not very convenient setup-wise. For instance, the current ArduCopter code (or a PR to that) requires the user to set the id explicitly. That is, a user has to set the id in the UAVCAN node correctly as well as in the flight controller firmware. I don't see any real need for that, it could autodetect the ids.

I guess this is needed in a setup where you're using multiple batteries with a need to identify them in the system.

@pavel-kirienko
Copy link
Member

The problem is in the specification (sigh). I'm going to add this to the list of things to explain clearer.

battery_id can only the define the ID of the battery on a particular system. It shall not be used for anything else, because that would defeat the purpose of identification. The flight controller or any other logic must know what specific on-board unit is providing certain data, and it doesn't care how many other batteries the user may have.

I.e. in a single-battery scenario, the ID must be zero always. Taking an aircraft for example equipped with a battery on either side (just for the sake of an example, you understand), the left battery would be designated 0, the right one would be 1. It doesn't matter which particular battery is installed in the right and left slots.

Normally, batteries are not equipped with CAN bus interface due to unreasonable costs involved. CAN is expensive and poorly suited for expendable entities such as batteries. There is already an existing standard in place for smart battery interfaces based on I2C, it works well, and I see no reason to replace it with anything else so far. However, if somebody chose to equip their batteries with CAN, "fumbling around with the ID's" would be unavoidable.

@pavel-kirienko
Copy link
Member

Regarding the cell voltages, I'm not sure why that would be useful, as well as I don't see any value in reporting the amount of used energy only. The autopilot or any other external entity would mostly care about higher-level concepts related to the battery: its state of charge, its state of health, time-to-discharge, et cetera. Normally, in an idealistic scenario, no other node in the system should care about low-level parameters such as mAh or cell voltages, because they cannot be sensibly interpreted without a sizeable amount of context (such as the type of the battery, its capacity, and so on).

The proposition to just throw in another message for voltages and mAh sounds like an attempt to design a better I2C rather than a proper data bus.

@olliw42
Copy link
Author

olliw42 commented Aug 18, 2017

I'm sorry, I really give up now with suggestions on DSDL
battery_id: if it is fixed to a slot, which IMHO is indeed a good scenario, then node id would do it equally well. Superflous.
mAh: I really get the impression that you don't care about any paractical realities. It feels as if you don't want UAVCAN to be used except for exactly the "high-level" stuff you have in mind. Pl see https://discuss.ardupilot.org/t/uavcan-for-hobbiests/16464/59 ifor an example for why it can be good to have that data. I can't see how one could argue that CircuitStatus is a reasonable substitute.
Anyway, as said, I give up on this. The DSDL standard is predictably doomed. Gladly the UAVCAN transport layer is well designed, and DSDL sufficiently rich, so that it's no real issue.

:)

@olliw42 olliw42 closed this as completed Aug 18, 2017
@EShamaev
Copy link

battery_id: if it is fixed to a slot, which IMHO is indeed a good scenario, then node id would do it equally well. Superflous.

There is auto id numbering being requested. In such case it will not work at all.

mAh: I really get the impression that you don't care about any paractical realities.

Wh is actually much more usefull. The motors really do consume current but for propulsion the power is much more meaningfull parameter. So the remaining power is what to be considered and not only mAh. That is really practical side.

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

No branches or pull requests

4 participants