-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
Updated batt_smbus. #9080
Conversation
…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
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)? |
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. |
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 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. |
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. |
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.