Add Smart_Battery_Info MAVLink message #16167

hendjoshsr71 · 2020-12-30T03:30:03Z

This adds the MAVLink message 370 SMART_BATTERY_INFO to send serial number, cycles, capacity, product name, etc.

PR to add the message to ArduPilot/mavlink here: ArduPilot/mavlink#181
One small documentation change on the MAVLink side here: mavlink/mavlink#1551

serial_number: I wasn't sure the best way/place to deal with the conversion from int32_t to char[16].
AP_BattMonitor.h: get_serial_number() that is used by AP_BattMonitor_UAVCAN.h not sure how to combine this together and limit the extra functions

SIM_BattMonitor_SMBus.cpp Issue: The method to fill the registers for manufacturer_name and device_name causes the data from device name to write over all but the first char of the manufacturer_name. (Fixed here #16228)

libraries/GCS_MAVLink/GCS_Common.cpp

amilcarlucas · 2020-12-30T09:26:49Z

The message is marked as "work-in-progress". Is it a good idea to bring it in? I do like your changes though.

peterbarker

I love the addition to the simulated battery.

There might be some push-back on having the method returning a string for the serial number. We'll see.

@magicrub you might be interested in options to pass more CAN battery information back to the GCS with this message.

This looks pretty good, really.

libraries/AP_BattMonitor/AP_BattMonitor.cpp

libraries/AP_BattMonitor/AP_BattMonitor_Backend.h

libraries/AP_BattMonitor/AP_BattMonitor_SMBus.cpp

libraries/AP_BattMonitor/AP_BattMonitor_SMBus.h

libraries/GCS_MAVLink/GCS_Common.cpp

peterbarker · 2021-01-02T10:47:18Z

libraries/GCS_MAVLink/GCS_Common.cpp

+    for(uint8_t i = 0; i < battery.num_instances(); i++) {
+        if (battery.get_type(i) != AP_BattMonitor::Type::NONE) {
+            CHECK_PAYLOAD_SIZE(SMART_BATTERY_INFO);
+            send_smart_battery_info(i);
+        }


@WickedShell has grown to dislike this pattern, preferring to keep some state around indicating the last instance we successfully sent.

The pattern for the other battery message like you describe has to be modified with the comment below due to where I have the message being sent from. Suggestions?

for(uint8_t i = 0; i < battery.num_instances(); i++) { const uint8_t battery_id = (last_smart_battery_info_idx + 1) % AP_BATT_MONITOR_MAX_INSTANCES; if (battery.get_type(battery_id) != AP_BattMonitor::Type::NONE) { CHECK_PAYLOAD_SIZE(SMART_BATTERY_INFO); send_smart_battery_info(battery_id); last_smart_battery_info_idx = battery_id; //return true; } else { last_smart_battery_info_idx = battery_id; } } return true;

peterbarker

I'm going to mark this for discussion at the European DevCall. I want more people to chime in on the send-as-part-of-banner thing.

libraries/AP_BattMonitor/AP_BattMonitor.cpp

libraries/AP_BattMonitor/AP_BattMonitor_SMBus.cpp

libraries/GCS_MAVLink/GCS_Common.cpp

peterbarker · 2021-01-05T03:46:25Z

@hendjoshsr71 let me know if you'd like me to rebase this on the changes I've just made to the simulated-smbus stuff - it obviously conflicted. I think the commit in your list here can just be removed.

hendjoshsr71 · 2021-01-05T04:16:04Z

I got it. Thanks for the update! Adding in capacity and other fields to the sim and will reupload once working.

hendjoshsr71 · 2021-01-05T10:10:37Z

Added design capacity and few more items to SITL_SMBus model

amilcarlucas · 2021-03-02T16:23:33Z

This needs a rebase

hendjoshsr71 · 2021-03-02T21:15:04Z

Rebase complete.

peterbarker

Once this is merged we should send a message to the gcs-maintainers mailing list letting them know that they can request the message...

@WickedShell you're going to want to be happy with this before merge, I think?

libraries/AP_BattMonitor/AP_BattMonitor.cpp

libraries/AP_BattMonitor/AP_BattMonitor_SMBus.h

hendjoshsr71 · 2021-03-15T09:19:43Z

Thanks @peterbarker for the all the reviews!!

Moved the SMBUS stuff to the backends.
Moved the logging of the BATI message to the new AP_BattMonito_Logging.cpp file
Fixed hopefully all the trailing whitespaces...

peterbarker · 2021-03-15T09:48:30Z

On Mon, 15 Mar 2021, Josh Henderson wrote: Used, for the snprintf's in the serial number, product, device, and manufacturer name functions

In the header?

hendjoshsr71 · 2021-03-15T09:51:47Z

On Mon, 15 Mar 2021, Josh Henderson wrote: Used, for the snprintf's in the serial number, product, device, and manufacturer name functions
In the header?

Ahh I think I see your point it should be in the `AP_BattMonitor_SMBus.cpp' instead?

hendjoshsr71 · 2021-03-15T21:56:46Z

Changed the capacity functions to int32_t as smbus spec of uint16 doesn't leave much room at 65,536 mAh
Still need to handle the strncpy truncation warnings
Could add manufactured date?

tridge

i think we need to use HAL_MINIMIZE_FEATURES for this feature, to prevent the flash and ram cost for small boards with 1M flash

libraries/AP_BattMonitor/AP_BattMonitor_SMBus.h

libraries/SITL/SIM_BattMonitor_SMBus.cpp

libraries/AP_BattMonitor/AP_BattMonitor_Logging.cpp

tridge · 2021-06-29T01:15:46Z

added DelayMerge for delay after final rebase for 4.1.0

libraries/GCS_MAVLink/GCS_Common.cpp

hendjoshsr71

Fixed the GCS_send so that false only gets returned if there is no space on the link.

Tested with 2 analog battmons, 1 smbus_generic, 1 rotoye, and 1 maxell. That logging and mavlink send works with my mavproxy submodule

Tested with SMART_BATTERY_INFO disabled and checked logs such that all makes sense for given monitor type.

amilcarlucas · 2021-07-06T00:02:25Z

This requires 2440 bytes on Durandal (arducopter)

WickedShell

Some nitpicks. I didn't have time to fully tell but the initialization bit still looks worrisome to me, as I'm not seeing a path where we don't try and spend a ton of I2C bandwidth talking to batteries that aren't there....

WickedShell · 2021-06-16T21:55:01Z

libraries/AP_BattMonitor/AP_BattMonitor.cpp

+    logger->Write_Power();
+
+#if HAL_SMART_BATTERY_INFO_ENABLED
+    // Check if the log_count has changed as logging can open/close at anytime


Can this be moved down into the other #if check below? It would help keep the ideas closer together and make the file more readable

I moved it, but now it all gets called inside the loop more times than necessary on each read cycle.

WickedShell · 2021-06-16T21:59:18Z

libraries/AP_BattMonitor/AP_BattMonitor.cpp

+#endif
+
+    for (uint8_t i=0; i<_num_instances; i++) {
+        if (drivers[i] == nullptr || get_type(i) == Type::NONE) {


Let's remove the check for type being none. Changing type already needs a restart, and it helps save some flash cost. (Unless there's a different/better reason I'm missing here?)

Removed. What about L298 below? I think I've missed the point of checking Type::NONE that was here before this PR.

for (uint8_t i=0; i<_num_instances; i++) { if (drivers[i] != nullptr && get_type(i) != Type::NONE) { drivers[i]->read(); drivers[i]->update_resistance_estimate(); } }

libraries/AP_BattMonitor/AP_BattMonitor_Logging.cpp

libraries/AP_BattMonitor/AP_BattMonitor_Backend.h

libraries/AP_BattMonitor/AP_BattMonitor_SMBus.cpp

libraries/AP_BattMonitor/AP_BattMonitor_SMBus.h

libraries/AP_BattMonitor/AP_BattMonitor_SMBus_Generic.cpp

WickedShell · 2021-07-08T19:07:59Z

libraries/AP_BattMonitor/AP_BattMonitor_SMBus_Generic.cpp

-    const uint8_t read_size = bufflen + 1 + (_pec_supported ? 1 : 0);
-    uint8_t buff[read_size];
+    // check cell count
+    if (_state.healthy) {


Unless I've missed something healthy isn't actual true initially, and it's normally toggled by the polling loop. Why are we looking at it at all inside of the initialization block?

Especially since this is called repeatedly during polling until the initialized flag (which can only be set by entering here) doesn't this mean we might repeatedly poll for all the smart battery info for ever on a non healthy battery and massively bring down the I2C bus by accident?

We would only continue to poll for smart battery info. that hasn't already been successfully read, but...

I think your right about pulling it down due to the possible delays in SMBus. The crux of the issue is the termination condition & its placement for reading these registers. I've added a separate timer for reading info register termination.

However, guidance on the appropriate termination condition for reading the info. registers would be appreciated.

Use a timeout?

for each called register only allow one attempt to read

only call initialize() Z number of times

If I brought back all of the bool returns from the read_zz() I could OR those and return before timeout as well? (But that costs flash.)

The // check cell count section of code appears to be an initialization task to set _cell_count_fixed. I was trying to pull out initialization tasks into here. For now I'm going to put this block back and leave getting that into here for another day.

I really wish there was way to run these initialization type tasks first on the bus before registering the timer()

hendjoshsr71

@WickedShell Addressed most of your comments. But I have left a question or two about the termination condition for the initialization function.

libraries/AP_BattMonitor/AP_BattMonitor_SMBus.cpp

hendjoshsr71 · 2021-07-12T17:52:38Z

libraries/AP_BattMonitor/AP_BattMonitor.cpp

+    logger->Write_Power();
+
+#if HAL_SMART_BATTERY_INFO_ENABLED
+    // Check if the log_count has changed as logging can open/close at anytime


I moved it, but now it all gets called inside the loop more times than necessary on each read cycle.

hendjoshsr71 · 2021-07-12T17:52:44Z

libraries/AP_BattMonitor/AP_BattMonitor.cpp

+#endif
+
+    for (uint8_t i=0; i<_num_instances; i++) {
+        if (drivers[i] == nullptr || get_type(i) == Type::NONE) {


Removed. What about L298 below? I think I've missed the point of checking Type::NONE that was here before this PR.

for (uint8_t i=0; i<_num_instances; i++) { if (drivers[i] != nullptr && get_type(i) != Type::NONE) { drivers[i]->read(); drivers[i]->update_resistance_estimate(); } }

libraries/AP_BattMonitor/AP_BattMonitor_Backend.h

libraries/AP_BattMonitor/AP_BattMonitor_SMBus.cpp

hendjoshsr71 · 2021-07-15T05:57:22Z

libraries/AP_BattMonitor/AP_BattMonitor_SMBus_Generic.cpp

-    const uint8_t read_size = bufflen + 1 + (_pec_supported ? 1 : 0);
-    uint8_t buff[read_size];
+    // check cell count
+    if (_state.healthy) {


We would only continue to poll for smart battery info. that hasn't already been successfully read, but...

I think your right about pulling it down due to the possible delays in SMBus. The crux of the issue is the termination condition & its placement for reading these registers. I've added a separate timer for reading info register termination.

However, guidance on the appropriate termination condition for reading the info. registers would be appreciated.

Use a timeout?

for each called register only allow one attempt to read

only call initialize() Z number of times

If I brought back all of the bool returns from the read_zz() I could OR those and return before timeout as well? (But that costs flash.)

The // check cell count section of code appears to be an initialization task to set _cell_count_fixed. I was trying to pull out initialization tasks into here. For now I'm going to put this block back and leave getting that into here for another day.

hendjoshsr71 · 2021-07-15T06:27:59Z

libraries/AP_BattMonitor/AP_BattMonitor_SMBus_Generic.cpp

-    const uint8_t read_size = bufflen + 1 + (_pec_supported ? 1 : 0);
-    uint8_t buff[read_size];
+    // check cell count
+    if (_state.healthy) {


I really wish there was way to run these initialization type tasks first on the bus before registering the timer()

hendjoshsr71 · 2021-09-22T20:05:28Z

Rebased due to #if HAL_GCS_ENABLED change

amilcarlucas suggested changes Dec 30, 2020

View reviewed changes

libraries/GCS_MAVLink/GCS_Common.cpp Outdated Show resolved Hide resolved

amilcarlucas requested a review from WickedShell December 30, 2020 09:27

hendjoshsr71 force-pushed the smart_batt_mavlink branch from f173c3c to 40c1c25 Compare December 30, 2020 22:52

peterbarker requested changes Jan 2, 2021

View reviewed changes

hendjoshsr71 force-pushed the smart_batt_mavlink branch 2 times, most recently from 7d68053 to 2cc10fa Compare January 4, 2021 23:17

hendjoshsr71 mentioned this pull request Jan 4, 2021

AP_Logger & GCS_MAVLink: Log smart battery cycle count #16157

Closed

peterbarker requested changes Jan 5, 2021

View reviewed changes

hendjoshsr71 force-pushed the smart_batt_mavlink branch from 2cc10fa to 697046d Compare January 5, 2021 10:08

hendjoshsr71 force-pushed the smart_batt_mavlink branch from 697046d to 27febdc Compare January 27, 2021 00:52

TunaLobster mentioned this pull request Feb 2, 2021

AP_BattMonitor: add logging of cycle count (and other health data) to onboard logs #13061

Closed

hendjoshsr71 force-pushed the smart_batt_mavlink branch 2 times, most recently from efaab7e to d901a36 Compare March 2, 2021 21:09

IamPete1 added the DevCallTopic label Mar 14, 2021

peterbarker requested changes Mar 15, 2021

View reviewed changes

hendjoshsr71 force-pushed the smart_batt_mavlink branch from d901a36 to 43813be Compare March 15, 2021 09:08

hendjoshsr71 force-pushed the smart_batt_mavlink branch from 43813be to 6c5ce94 Compare March 15, 2021 09:48

hendjoshsr71 force-pushed the smart_batt_mavlink branch 2 times, most recently from c45751c to fc3bc5d Compare March 15, 2021 21:54

tridge requested changes Mar 16, 2021

View reviewed changes

tridge added the DelayMerge label Jun 29, 2021

peterbarker reviewed Jun 29, 2021

View reviewed changes

libraries/GCS_MAVLink/GCS_Common.cpp Outdated Show resolved Hide resolved

CraigElder removed the DevCallTopic label Jun 29, 2021

hendjoshsr71 force-pushed the smart_batt_mavlink branch 2 times, most recently from 1875af3 to ce802c4 Compare June 30, 2021 06:13

hendjoshsr71 commented Jun 30, 2021

View reviewed changes

hendjoshsr71 force-pushed the smart_batt_mavlink branch from ce802c4 to ec0a733 Compare July 1, 2021 21:16

hendjoshsr71 mentioned this pull request Jul 3, 2021

mavgen_python: fix to_string decoding of arrays with garbage ArduPilot/pymavlink#568

Open

WickedShell requested changes Jul 8, 2021

View reviewed changes

hendjoshsr71 force-pushed the smart_batt_mavlink branch from ec0a733 to 626c7b2 Compare July 15, 2021 07:39

hendjoshsr71 commented Jul 15, 2021

View reviewed changes

hendjoshsr71 force-pushed the smart_batt_mavlink branch from 626c7b2 to 834b614 Compare August 16, 2021 02:27

hendjoshsr71 force-pushed the smart_batt_mavlink branch 3 times, most recently from 0d0d04a to 4bebd6c Compare September 10, 2021 22:49

hendjoshsr71 force-pushed the smart_batt_mavlink branch from 4bebd6c to 3743643 Compare September 22, 2021 20:04

hendjoshsr71 force-pushed the smart_batt_mavlink branch 2 times, most recently from 2f23d42 to ed01ef5 Compare November 17, 2021 20:07

hendjoshsr71 added 6 commits April 15, 2022 17:31

SITL: SIM_BattMonitor_SMBus add registers to smartbatts

961e26f

AP_BattMonitor: move read_block up to SMBus base class

625a7a6

AP_BattMonitor_SMBus: fix append_zero bufflen

7d2ecfb

GCS_MAVLink: Send SMART_BATTERY_INFO message

995e35e

AP_BattMonitor: add functions for SmartBattery_Info

b32a4f5

Tools: autotset add check for BATI logging

a69ddfa

hendjoshsr71 force-pushed the smart_batt_mavlink branch from ed01ef5 to a69ddfa Compare April 15, 2022 23:44

This was referenced Apr 15, 2022

SITL: SIM_BattMonitor_SMBus add registers to smartbatts #20542

Merged

AP_BattMonitor: SMBus read_block fix append_zero bufflen #20544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Smart_Battery_Info MAVLink message #16167

Add Smart_Battery_Info MAVLink message #16167

hendjoshsr71 commented Dec 30, 2020 •

edited

Loading

amilcarlucas commented Dec 30, 2020

peterbarker left a comment

peterbarker Jan 2, 2021

hendjoshsr71 Jan 3, 2021

peterbarker left a comment

peterbarker commented Jan 5, 2021

hendjoshsr71 commented Jan 5, 2021

hendjoshsr71 commented Jan 5, 2021

amilcarlucas commented Mar 2, 2021

hendjoshsr71 commented Mar 2, 2021

peterbarker left a comment

hendjoshsr71 commented Mar 15, 2021

peterbarker commented Mar 15, 2021 via email

hendjoshsr71 commented Mar 15, 2021

hendjoshsr71 commented Mar 15, 2021

tridge left a comment

tridge commented Jun 29, 2021

hendjoshsr71 left a comment

amilcarlucas commented Jul 6, 2021

WickedShell left a comment

WickedShell Jun 16, 2021

hendjoshsr71 Jul 12, 2021

WickedShell Jun 16, 2021

hendjoshsr71 Jul 12, 2021

WickedShell Jul 8, 2021

hendjoshsr71 Jul 15, 2021

hendjoshsr71 Jul 15, 2021

hendjoshsr71 left a comment

hendjoshsr71 Jul 12, 2021

hendjoshsr71 Jul 12, 2021

hendjoshsr71 Jul 15, 2021

hendjoshsr71 Jul 15, 2021

hendjoshsr71 commented Sep 22, 2021

Add Smart_Battery_Info MAVLink message #16167

Are you sure you want to change the base?

Add Smart_Battery_Info MAVLink message #16167

Conversation

hendjoshsr71 commented Dec 30, 2020 • edited Loading

amilcarlucas commented Dec 30, 2020

peterbarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbarker left a comment

Choose a reason for hiding this comment

peterbarker commented Jan 5, 2021

hendjoshsr71 commented Jan 5, 2021

hendjoshsr71 commented Jan 5, 2021

amilcarlucas commented Mar 2, 2021

hendjoshsr71 commented Mar 2, 2021

peterbarker left a comment

Choose a reason for hiding this comment

hendjoshsr71 commented Mar 15, 2021

peterbarker commented Mar 15, 2021 via email

hendjoshsr71 commented Mar 15, 2021

hendjoshsr71 commented Mar 15, 2021

tridge left a comment

Choose a reason for hiding this comment

tridge commented Jun 29, 2021

hendjoshsr71 left a comment

Choose a reason for hiding this comment

amilcarlucas commented Jul 6, 2021

WickedShell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hendjoshsr71 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hendjoshsr71 commented Sep 22, 2021

hendjoshsr71 commented Dec 30, 2020 •

edited

Loading