-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
UAVCAN Smart Battery Improvements #14198
Conversation
Looks like we need some system wide configuration to properly configure the smart battery scenario. |
I just completed a 25 minute hover test. It ended with the battery cutting out which is all on my side configuring the fuel gauge. https://review.px4.io/plot_app?log=807e8661-99b8-4253-a840-e627ff2c02b0 We also need to think about how to support multiple batteries in parallel with this driver. |
Here is a 25 minute flight on a Pixhawk 4 with this driver. https://review.px4.io/plot_app?log=f0221744-79fc-4473-be30-932dcd7dbc2e |
… into uavcan_smart_battery
21eba11
to
768157e
Compare
@dagar Just got a short flight with two smart batteries. With commander and mavlink already setup to handle multiple batteries, this driver works with multiple uavcan packs well. https://review.px4.io/plot_app?log=e981e85c-466d-4841-9eff-c4094cef5e57 |
@dagar , @dakejahl and I went through the battery parameters last night and found that the new battery parameters are only loaded if the battery_status module is started. For the time being, I un-depricated BAT_SOURCE and I'm using it to not start battery_status. BAT%D_SOURCE for each battery doesn't really make sense either. |
@dagar Any thoughts? |
This moves the handling of the BAT%d_SOURCE param inside of the battery library. Users of the library now pass the source instead of the flag whether to publish. The battery library then checks if the source is selected using the param and publishes accordingly. Since we removed the strange system_source flag, we now need to look at all batteries in commander. For current estimation - I think - it makes sense to sum them up.
Merged in #14490 and resolved conflicts. |
@julianoes Got out and flew this PR with your branch merged in today. Everything is looking good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you have rebased on #14490, that's cool.
@dagar @julianoes what's missing here? can we merge this in? |
Describe problem solved by this pull request
This PR is a continuation of @dakejahl 's work to integrate a UAVCAN smart battery.
Describe your solution
Improved the UAVCAN battery sensor bridge driver. Un-deprecated BAT_SOURCE to use as a parameter to disable battery_status module when a UAVCAN battery is being used.
Test data / coverage
Flight testing has been done with prototype ARK BMS smart batteries with UAVCAN. Flight testing with a single and dual batteries have been done.
Dual Battery Flight Test
Single Battery 25 Minute Flight Test
Additional context
See the BMS Workgroup.