pi_ina219 icon indicating copy to clipboard operation
pi_ina219 copied to clipboard

Abstract i2c driver

Open janoskut opened this issue 3 years ago • 3 comments

@chrisb2 here is the suggested abstraction of the I2C drivers.

The PR targets master, though it builds up on my other branch add-type-hints (https://github.com/chrisb2/pi_ina219/pull/29). Not sure how to change this on GitHub.

This change would make your branch convert-to-smbus2 obsolete.

I'm also removing the install_requires from setup.py, because testing packages aren't needed for installation.

To be discussed would be, what the best location for the example/reference implementations of I2C devices (AdafruitI2cDevice, SmbusI2cDevice, and SmbusI2cDevice) could be. For now, I just put them into the same module. But the import statements within the constructors are a bit debatable - not a problem for example code I'd say, but not so nice in the delivered module. Speaking of which - perhaps examples.py wouldn't be such a bad place for those.

janoskut avatar Nov 21 '21 22:11 janoskut

Codecov Report

Merging #30 (4282d43) into master (1f5da6b) will decrease coverage by 7.14%. The diff coverage is 78.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
- Coverage   99.14%   92.00%   -7.15%     
==========================================
  Files           1        1              
  Lines         234      275      +41     
  Branches       18       26       +8     
==========================================
+ Hits          232      253      +21     
- Misses          1       21      +20     
  Partials        1        1              
Impacted Files Coverage Δ
ina219.py 92.00% <78.49%> (-7.15%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1f5da6b...4282d43. Read the comment docs.

codecov-commenter avatar Nov 21 '21 22:11 codecov-commenter

I think this is looking good, may take me a bit of time to review (I am learning a few things in Python from what you have done!).

This will be a breaking change for people who upgrade, but I guess we can just give instructions in Readme on exactly what to do?

chrisb2 avatar Nov 29 '21 06:11 chrisb2

I had a detailed look at this, pretty happy.

Would just like to discuss how we can make this library as easy as possible to use particularly for those who have only minimal experience with Python.

  • imports in constructors - you indicated this is not ideal, can you explain? I did not find anything googling. Seems like not a problem if it makes it easier for newbies?
  • I2C lib dependency - we could add smbus2 to requirements in setup.py and fix example.py to match this, this would mean newbies would get working code out of the box after installing ina219. The readme could then separately describe how to use the other I2C libs. We would provide upgrade instructions in readme for those upgrading from 1.x explaining code change required. Maybe you can think of something else from your Python knowledge?

Chris

chrisb2 avatar Dec 13 '21 06:12 chrisb2