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

Integrate INA226 with PX4 #12673

Merged
merged 5 commits into from
Dec 28, 2019
Merged

Integrate INA226 with PX4 #12673

merged 5 commits into from
Dec 28, 2019

Conversation

AmeliaEScott
Copy link
Contributor

@AmeliaEScott AmeliaEScott commented Aug 9, 2019

Describe problem solved by the proposed pull request
Support for the INA226 I2C power monitor was added in #11755. This PR changes that driver to publish as a battery_status uORB topic instead of a separate power_monitor topic.

Additional functionality added here:

  • Support for multiple instances of the power module
    • (Tested where all modules have the same I2C address on separate buses, but works in theory if they have different addresses on the same bus)
  • Support for detecting if a battery is plugged in that was not plugged in at boot (See the -f command line flag, as documented in the driver docs)
  • Edited v5x rc.board_sensors to start up driver automatically at boot

Test data / coverage
These tests were conducted on both a Pixhawk 4 (using an adapter board) and on a prototype fmu-v5x board. Results were verified using listener battery_status and by checking the battery status in QGC.

  • 1 INA226 power module: Battery plugged in an unplugged multiple times. Driver started and stopped multiple times, on every available bus and with a variety of I2C addresses.
  • 2 power modules: Both batteries alternately plugged in and unplugged. Each battery had a different cell count, set by BAT(i)_N_CELLS

Also, @dinomani has tested this code in flight, and it has worked as expected.

@hamishwillee
Copy link
Contributor

@ItsTimmy After some advice here from you. We don't have any standalone docs on any power monitors/power modules - though we do have some as part of vehicle build logs and mentioning how to power a vehicle in the flight controller assembly quickstarts.

So:

  1. "do we need to add information about power module/monitor" peripherals as a sub topic of http://docs.px4.io/master/en/peripherals/?

I'm guessing this might cover information like what controllers it can work with, whether it provides both voltage and current, or just voltage. What else it provides. Ratings for systems it can be used with etc etc (advice hoped for!)

  1. Ideally each module and/or driver should have docs, which generate to here: https://dev.px4.io/master/en/middleware/modules_driver.html - Worth doing that for this driver?

@AmeliaEScott
Copy link
Contributor Author

AmeliaEScott commented Aug 12, 2019

@hamishwillee

  1. Yes, I think it is worthwhile to have information about peripherals. For example, the INA226 board I am using can only be plugged into the Pixhawk 4 through another board, which is not immediately obvious without documentation. Also, the specifications (measurement capabilities and maximum ratings) would be good to have written down.

  2. Yes, I think it is worth documenting this driver. I can do that today!

@AmeliaEScott AmeliaEScott force-pushed the pr-i2c-battery branch 4 times, most recently from 65db731 to bc0c416 Compare August 12, 2019 12:45
src/lib/battery/battery.cpp Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Contributor

Just a reminder ^^^ #12673 (comment)

@AmeliaEScott
Copy link
Contributor Author

@hamishwillee Who should I talk to about documenting the capabilities of different hardware?

@AmeliaEScott
Copy link
Contributor Author

Open question for anyone who has an idea: How should this module be started? Right now it must be started manually, with the command ina226 start -b 1 -t 1 pr ina226 start -b 2 -t 2. But on boards where this power module is the default, it should start automatically.

src/drivers/power_monitor/ina226/ina226.cpp Outdated Show resolved Hide resolved
src/drivers/power_monitor/ina226/ina226.cpp Outdated Show resolved Hide resolved
src/drivers/power_monitor/ina226/ina226.h Outdated Show resolved Hide resolved
src/drivers/power_monitor/ina226/ina226.cpp Show resolved Hide resolved
src/drivers/power_monitor/ina226/ina226.cpp Outdated Show resolved Hide resolved
src/drivers/power_monitor/ina226/ina226.cpp Outdated Show resolved Hide resolved
src/drivers/power_monitor/ina226/ina226.cpp Outdated Show resolved Hide resolved
src/drivers/power_monitor/ina226/ina226_main.cpp Outdated Show resolved Hide resolved
src/drivers/power_monitor/ina226/ina226_main.cpp Outdated Show resolved Hide resolved
@AmeliaEScott AmeliaEScott force-pushed the pr-i2c-battery branch 2 times, most recently from 9f849c2 to 7188d47 Compare December 12, 2019 10:13
Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

I think some of the commented out code needs to be removed and then this is good to go in.

@AmeliaEScott AmeliaEScott marked this pull request as ready for review December 19, 2019 08:59
@AmeliaEScott
Copy link
Contributor Author

@julianoes I have removed the commented code. @dinomani has tested it in flight

julianoes
julianoes previously approved these changes Dec 19, 2019
julianoes
julianoes previously approved these changes Dec 19, 2019
julianoes
julianoes previously approved these changes Dec 19, 2019
Copy link
Member

@dagar dagar left a comment

Choose a reason for hiding this comment

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

I still think the overall battery/power architecture needs proper aggregation and configuration, but this is a good step forward for v5x.

@dagar dagar merged commit fc13412 into PX4:master Dec 28, 2019
@AmeliaEScott AmeliaEScott deleted the pr-i2c-battery branch January 24, 2020 10:31
BazookaJoe1900 pushed a commit that referenced this pull request Feb 3, 2020
* Publish I2C battery data as battery_status
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants