AP_BattMonitor: UAVCAN support #6527

EShamaev · 2017-07-01T11:30:27Z

Support for voltage and current with CircuitStatus and BatteryInfo messages over UAVCAN.

WickedShell · 2017-07-01T20:58:27Z

I haven't had time to look at it in detail, but is there any reason why we couldn't just do a single UAVCAN backend that pulls either a battery status or a circuit status message? (with a preference for the battery one if it's available?)

EShamaev · 2017-07-01T20:59:54Z

The messages are very different and ID of circuit and Battery have absolutely different meanings.

EShamaev · 2017-07-01T21:08:34Z

But if there is a way to have 2 different IDs at the same time (one for circuit and one for battery) with current set of parameters, I will be happy to make such change.

WickedShell · 2017-07-01T21:20:09Z

I'm a bit confused, the same node wouldn't be sending both CircuitStatus and BatteryInfo would it? From an implementation perspective the only difference I see is that one of them happens to come with temperature information. AP_BattMonitor at the moment is explicitly and only intended for batteries (particularly meant for things that when they get low/consume a large amount of power we expect the vehicle to begin a failsafe action). If we want generic power logging around the system I have to wonder if that deserves a different library, (primarily so that we can have proper MAVLink reporting, and don't gain so many parameters per device we are monitoring).

EShamaev · 2017-07-01T21:24:05Z

I am referring not to node ID.
With CircuitStatus, same node can send many circuits ID (it is an electronics switch board with solid state circuit brakers and has 16 of circuits IDs) so this PR uses CircuitStatus ID and Battery ID and not the node ID, which seems much more appropriate to differentiate among those.

As for generic power logging, I am preparing such system, but it needs some more time.

EShamaev · 2017-07-01T21:26:01Z

For example "power bricks" used now has really nothing to do with batteries, but we treat voltage and current from those as a battery information.
The same is with CircuitStatus message.
If it is reporting voltage and current from battery - we can well define it a battery.

EShamaev · 2017-07-01T21:29:11Z

the same node wouldn't be sending both CircuitStatus and BatteryInfo would it?

That might be.
Smart battery that I use has generator input for charging and it is broadcasting it as a CircuitStatus together with BatteryInfo.

WickedShell · 2017-07-03T19:05:33Z

My hesitation is why we would accept a CircuitStatus as a battery input. We have the logical distinction, and I'm not so keen on blurring the line inside the BattMonitor library.

I'd argue the power bricks are a bit different, people tend to stick them upstream of all other components in the system, so it is actually monitoring the entire pack draw, which makes it a fair report on the battery status.

We only have support for 2 battery instances at the moment, and adding more eats parameters quickly (point of fact I don't think we have enough parameters to go past 5 in the current parameter group), which means that the 16 node CircutStatus wouldn't be fully monitored anyways...

EShamaev · 2017-07-03T19:18:14Z

2 instances are really enough, maybe only in extreme cases it can go over that.
Out of the 16 node CircutStatus I mentioned, only 2 are inputs from battery and the rest are outputs.
So as a use case, CircutStatus can represent the battery just like the powerbrick. And I actually expect that someone will release a "smart" power brick that will communicate over CAN. And in that case it will be CircutStatus message, not the Battery.
Anyway after combining the messages, the difference in AP_BattMonitor is really minimal and both can be supported without almost any overhead.

liang-tang · 2017-07-15T17:31:48Z

what is CircuitStatus?

olliw42 · 2017-08-05T16:51:00Z

~~any chance this is getting merged anytime soon?~~ not needed anymore, did it myself

I do have a working DIY uavcan power brick emitting power.CircuitStatus messages now, and would be eager to test it in real life
https://discuss.ardupilot.org/t/uavcan-for-hobbiests/16464/52

:)

olliw42 · 2017-08-12T08:03:03Z

I'm not happy with the definition and usage of the uavcan.equipment.power messages

The usage of CircuitStatus I find illogical, and looks more like a misuse. The point IMHO is this: A "battery" has a specific additional function in AP (and likely any other system), in that it's status can trigger particular reactions. Specifically, e.g. a RTL. This would be so for any "battery", e.g. a smart battery, or a battery with a 3DR module to measure its voltage/current, or any other "thing" which powers the system and the voltage/current of which is used in that way. So, there is a clear cut conceptional difference between a "battery" and "any other electronics circuit". This is not reflected at all by the messages, and the above discussion and confusion is IMHO just a clear indication of that.

So, I would propose a set of "battery" messages, which is used for the purpose at hand, and to not use CircuitStatus. BatteryInfo is then for smart batteries (sad that it wasn't called SmartBatteryInfo). I suggest to add a new message "GenericBatteryInfo" (or named whatever), and to use that instead of CircuitStatus in this PR. For its fields I tentatively suggest

uint16 battery_id
float16 voltage
float16 current
float16 charge_consumed_mAh
uint8 status_flags

The addition of charge_consumed_mAh I think should be very helpful. Using current and the time difference between two messages does not allow a precise determination of the discharge. However, any node, which very likely is able to measure the current at much higher rate, can produce a much more precise measurement of the consumed charge. In my "old" days when I was flying a 450 heli a lot, which I equipped with my own-made telemetry, I found this to easily make a difference of 5-10%, which is a lot, IMHO.

This could also allow to not having to specify the "id", simplifying the setup enormously. E.g, if one can assume that only two batteries are in the system, one simply could catch all incoming messages of the battery type, and auto-fill in the SERIAL_NUM as it is done for a smart battery. If more than two batteries are connected , it's the very same situation as with all other "batteries".

If you think about it, the need to specify the correct circuit_id or battery_id makes the whole thing totally useless in practical terms. This is just another way to present the conceptual issue mentioned in the above.

Even though OT, I feel this needs to be added: It appears that essentially all messages of the "standard data types" which had been recently touched are quite ill designed, which I think should make one wonder if the adopted procedure which resulted in these standards is appropriate for defining a standard, and if it shouldn't be replaced by a more appropriate procedure.
IMHO there should be some sort of "confirmed to work in practice" before a message is approved as standard.

olliw42 · 2017-08-12T21:16:58Z

this is a bug in L389:

ardupilot/libraries/AP_UAVCAN/AP_UAVCAN.cpp

Line 389 in 4c3487c

for (uint8_t i = 0; i < AP_UAVCAN_MAX_MAG_NODES; i++) {

it should be: for (uint8_t i = 0; i < AP_UAVCAN_MAX_BI_NUMBER; i++) {

bug or not?
it was intentionally that the cs listerners are sized by AP_UAVCAN_MAX_CS_NUMBER and not AP_UAVCAN_MAX_LISTENERS, as all the other listerners, right? (because there are so much more?)(maybe worth a comment)
uint16_t _cs_BM_listener_to_id[AP_UAVCAN_MAX_CS_NUMBER];
AP_BattMonitor_Backend* _cs_BM_listeners[AP_UAVCAN_MAX_CS_NUMBER];
https://github.com/EShamaev/ardupilot/blob/4c3487cea190a62dd83656c2d8eee3c93718fad9/libraries/AP_UAVCAN/AP_UAVCAN.h#L163-L164
https://github.com/EShamaev/ardupilot/blob/4c3487cea190a62dd83656c2d8eee3c93718fad9/libraries/AP_UAVCAN/AP_UAVCAN.cpp#L378-L387
Isn't this a bit of breaking the logic? I mean, obviously it's possible to have a different number of instances and number of listeners, so one should have two defines, as for the other messages. If that is in fact not allowed, then one define for all messages would be appropriate, as for the cs message. It's a bit wired as it is.

Isn't it a bit inelegant that the data structures in AP_UAVCAN are called XXXX_Info (except for gps), but the respective array to get them _xxxxx_id_state[xx]?

I understand that using e.g. macros/templates to define functions etc. isn't considered the best practice by everyone, but the code in AP_UAVCAN.cpp is so extremely repetitive, that I would consider that here appropriate, and make it much cleaner and more readable.

EShamaev · 2017-11-12T14:26:27Z

Removed CircuitStatus from battery monitoring.
New Electrical subsystem is coming soon.

magicrub · 2018-01-19T22:05:50Z

Hi @EShamaev can we get this rebased please?

magicrub · 2018-01-26T22:13:10Z

status update?

magicrub · 2018-02-08T19:55:22Z

@EShamaev ping. Needs rebase.

EShamaev · 2018-02-16T22:53:40Z

@magicrub Rebased, squashed.

WickedShell

Couple of small points on the battery side (mostly due to the param changes). I've otherwise ignored the UAVCAN side of the changes though..

WickedShell · 2018-02-18T01:13:22Z

libraries/AP_BattMonitor/AP_BattMonitor.h

@@ -45,6 +47,23 @@ class AP_BattMonitor
        return *_singleton;
    }

+    // Battery monitor driver types
+    enum BattMonitor_Type {


Accidental regression? (This has moved to batt monitor params )

WickedShell · 2018-02-18T01:14:21Z

libraries/AP_BattMonitor/AP_BattMonitor_Params.cpp

@@ -7,7 +7,8 @@ const AP_Param::GroupInfo AP_BattMonitor_Params::var_info[] = {
    // @Param: MONITOR
    // @DisplayName: Battery monitoring
    // @Description: Controls enabling monitoring of the battery's voltage and current
-    // @Values: 0:Disabled,3:Analog Voltage Only,4:Analog Voltage and Current,5:Solo,6:Bebop,7:SMBus-Maxell
+    // @Values: 0:Disabled,3:Analog Voltage Only,4:Analog Voltage and Current,5:Solo,6:Bebop,7:SMBus-Maxell,8:UAVCAN-BatteryInfo
+


Extra whitespace.

WickedShell · 2018-02-18T01:14:31Z

libraries/AP_BattMonitor/AP_BattMonitor_Params.cpp

@@ -63,7 +64,8 @@ const AP_Param::GroupInfo AP_BattMonitor_Params::var_info[] = {

    // @Param: SERIAL_NUM
    // @DisplayName: Battery serial number
-    // @Description: Battery serial number, automatically filled in for SMBus batteries, otherwise will be -1
+    // @Description: Battery serial number, automatically filled in for SMBus batteries, otherwise will be -1. With UAVCAN it is the battery_id.
+


Extra whitespace.

WickedShell · 2018-02-18T01:15:12Z

libraries/AP_BattMonitor/AP_BattMonitor.h

+    };
+
+    // low voltage sources (used for BATT_LOW_TYPE parameter)
+    enum BattMonitor_LowVoltage_Source {


WickedShell · 2018-02-18T01:20:59Z

libraries/AP_BattMonitor/AP_BattMonitor_UAVCAN.cpp

+        // .0002778 is 1/3600 (conversion to hours)
+        _state.current_total_mah += (float) ((double) _state.current_amps * (double) dt * (double) 0.0000002778);
+    }
+


You could log from the _state.consumed_wh = full_charge_capacity_wh - remaining_capacity_wh (I get that this is new from when the PR first came in).

EShamaev · 2018-02-24T14:19:36Z

@WickedShell Done, rebased, squashed

WickedShell

Battery side looks reasonable after a quick skim.

@EShamaev why add helper functions that are unusued? It seems to me that unused helper functions are extremely prone to bitrot, and this is a case where a more generic version would probably actually be safer/easier to maintain. (This is just a side question, not related to actually merging this)

EShamaev · 2018-02-24T17:24:33Z

First, I tend to implement the whole interface so for example if there is addition - I also do removing. That helps to see if the overal idea of the implementation is working and done properly.
Second, they are logical and even if not used right now in platforms that we have - does not mean that they will not be used ever.
Example is: #7768

However I understand your point of view, but would prefer them to stay.

magicrub · 2018-03-01T23:45:59Z

LGTM. Only weird part about it is BATT_SERIAL_NUM will require a reboot to take effect... only with UAVCAN. I'm not sure if that's worth putting in the description, or the @ REQUIRESREBOOT field, but should probably be noted in a comment somewhere

magicrub · 2018-03-01T23:47:23Z

regarding the dead-code rule. I am normally in favor of that but in the case of CAN there's a lot of development going on so I'd rather leave it in until we get the drivers all filled out, then we can go back and clean things up.

magicrub · 2018-03-01T23:48:10Z

merged!

skyyuzhang · 2018-12-11T08:35:50Z

I been researching AP_BattMonitor.UAVCAN.cpp for a while. I want to make uavcan batter monitor. I used libcanard to developer my project. data type which I used is "uavcan.equipment.power.BatteryInfo" I can get data volt,current,temperature in UAVCAN GUI correctly . but I can not get data in MissionPlanner. it always shown "Bat 0.00v 0.0A 100%" also it is red word . the bitrate is 1000000. I check everything I can thought but It not work .can you help me ? thank you !! the parameter I set are as follow

Copter-V3.6.2
CAN_D1_PROTOCOL =1
CAN_D2_PROTOCOL =1
CAN_D1_DRIVER =1
CAN_D2_DRIVER =2
BATT_MONITOR =8
BATT_SERIAL_NUM=0 note: my uavcan battery id is 0(not node id!!!)

here is my code

https://github.com/skyyuzhang/uavcan_bm

EShamaev · 2018-12-11T09:19:19Z

Please try battery id distinct from zero.

Current version is working for sure, so the problem seems to be in code other than AP.

skyyuzhang · 2018-12-11T13:55:15Z

Please try battery id distinct from zero.

Current version is working for sure, so the problem seems to be in code other than AP.

Actually It was no use even I set battery distinct from zero . one of thing is I can get data on UAVCAN GUI Tool but I can not get data on mission planner. I have been finding the problem but no use . does libcanard supported ardupilot? or others problem? thank god ! I can not get this done . please help~~~~ thank you!!!

475241512 · 2019-07-18T01:35:13Z

Please try battery id distinct from zero.
Current version is working for sure, so the problem seems to be in code other than AP.

Actually It was no use even I set battery distinct from zero . one of thing is I can get data on UAVCAN GUI Tool but I can not get data on mission planner. I have been finding the problem but no use . does libcanard supported ardupilot? or others problem? thank god ! I can not get this done . please help~~~~ thank you!!!

Have you solved the problem？I used libcanard "uavcan.equipment.power.BatteryInfo"datatype,It can be get and view in UAVCAN GUI,but mission planner always shown "0.00V,0.0A,100%",What did I do wrong? Do I have any details to pay attention to?thanks.

EShamaev added AllVehicles CAN Driver Enhancement Library WIP labels Jul 1, 2017

EShamaev requested a review from OXINARF July 1, 2017 11:30

EShamaev force-pushed the AP_BattMonitor_UAVCAN_support branch from 4b96694 to 81ea989 Compare July 1, 2017 19:03

EShamaev removed the WIP label Jul 1, 2017

EShamaev changed the title ~~WIP: AP_BattMonitor: UAVCAN support~~ AP_BattMonitor: UAVCAN support Jul 1, 2017

EShamaev force-pushed the AP_BattMonitor_UAVCAN_support branch from 81ea989 to ff5a757 Compare July 1, 2017 20:25

EShamaev requested a review from WickedShell July 1, 2017 20:38

EShamaev force-pushed the AP_BattMonitor_UAVCAN_support branch from ff5a757 to 4c3487c Compare July 3, 2017 15:37

olliw42 mentioned this pull request Aug 13, 2017

DSDL Suggestion: GenericBatteryInfo OpenCyphal/public_regulated_data_types#33

Closed

EShamaev force-pushed the AP_BattMonitor_UAVCAN_support branch from 53df3db to 3e14a14 Compare September 17, 2017 08:48

olliw42 mentioned this pull request Nov 3, 2017

WIP (do not merge): "proper" fork containing OlliW's betacopter36dev-v005 #7167

Closed

EShamaev force-pushed the AP_BattMonitor_UAVCAN_support branch from 0b4bc29 to 75e3352 Compare November 12, 2017 09:15

EShamaev force-pushed the AP_BattMonitor_UAVCAN_support branch 2 times, most recently from d2a7513 to 633902d Compare February 16, 2018 22:34

WickedShell requested changes Feb 18, 2018

View reviewed changes

EShamaev added 2 commits February 24, 2018 16:18

AP_BattMonitor: UAVCAN battery information added

09dd9f4

AP_UAVCAN: UAVCAN battery information added

31417ff

EShamaev force-pushed the AP_BattMonitor_UAVCAN_support branch from 633902d to 31417ff Compare February 24, 2018 14:19

WickedShell approved these changes Feb 24, 2018

View reviewed changes

EShamaev requested a review from magicrub February 24, 2018 17:26

magicrub approved these changes Mar 1, 2018

View reviewed changes

magicrub merged commit 844ed61 into ArduPilot:master Mar 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AP_BattMonitor: UAVCAN support #6527

AP_BattMonitor: UAVCAN support #6527

EShamaev commented Jul 1, 2017 •

edited

Loading

WickedShell commented Jul 1, 2017

EShamaev commented Jul 1, 2017

EShamaev commented Jul 1, 2017

WickedShell commented Jul 1, 2017

EShamaev commented Jul 1, 2017

EShamaev commented Jul 1, 2017

EShamaev commented Jul 1, 2017

WickedShell commented Jul 3, 2017

EShamaev commented Jul 3, 2017

liang-tang commented Jul 15, 2017

olliw42 commented Aug 5, 2017 •

edited

Loading

olliw42 commented Aug 12, 2017 •

edited

Loading

olliw42 commented Aug 12, 2017 •

edited

Loading

EShamaev commented Nov 12, 2017

magicrub commented Jan 19, 2018

magicrub commented Jan 26, 2018

magicrub commented Feb 8, 2018

EShamaev commented Feb 16, 2018

WickedShell left a comment

WickedShell Feb 18, 2018

WickedShell Feb 18, 2018

WickedShell Feb 18, 2018

WickedShell Feb 18, 2018

WickedShell Feb 18, 2018

EShamaev commented Feb 24, 2018

WickedShell left a comment

EShamaev commented Feb 24, 2018

magicrub commented Mar 1, 2018

magicrub commented Mar 1, 2018

magicrub commented Mar 1, 2018

skyyuzhang commented Dec 11, 2018 •

edited

Loading

EShamaev commented Dec 11, 2018

skyyuzhang commented Dec 11, 2018

475241512 commented Jul 18, 2019

AP_BattMonitor: UAVCAN support #6527

AP_BattMonitor: UAVCAN support #6527

Conversation

EShamaev commented Jul 1, 2017 • edited Loading

WickedShell commented Jul 1, 2017

EShamaev commented Jul 1, 2017

EShamaev commented Jul 1, 2017

WickedShell commented Jul 1, 2017

EShamaev commented Jul 1, 2017

EShamaev commented Jul 1, 2017

EShamaev commented Jul 1, 2017

WickedShell commented Jul 3, 2017

EShamaev commented Jul 3, 2017

liang-tang commented Jul 15, 2017

olliw42 commented Aug 5, 2017 • edited Loading

olliw42 commented Aug 12, 2017 • edited Loading

olliw42 commented Aug 12, 2017 • edited Loading

EShamaev commented Nov 12, 2017

magicrub commented Jan 19, 2018

magicrub commented Jan 26, 2018

magicrub commented Feb 8, 2018

EShamaev commented Feb 16, 2018

WickedShell left a comment

Choose a reason for hiding this comment

WickedShell Feb 18, 2018

Choose a reason for hiding this comment

WickedShell Feb 18, 2018

Choose a reason for hiding this comment

WickedShell Feb 18, 2018

Choose a reason for hiding this comment

WickedShell Feb 18, 2018

Choose a reason for hiding this comment

WickedShell Feb 18, 2018

Choose a reason for hiding this comment

EShamaev commented Feb 24, 2018

WickedShell left a comment

Choose a reason for hiding this comment

EShamaev commented Feb 24, 2018

magicrub commented Mar 1, 2018

magicrub commented Mar 1, 2018

magicrub commented Mar 1, 2018

skyyuzhang commented Dec 11, 2018 • edited Loading

EShamaev commented Dec 11, 2018

skyyuzhang commented Dec 11, 2018

475241512 commented Jul 18, 2019

EShamaev commented Jul 1, 2017 •

edited

Loading

olliw42 commented Aug 5, 2017 •

edited

Loading

olliw42 commented Aug 12, 2017 •

edited

Loading

olliw42 commented Aug 12, 2017 •

edited

Loading

skyyuzhang commented Dec 11, 2018 •

edited

Loading