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

Updated batt_smbus. #9080

Merged
merged 6 commits into from
Mar 15, 2018
Merged

Updated batt_smbus. #9080

merged 6 commits into from
Mar 15, 2018

Conversation

AlexKlimaj
Copy link
Member

@AlexKlimaj AlexKlimaj commented Mar 14, 2018

Added complementary filter to remaining capacity, reversed warning state checks, and added remaining/discharged out of range checks to handle bad battery calibrations.

Through flight testing, I found that sometimes the fuel gauge would temporarily report a remaining capacity that would cause a Land. I've added a complementary filter to the remaining capacity to filter out any erroneous readings.

Also found a case where the low battery states wouldn't cause a RTL or Land. I've reversed the warning checks to default to Emergency. Also added checks for out of range remaining capacity and discharged amount.

…tery_status.msg. Fixed mavlink_messages.cpp temperature, added commented out expanded battery_status.msg parameters for future mavlink expansion.
…d warning state checks, and added remaining/discharged out of range checks to handle bad battery calibrations
@dagar
Copy link
Member

dagar commented Mar 15, 2018

Wouldn't you already get this by using Battery (https://github.com/PX4/Firmware/blob/master/src/modules/systemlib/battery.h) like other parts of the system?

I think all of this needs to be consolidated and pushed into Battery (or similar) before it becomes unmaintainable. We shouldn't have different parts of the system independently reading and interpreting battery failsafe thresholds parameters.

Isn't the sensors module simultaneously publishing an empty battery_status (probably the first instance)?

@AlexKlimaj
Copy link
Member Author

The default battery driver is built around reading the battery voltage through the ADC. It gets bypassed in sensors.cpp if batter_source isn't set to 0.

I would rather keep it separate from the battery driver and use BAT_SOURCE to select which one to use. The majority of users don't use a smart battery and I'd worry about breaking the default battery driver.

@dagar
Copy link
Member

dagar commented Mar 15, 2018

The ADC portion is handled in sensors, I'm talking about the Battery class (https://github.com/PX4/Firmware/blob/master/src/modules/systemlib/battery.h) that brings in the threshold parameters (BAT_CRIT_THR, BAT_LOW_THR, BAT_EMERGEN_THR), filters, remaining calculation, etc.

We need some level of consistency between these things, otherwise the system becomes fragmented and unmaintainable (batt_smbus itself can still remain separate). There needs to be an interface or common class like Battery defined for what a power module should even do. There's still a large overlap between a smart battery and analog input.

This PR looks fine to merge right now, but if we don't find a better way to consolidate these common things (different types of batteries) with well defined interfaces it's very likely going to break again in the future as things evolve with limited test coverage.

@dagar dagar merged commit 08a53a9 into PX4:master Mar 15, 2018
@AlexKlimaj
Copy link
Member Author

@dagar Thanks for accepting the PR.

I originally used the Battery.h class when I wrote the code. You commented about it a few weeks ago. The problem is that you need to subscribe to actuator controls to update the battery class.

#8854 (comment)

@dagar
Copy link
Member

dagar commented Mar 15, 2018

Yes, so if actuator_controls is a requirement for battery estimation it should be pushed into the Battery class. All of these different battery device types (analog, smbus, crazyflie, mavlink) would then get the same behaviour. There are also weird cases, like a quadplane where the throttle is going to be wrong after transition to forward flight. Details like that need to be captured and handled centrally.

This pull request was closed.
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.

2 participants