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

SMART_BATTERY_INFO to BATTERY_INFO #22875

Merged
merged 4 commits into from
Mar 19, 2024
Merged

SMART_BATTERY_INFO to BATTERY_INFO #22875

merged 4 commits into from
Mar 19, 2024

Conversation

hamishwillee
Copy link
Contributor

#22724 is failing because SMART_BATTERY_INFO was renamed to BATTERY_INFO and aligned with UAVCAN in https://github.com/mavlink/mavlink/pull/2070/files

This was reasonable as there are no smart batteries, but breaks MAVLink import, because PX4 implements it. This includes the same MAVLink module commit, but also renames SMART_BATTERY_INFO to BATTERY_INFO.

I'm fairly sure I got it right (all builds) but I'm not a software engineer. NOTE, a bunch of these fields aren't implemented in PX4 uorb topic so can't be passed through to MAVLink.

You can see the changes in link above, but at high level:

  • Unchanged fields: id, battery_function, type, cycle_count, weight, cells_in_series, cycle_count`
  • Modified
    • serial_number - char[16] to [32]
    • capacity_full_specification renamed to design_capacity and changed from mAh int32 to float Ah
    • capacity_full renamed to full_charge_capacity and changed from mAh int32 to float Ah
    • device_name renamed to name
    • discharge_minimum_voltage, charging_minimum_voltage, resting_minimum_voltage, charging_maximum_voltage, discharge_maximum_current, discharge_maximum_burst_current - were uint16_t mV, changed to float V and 0 if not supported
    • manufacture_date - char[11] to char[19] - stripped out the / separators
  • New - charging_maximum_current, nominal_voltage
    • Also new is state_of_health which is in the uorb topic but not in old message.

PX4BuildBot and others added 2 commits March 13, 2024 01:02
    - mavlink in PX4/Firmware (497327e): mavlink/mavlink@c4a5c49
    - mavlink current upstream: mavlink/mavlink@a3558d6
    - Changes: mavlink/mavlink@c4a5c49...a3558d6

    a3558d6b 2024-03-07 Hamish Willee - common - DO_FENCE_ENABLE/PARACHUTE fix (#2090)
b9730e0f 2024-03-06 olliw42 - update RADIO_RC_CHANNELS to latest, remove all mlrs from storm32.xml (#1919)
7fed0268 2024-03-06 Patrick José Pereira - common: MAV_CMD_DO_SET_SYS_CMP_ID: Add first version (#2082)
2909b481 2024-03-06 Hamish Willee - Update Pymavlink (#2089)
e9b532a9 2024-03-05 Randy Mackay - common: add set-camera-source command (#2079)
bcdbeb7f 2024-03-01 auturgy - Allow individual fences to be enabled and disabled (#2085)
2f8403d1 2024-02-29 Hamish Willee - MAV_CMD_ODID_SET_EMERGENCY -  (#2086)
daa59c02 2024-02-22 Peter Barker - common.xml: add a command to deal with safety switch (#2081)
977332e2 2024-02-14 Hamish Willee - COMPONENT_INFORMATION_BASIC - add manufacturer date (#2078)
4fef7de2 2024-02-07 Randy Mackay - Common: rename SMART_BATTERY_INFO to BATTERY_INFO and add SOH (#2070)
3865b311 2024-02-01 Hamish Willee - FLIGHT_INFORMATION - description to match PX4 (#2067)
f80e6818 2024-01-31 KonradRudin - development.xml: merge both MAV_CMD enums together (#2074)
Comment on lines +99 to +110
msg.name = 0; // char[50]
msg.weight = 0;
msg.discharge_minimum_voltage = 0;
msg.charging_minimum_voltage = 0;
msg.resting_minimum_voltage = 0;
msg.charging_maximum_voltage = 0;
msg.charging_maximum_current = 0;
msg.discharge_maximum_current = 0;
msg.discharge_maximum_burst_current = 0;
msg.cells_in_series = 0;
msg.nominal_voltage = 0;
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that if the default is 0 you don't need to set it. I've kept here to show what we haven't implemented yet in the MAVLink message.

Comment on lines +77 to +78
msg.design_capacity = (float)(battery_status.capacity * 1000);
msg.full_charge_capacity = (float)(battery_status.state_of_health * battery_status.capacity * 1000.f) / 100.f;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two changed from mAh to Ah, and from int to float.

@@ -67,36 +67,49 @@ class MavlinkStreamSmartBatteryInfo : public MavlinkStream

if (battery_sub.update(&battery_status)) {
if (battery_status.serial_number == 0) {
// This is not smart battery
// Required to emit
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're no longer identifying as "smart batteries". This might not be a reasonable thing to check on because perhaps a battery outputs the other information but not this.

snprintf(msg.serial_number, sizeof(msg.serial_number), "%d", battery_status.serial_number);
}

//msg.device_name = ??
msg.weight = -1;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI this was wrong previously too

@dagar
Copy link
Member

dagar commented Mar 13, 2024

Astyle is angry about whitespace. https://github.com/PX4/PX4-Autopilot/actions/runs/8260343314/job/22595701064?pr=22875

image

Note this stream isn't enabled by default.

Other than that this seems okay, the bigger problem I see is how badly we've overloaded BatteryStatus.msg to try to cover everything from simple ADC voltage to digital power monitors to proper smart batteries. What's actually populated is a mess. I think it really needs to be split probably capturing the different classes of battery at a minimum.

@hamishwillee
Copy link
Contributor Author

@dagar Yes this needs to be done right, but perhaps as a post process so we can get MAVLink auto updating again? There is one failing test so I can't merge this.

What's actually populated is a mess. I think it really needs to be split probably capturing the different classes of battery at a minimum.

Yes. Perhaps split this along the lines of the MAVLink new high and low rate messages: BATTERY_STATUS_V2 and BATTERY_INFO.

  • Note that BATTERY_STATUS_V2 is intended to replace BATTERY_STATUS but my assumption is that we'd stream both for a while?
  • The assumption is that BATTERY_INFO is a low rate message. It should be streamed all the time IFF there is data in it. Or we could explicitly state in the message definition in MAVLink that it must be requested.

Do you want me to take a shot at the uorb split? If I do it the work will be slow.

@hamishwillee
Copy link
Contributor Author

Do we need to worry about these tests?

@julianoes julianoes merged commit 95627ea into main Mar 19, 2024
90 of 92 checks passed
@julianoes julianoes deleted the pr_battery_info_mavlink branch March 19, 2024 22:33
@julianoes
Copy link
Contributor

It's flash overflowing. In my opinion we just need to go to GCC 10 or higher to get some flash back.

Peize-Liu pushed a commit to Peize-Liu/PX4-Autopilot that referenced this pull request Mar 24, 2024
* Update submodule mavlink to latest Wed Mar 13 01:02:16 UTC 2024

    - mavlink in PX4/Firmware (497327e): mavlink/mavlink@c4a5c49
    - mavlink current upstream: mavlink/mavlink@a3558d6
    - Changes: mavlink/mavlink@c4a5c49...a3558d6

    a3558d6b 2024-03-07 Hamish Willee - common - DO_FENCE_ENABLE/PARACHUTE fix (PX4#2090)
b9730e0f 2024-03-06 olliw42 - update RADIO_RC_CHANNELS to latest, remove all mlrs from storm32.xml (PX4#1919)
7fed0268 2024-03-06 Patrick José Pereira - common: MAV_CMD_DO_SET_SYS_CMP_ID: Add first version (PX4#2082)
2909b481 2024-03-06 Hamish Willee - Update Pymavlink (PX4#2089)
e9b532a9 2024-03-05 Randy Mackay - common: add set-camera-source command (PX4#2079)
bcdbeb7f 2024-03-01 auturgy - Allow individual fences to be enabled and disabled (PX4#2085)
2f8403d1 2024-02-29 Hamish Willee - MAV_CMD_ODID_SET_EMERGENCY -  (PX4#2086)
daa59c02 2024-02-22 Peter Barker - common.xml: add a command to deal with safety switch (PX4#2081)
977332e2 2024-02-14 Hamish Willee - COMPONENT_INFORMATION_BASIC - add manufacturer date (PX4#2078)
4fef7de2 2024-02-07 Randy Mackay - Common: rename SMART_BATTERY_INFO to BATTERY_INFO and add SOH (PX4#2070)
3865b311 2024-02-01 Hamish Willee - FLIGHT_INFORMATION - description to match PX4 (PX4#2067)
f80e6818 2024-01-31 KonradRudin - development.xml: merge both MAV_CMD enums together (PX4#2074)

* SMART_BATTERY_INFO to BATTERY_INFO on new mavlink module

* Update src/modules/mavlink/streams/BATTERY_INFO.hpp

* fix trivial whitespace

---------

Co-authored-by: PX4 BuildBot <bot@px4.io>
Co-authored-by: Daniel Agar <daniel@agar.ca>
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

Successfully merging this pull request may close these issues.

None yet

4 participants