Updated batt_smbus. Expanded battery_status.msg. Fixed mavlink_messages.cpp temperature. #8991
Conversation
…tery_status.msg. Fixed mavlink_messages.cpp temperature, added commented out expanded battery_status.msg parameters for future mavlink expansion.
| } | ||
|
|
||
| const char *verb = argv[optind]; | ||
|
|
||
| if (!strcmp(verb, "start")) { | ||
| if (g_batt_smbus != nullptr) { | ||
| PX4_ERR("already started"); | ||
| return 1; | ||
| errx(1, "already started"); |
There was a problem hiding this comment.
Can you remove anything that actually exits? For example replace errx with PX4_ERR and a return. Then this driver will be usable out of the box on linux where an exit actually kills the px4 process.
| @@ -573,7 +573,13 @@ class MavlinkStreamSysStatus : public MavlinkStream | |||
| bat_msg.energy_consumed = -1; | |||
| bat_msg.current_battery = (battery_status.connected) ? battery_status.current_filtered_a * 100 : -1; | |||
| bat_msg.battery_remaining = (battery_status.connected) ? ceilf(battery_status.remaining * 100.0f) : -1; | |||
| bat_msg.temperature = INT16_MAX; | |||
| bat_msg.temperature = (battery_status.connected) ? (int16_t)battery_status.temperature : INT16_MAX; | |||
| //bat_msg.average_current_battery = (battery_status.connected) ? battery_status.average_current_a * 100.0f : -1; | |||
There was a problem hiding this comment.
It needs an updated version of Mavlink with these messages to build correctly. I put them there for the future.
|
@LorenzMeier do you have anything else we could test this with? Should we make sure it fully works on a Solo? |
|
Same question as in the last PR: Why did you remove functionality to read some values from SMBus? This PR is not an update, it is a change to one particular power management part and I'd like to understand the rationale. |
|
I removed functions that don't make much sense for a smart battery. The values being read also weren't being used or passed anywhere. Also, the code didn't build as of a few weeks ago, so it's safe to say nobody has been using it recently.
|
|
Hi, I've reviewed this work with Alex and we've tried diligently to generalize the (heavily Solo biased) nature of the driver to serve a broader audience. Although the Solo certainly has a history, it doesn't seem to have much of a future... This work is primarily disposed towards a general (more broad) usage of smart battery fuel gauges that we hope to be more of the norm as time progresses. Although this PR might fit the Solo battery less well, we hope that it will fit other smart batteries better. Perhaps this is the best rationale to move ahead with this (type) of work? Thanks for your review and consideration. We hope to broaden and generalize this driver for future systems with this work. -Mark |
|
That makes sense. I just wanted to be sure the code matches the considerations and intentions, which it does. |
|
Thanks @LorenzMeier and @dagar for your review (and also acceptance) of this code! We've put another ~1.5hrs / 8 flight packs through flight testing today as well as the full day of bench evaluation on ~10 fuel gauge equipped packs with today's Firmware/master branch... flawless results so far. It is very fun to breathe new life into the smbus code! Thanks again for your careful consideration. -Mark |
Updated and expanded batt_smbus to work with bq40z50-R2. Expanded battery_status.msg. Fixed mavlink_messages.cpp temperature, added commented out expanded battery_status.msg parameters for future mavlink expansion.
I've been flying this code with our smart battery for a couple of weeks now without issue.
Planning future enhancements to fall back on ADC battery driver if communication with the smart battery fails for some reason.
Batt_source needs to be set to 1 to bypass battery.cpp ADC driver.
QGC Mavlink Inspector
QGC Battery Status
Nut Shell Output: