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

Possible fix for #11 #12

Merged
merged 3 commits into from Aug 6, 2019
Merged

Possible fix for #11 #12

merged 3 commits into from Aug 6, 2019

Conversation

caternuson
Copy link
Contributor

Can't just add logic in existing function since it would still hasattr. Need to dynamically provide it as needed.

Don't have an INA219 to test with original issue, but tested with an I2C FRAM and TCA:
tca_fram_test.py

import board
import adafruit_fram
import adafruit_tca9548a

tca = adafruit_tca9548a.TCA9548A(board.I2C())

fram = adafruit_fram.FRAM_I2C(tca[0])

fram[0] = 1
print(fram[0])

RPI/Blinka

pi@pizerow:~ $ python3
Python 3.7.3 (default, Apr  3 2019, 05:39:12) 
[GCC 8.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import tca_fram_test
bytearray(b'\x01')
>>>

ItsyM4/CP

Press any key to enter the REPL. Use CTRL-D to reload.
Adafruit CircuitPython 4.0.2 on 2019-06-27; Adafruit ItsyBitsy M4 Express with samd51g19
>>> import tca_fram_test
bytearray(b'\x01')
>>>

@ladyada
Copy link
Member

ladyada commented Jul 25, 2019

i thought we added it to blinka?

@ladyada
Copy link
Member

ladyada commented Jul 25, 2019

oh right its on blinka (required) but not busio.

@caternuson
Copy link
Contributor Author

Yep. And that all worked. New issue is that it can then fail on native CP since I2CDevice will try to call the linux func.

Copy link
Member

@tannewt tannewt 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 we always want writeto_then_readfrom available so that drivers are written for it going forwards. (Otherwise they won't work with Blinka.)

So, on this PR detect in writeto_then_readfrom or in the constructor whether it is available and provide a substitute. Bonus points for adding writeto_then_readfrom into CircuitPython too. Thanks!

@caternuson
Copy link
Contributor Author

Don't most drivers use I2CDevice and call write_then_readinto? It's then I2CDevice that turns that into platform specific calls. With the current logic, the simple existence of writeto_then_readfrom is seen as being linux:
https://github.com/adafruit/Adafruit_CircuitPython_BusDevice/blob/master/adafruit_bus_device/i2c_device.py#L145
Without changing that logic, this class needs to behave dynamically. So it's checking with hasattr and providing with setattr.

And in the future, when writeto_then_readfrom gets added to busio.I2C, it would still work.

@tannewt
Copy link
Member

tannewt commented Jul 26, 2019

Yes, most do but this driver shouldn't assume I2CDevice is used (just like busio.I2C shouldn't.) It should always provide writeto_then_readfrom even if the underlying I2C bus doesn't. As is, it only adds it when the underlying implementation has it. Doing it only sometimes risks people writing code that won't work on Blinka.

I created a PR to remove stop= from CircuitPython in favor of writeto_then_readfrom here: adafruit/circuitpython#2012

@caternuson
Copy link
Contributor Author

Is this PR even needed then? If CP's busio.I2C provides writeto_then_readfrom, then the existing version will just start working.

@tannewt
Copy link
Member

tannewt commented Jul 30, 2019

It'll be needed for older versions. Want me to wrap it up?

@caternuson
Copy link
Contributor Author

Sure. Works for me.

@tannewt tannewt self-assigned this Aug 2, 2019
@tannewt
Copy link
Member

tannewt commented Aug 6, 2019

@caternuson I've commited to your branch with what I think will work. Please let me know what you think. Thanks!

@caternuson
Copy link
Contributor Author

Ah, OK. I think I was trying to avoid duplicating the logic from I2CDevice and just drive the behavior from here.

Not sure what it would be - but can you think of any issues in the future if the code in I2CDevice.writeto_then_readfrom changes and then code here is not updated to match?

@tannewt
Copy link
Member

tannewt commented Aug 6, 2019

Ah, I think it's ok to duplicated it since it's temporary. There is no guarantee the this is used with I2CDevice so it's better to have this updated now.

I can't think of any issues but we can always fix things if it comes up.

@caternuson
Copy link
Contributor Author

OK, cool with me. I just took care of the minor lints.

@tannewt
Copy link
Member

tannewt commented Aug 6, 2019

Thanks! I totally missed that it had issues.

@tannewt tannewt merged commit c5617b7 into adafruit:master Aug 6, 2019
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Aug 7, 2019
Updating https://github.com/adafruit/Adafruit_CircuitPython_HX8357 to 1.1.0 from 1.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_HX8357#4 from makermelissa/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_TCA9548A to 0.2.1 from 0.2.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_TCA9548A#12 from caternuson/iss11
  > Merge pull request adafruit/Adafruit_CircuitPython_TCA9548A#10 from caternuson/iss9

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Shapes to 1.1.2 from 1.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Shapes#6 from makermelissa/master
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Shapes#5 from makermelissa/master
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Shapes#4 from makermelissa/master
  > fixed typo in rtd link
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants