adafruit / Adafruit_CircuitPython_BusDevice

Two helper classes that handle transaction related state for I2C and SPI including locks.
MIT License
108 stars 76 forks source link

Fix I2C init error on BeagleBone Black #22

Closed pdp7 closed 5 years ago

pdp7 commented 6 years ago

This init method verifies that a device exists at the given address.

It was writing a zero byte value to the bus. This triggered a bug in the Linux kernel I2C bus OMAP2 driver for the BeagleBone. The driver returns an invalid write error when msg->len is 0: https://github.com/beagleboard/linux/blob/4.14/drivers/i2c/busses/i2c-omap.c#L665

The solution is to write the value 'x' instead of ''.

Refer to Adafruit_Blinka PR #42 for more information: https://github.com/adafruit/Adafruit_Blinka/pull/42#issuecomment-431948703

tannewt commented 6 years ago

How does Linux do the scan? Does it send an x value? Seems bad to transmit a random byte to an unknown device.

pdp7 commented 6 years ago

@tannewt good point. I'm looking at i2cdetect and it looks like reading a byte might be a better method to determine if there is a device at a given address: https://git.kernel.org/pub/scm/utils/i2c-tools/i2c-tools.git/tree/tools/i2cdetect.c#n101

            /* Probe this address */
            switch (cmd) {
            default: /* MODE_QUICK */
                /* This is known to corrupt the Atmel AT24RF08
                   EEPROM */
                res = i2c_smbus_write_quick(file,
                      I2C_SMBUS_WRITE);
                break;
            case MODE_READ:
                /* This is known to lock SMBus on various
                   write-only chips (mainly clock chips) */
                res = i2c_smbus_read_byte(file);
                break;
            }
pdp7 commented 6 years ago

Alternative approach is to use a read instead of a write.

This looks useful: Adafruit_Blinka/src/adafruit_blinka/microcontroller/raspi_23/i2c.py

    def scan(self):
        """Try to read a byte from each address, if you get an OSError it means the device isnt there"""
        found = []
        for addr in range(0,0x80):
            try:
                self._i2c_bus.read_byte(addr)
            except OSError:
                continue
            found.append(addr)
        return found
pdp7 commented 6 years ago

for __init__ in adafruit_bus_device/i2c_device.py:

   def __init__(self, i2c, device_address):
        """
        Try to read a byte from an address,
        if you get an OSError it means the device is not there
        """
        while not i2c.try_lock():
            pass
        try:
            result = bytearray(2)
            i2c.readfrom_into(device_address, result)
        except OSError:
            raise ValueError("No I2C device at address: %x" % device_address)
        finally:
            i2c.unlock()

        self.i2c = i2c
        self.device_address = device_address

instead of writing a zero byte, try to read a byte from an address. if you get an OSError it means the device is not there

this fixes issue for BealgeBone Black in Adafruit_Blinka: https://github.com/adafruit/Adafruit_Blinka/pull/42

pdp7 commented 6 years ago

dont you want result = bytearray(1)? only one byte read? or does that not work

@ladyada good point. I have change it to read just a single byte and it still works OK.

pdp7 commented 6 years ago

fyi - this is need for https://github.com/adafruit/Adafruit_Blinka/pull/42

pdp7 commented 6 years ago

@ladyada @deshipu do you think this is ok to merge? thanks!

ladyada commented 6 years ago

pardon - gonna test this on m0 and m4 and merge if they both work!

ladyada commented 6 years ago

ok tested with a plethora of sensors and the VEML6075 hates this so turns out you actually need something like....

    def __init__(self, i2c, device_address):
        """
        Try to read a byte from an address,
        if you get an OSError it means the device is not there
        """
        while not i2c.try_lock():
            pass
        try:
            i2c.writeto(device_address, b'')
        except OSError:
            # some OS's dont like writing an empty bytesting...
            # Retry by reading a byte
            try:
                result = bytearray(1)
                i2c.readfrom_into(device_address, result)
            except OSError:  
                raise ValueError("No I2C device at address: %x" % device_address)
        finally:
            i2c.unlock()

        self.i2c = i2c
        self.device_address = device_address

@pdp7 can you try this on BBB? it should work because it will fail at first but succeed on retry. the veml6075 will still be unhappy but maybe its better than nothing?

pdp7 commented 6 years ago

@ladyada thanks for the feedback. I'm currently travelling post-supercon but I will try this out on Beagle once I return home.

ladyada commented 5 years ago

@pdp7 ping!

pdp7 commented 5 years ago

@ladyada thank you for the ping. I will test on BBB this morning

pdp7 commented 5 years ago

@ladyada the example you provided does work OK on the BBB:

diff --git a/adafruit_bus_device/i2c_device.py b/adafruit_bus_device/i2c_device.py
index 1310769..5dea936 100644
--- a/adafruit_bus_device/i2c_device.py
+++ b/adafruit_bus_device/i2c_device.py
@@ -56,6 +56,7 @@ class I2CDevice:
             with device:
                 device.write(bytes_read)
     """
+
     def __init__(self, i2c, device_address):
         """
         Try to read a byte from an address,
@@ -64,10 +65,15 @@ class I2CDevice:
         while not i2c.try_lock():
             pass
         try:
-            result = bytearray(1)
-            i2c.readfrom_into(device_address, result)
+            i2c.writeto(device_address, b'')
         except OSError:
-            raise ValueError("No I2C device at address: %x" % device_address)
+            # some OS's dont like writing an empty bytesting...
+            # Retry by reading a byte
+            try:
+                result = bytearray(1)
+                i2c.readfrom_into(device_address, result)
+            except OSError:
+                raise ValueError("No I2C device at address: %x" % device_address)
         finally:
             i2c.unlock()

I added some debug output and verified that on the BeagleBone the write fails, and the fall back to read works OK:

adafruit_bus_device/i2c_device.py: __init__(): 119
__init__(): i2c.writeto(device_address, b'')
__init__(): OSError
__init__(): OSError: try: result=bytearray(b'\x00')
__init__(): OSError: try: i2c.readfrom_into(device_address, result)
__init__(): OSError: try: result=bytearray(b'\x7f')

Example of BME280 working OK:

debian@beaglebone:~/Adafruit_CircuitPython_BME280$ python3 ./examples/bme280_simpletest.py 

Temperature: 23.4 C
Humidity: 32.5 %
Pressure: 994.5 hPa
Altitude = 157.10 meters
pdp7 commented 5 years ago

I'll add a commit with this change.

ladyada commented 5 years ago

kk!

ladyada commented 5 years ago

@tannewt @dhalbert check it? i think this should resolve the problem for now, we'll need to document the VEML6075 not working but that's for later

ladyada commented 5 years ago

@dhalbert i know its correct but that looks odd to me, i'd like to keep as is if that's ok :)

dhalbert commented 5 years ago

sure!