Add support for triple-channel INA3221 i2c voltage-and-current sensor #15091
Conversation
|
YAY! Thanks for taking on the task of getting this driver built. I'm gonna have a lot of fun putting this into new stuff. |
peterbarker
force-pushed
the
pr/ina3221
branch
from
ebdb8f3 to
8333687
Compare
peterbarker
force-pushed
the
pr/ina3221
branch
from
8333687 to
68bac54
Compare
peterbarker
force-pushed
the
pr/ina3221
branch
2 times, most recently
from
19ffc57 to
0dca858
Compare
peterbarker
force-pushed
the
pr/ina3221
branch
from
0dca858 to
2df68b8
Compare
peterbarker
force-pushed
the
pr/ina3221
branch
from
2df68b8 to
29ca867
Compare
|
Tested that voltage, current, and accumulation now work properly with real hardware on stampfly. Please feel free to rebase/squash as desired, though I would like credit for both of us to be preserved. The sim needs fixup to use the scaling values from the driver, and possibly modernization? |
|
Okay, this should be ready to go! Tests now do something useful too. |
There was a problem hiding this comment.
Looks good. I have flown the stampfly branch containing these changes with ELRS telem and the reported voltage and current seem reasonable.
I have not gone through the driver in detail (comparison with datasheet etc.). If you'd like that carried out as part of the review pls ltm.
Have a few suggestions:
- increase test timeout so it completes without needing to enable speedup
- add further comments on the hardcoded test values in test and simulated sensor
| tstart = self.get_sim_time() | ||
| while not (seen_1 and seen_3): | ||
| m = self.assert_receive_message('BATTERY_STATUS') | ||
| if self.get_sim_time() - tstart > 10: |
There was a problem hiding this comment.
10s results in a timeout when running at standard speed on a mac M1. With --speedup 2 the test completes. Not sure what the policy is for autotest (do we assume that in CI they'll be run with a speedup?). Increasing to 15 passes.
There was a problem hiding this comment.
This is strange to hear, I chose 10 seconds as a "couldn't possibly fail" value. There might be an inherent test timeout too which would remove the need to check, but I'm not 100% sure? Also get_sim_time should be scaled with speedup.
How are you running the test? Using Tools/autotest/autotest.py build.Sub test.Sub.INA3221 --no-clean and a print statement the total elapsed "time" is well under 1 second on my x86 machine and passes every time. I tested using --speedup=1, --speedup=2, and whatever the autotest script default is.
There was a problem hiding this comment.
Disregard - I was using a --debug build and that explains the timeout.
|
@tpwrules @peterbarker this PR looks good to me, just needs some commits squashing |
* correctly validate channel parameter and improve other parameter access * dynamically enable channels to avoid spending time converting unused channels * implement tracking of reading health * correct reading scaling by using datasheet values * accumulate measured current to track used mAh and Wh * make configurable using #defines (and hwdef) for integrators * correctly separate and lock frontend and backend state. Note that _state of frontend can only be accessed in `read()` method.
* make test actually test something * fix scaling to match datasheet values
* make test actually test something * fix scaling to match datasheet values
This adds support for a new i2c-based voltage/current sensor.
This driver is not currently suitable for merging. It might be that this is more of a component than an e.g. battery monitor in-and-of itself. It might form a base class for a product based around it, perhaps (as several of the smbus-based devices are based around a "generic" driver). I'm waiting on more design details to see where it goes from here.
This has been tested on a
CJMCU-3221breakout board.Of interest here is the i2c simulator for the device - we're still fleshing out how simulated i2c devices work, but the "writable registers" mask is interesting.
This work is being sponsored by Wurzbach Electronics.