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

RP2040: Implement short I2C writes (2 bytes or less) using bitbangio #4315

Merged
merged 5 commits into from
Mar 3, 2021

Conversation

dhalbert
Copy link
Collaborator

@dhalbert dhalbert commented Mar 3, 2021

The RP2040 I2C hardware does not do writes of 2 bytes or less. They must be done via bitbang.

  • Handle short writes with bitbangio.I2C. A single bitbangio.I2C object is created once, not on the heap, for use as needed.
  • Include a 1-second timeout on read/write bus transactions.
  • (Style cleanup) Split shared-module/bitbangio/types.h into separate files: I2C.h, SPI.h, and OneWire.h. This makes it consistent with busio and similar modules.

Tested and works with the following sensors:

  • LIS3DH
  • LIS3DML
  • SHT31D
  • BME280
  • VCNL4040
  • TSL2561
  • MSA301

Does not work with:

  • TCS34725
  • PA1010D (mini GPS)

These do work with bitbangio.I2C, though it may be necessary to do bitbangio.I2C(..., probe=False). See #4066.

@dhalbert dhalbert requested a review from tannewt March 3, 2021 04:37
@dhalbert
Copy link
Collaborator Author

dhalbert commented Mar 3, 2021

The failed builds are all xtensa-build, unrelated to this PR. They are not even consistent with the other xtensa builds. I will try a re-run.

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.

Thank you!

@dhalbert
Copy link
Collaborator Author

dhalbert commented Mar 3, 2021

Merging anyway, because failed checks are irrelevant.

@dhalbert dhalbert merged commit cd48c5e into adafruit:main Mar 3, 2021
@dhalbert dhalbert deleted the rp2040-i2c-short-writes branch March 3, 2021 17:42
@Gadgetoid
Copy link

Not got as much testing done as I'd like.

Using the raspberry_pi_pico port compiled with this change:

  • msa301_tap_example
  • msa301_simpletest
  • bme280_normal_mode
  • bme280_simpletest

PA1010D remains a mystery, it seems to perform a few writes and then lock up, attempting to init I2C again (even after a soft reset of the RP2) will fail with a pull-up error.

However, to add pure unadulterated mystery this problem...running i2c.scan() between reads will unlock it. Similarly running a zero-byte write will also kick it back into action.

Here's the output of my tinkering with i2c.scan():

# Make it fail
>>> i2c.readfrom_into(0x10, inbuf)
>>> i2c.readfrom_into(0x10, inbuf)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 116] ETIMEDOUT
>>> i2c.readfrom_into(0x10, inbuf)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 116] ETIMEDOUT
# Unblock the plumbing with i2c.scan()
>>> i2c.scan()
[8, 16, 38, 118]
# Read
>>> i2c.readfrom_into(0x10, inbuf)
# Looks legit
>>> inbuf
bytearray(b'00,0.00,06\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n')
# Fails agan...
>>> i2c.readfrom_into(0x10, inbuf)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 116] ETIMEDOUT
# Unblock again...
>>> i2c.scan()
[8, 16, 38, 118]
# Read succeeds now
>>> i2c.readfrom_into(0x10, inbuf)
# Looks like NMEA sentences
>>> inbuf
bytearray(b'8.869,V,,,,,0.00,0.00,060180,,,N*57\r\n$GNGGA,000739.869,,,,,0')
# We get two reads this time...
>>> i2c.readfrom_into(0x10, inbuf)
>>> inbuf
bytearray(b',0,,,M,,M,,*5C\r\n$GNRMC,000739.869,V,,,,,0.00,0.00,060180,,,N')
# Third time is *not* a charm
>>> i2c.readfrom_into(0x10, inbuf)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 116] ETIMEDOUT
# Definitely broken
>>> i2c.readfrom_into(0x10, inbuf)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 116] ETIMEDOUT
# Unblock with i2c scan again
>>> i2c.scan()
[8, 16, 38, 118]
# Works again
>>> i2c.readfrom_into(0x10, inbuf)
# More valid NMEA
>>> inbuf
bytearray(b'56\r\n$GNGGA,000740.869,,,,,0,0,,,M,,M,,*52\r\n$GNRMC,000740.869')
>>> 

Here's the output of my tinkering with zero byte reads:

# Try to read
>>> i2c.readfrom_into(0x10, inbuf)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
# Read fails
OSError: [Errno 116] ETIMEDOUT
# Hit it with some zero byte writes
>>> i2c.writeto(0x10, bytearray())
>>> i2c.writeto(0x10, bytearray())
>>> i2c.writeto(0x10, bytearray())
# Try to read again
>>> i2c.readfrom_into(0x10, inbuf)
# Read succeeds (though 0xff suggests not usefully)
>>> inbuf
bytearray(b'\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff')
# Read again
>>> i2c.readfrom_into(0x10, inbuf)
# Now we have something resembling data
>>> inbuf
bytearray(b',,N*5\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n')
# Read again
>>> i2c.readfrom_into(0x10, inbuf)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
# Broken again...
OSError: [Errno 116] ETIMEDOUT
>>> i2c.readfrom_into(0x10, inbuf)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 116] ETIMEDOUT
# More zero byte writes
>>> i2c.writeto(0x10, bytearray())
>>> i2c.writeto(0x10, bytearray())
>>> i2c.writeto(0x10, bytearray())
# Read
>>> i2c.readfrom_into(0x10, inbuf)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
# No bueno!
OSError: [Errno 116] ETIMEDOUT
# Try again?
>>> i2c.readfrom_into(0x10, inbuf)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 116] ETIMEDOUT
# One more zero byte write for luck
>>> i2c.writeto(0x10, bytearray())
# That worked!?!?
>>> i2c.readfrom_into(0x10, inbuf)
>>> inbuf
bytearray(b'F\r\n$GNGGA,000343.868,,,,,0,0,,,M,,M,,*54\r\n$GNRMC,000343.868,')
>>>

This is... weird. I clearly need to get my hands on a TCS34725

@dhalbert
Copy link
Collaborator Author

Thanks for this testing. There are few devices that don't behave well, and we just have to document them for now. But your .scan() results are very interesting!

@Gadgetoid
Copy link

Gadgetoid commented Mar 12, 2021

[sorry for the churn - I mistakenly edited your reply instead of quote-replying and had to clean that up]

You're not going to like this, but I replaced the whole I2C read implementation with bitbangio and now reads from my PA1010D are rock solid.Specifically on top of this patch I did (ignoring the change to writes, since I was using only readfrom_into):

diff --git a/ports/raspberrypi/common-hal/busio/I2C.c b/ports/raspberrypi/common-hal/busio/I2C.c
index f5516e57b..1bb871b6c 100644
--- a/ports/raspberrypi/common-hal/busio/I2C.c
+++ b/ports/raspberrypi/common-hal/busio/I2C.c
@@ -163,7 +163,7 @@ void common_hal_busio_i2c_unlock(busio_i2c_obj_t *self) {
 
 uint8_t common_hal_busio_i2c_write(busio_i2c_obj_t *self, uint16_t addr,
                                    const uint8_t *data, size_t len, bool transmit_stop_bit) {
-    if (len == 0) {
+    if (len <= 2) {
         // The RP2040 I2C peripheral will not perform 0 byte writes.
         // So use bitbangio.I2C to do the write.
 
@@ -201,6 +201,25 @@ uint8_t common_hal_busio_i2c_write(busio_i2c_obj_t *self, uint16_t addr,
 
 uint8_t common_hal_busio_i2c_read(busio_i2c_obj_t *self, uint16_t addr,
         uint8_t *data, size_t len) {
+
+        gpio_set_function(self->scl_pin, GPIO_FUNC_SIO);
+        gpio_set_function(self->sda_pin, GPIO_FUNC_SIO);
+        gpio_set_dir(self->scl_pin, GPIO_IN);
+        gpio_set_dir(self->sda_pin, GPIO_IN);
+        gpio_put(self->scl_pin, false);
+        gpio_put(self->sda_pin, false);
+
+        uint8_t status = shared_module_bitbangio_i2c_read(&self->bitbangio_i2c,
+                                                           addr, data, len);
+
+        // The pins must be set back to GPIO_FUNC_I2C in the order given here,
+        // SCL first, otherwise reads will hang.
+        gpio_set_function(self->scl_pin, GPIO_FUNC_I2C);
+        gpio_set_function(self->sda_pin, GPIO_FUNC_I2C);
+
+        return status;
+ 
+
     int result = i2c_read_timeout_us(self->peripheral, addr, data, len, false, BUS_TIMEOUT_US);
     if (result == len) {
         return 0;

@dhalbert
Copy link
Collaborator Author

You're not going to like this, but I replaced the whole I2C read implementation with bitbangio and now reads from my PA1010D are rock solid.

That it works with bitbangio is expected, and that's the workaround for devices that don't work. Just use bitbangio.I2C() instead of busio.I2C() for such devices. We don't want to throw out the hardware I2C completely because it's more efficient, etc. It appears to be a hardware or SDK peculiarity, but I need to test the non-working devices with the Pico C SDK and MicroPython to make sure. However, what we're doing is not that complicated, so it might be hardware, or some tricky hardware setting.

@Gadgetoid
Copy link

Gadgetoid commented Mar 12, 2021

Had to freeze outside to get the teeny GPS to fix, but with the above (albeit very nuclear) patch:

========================================
Fix timestamp: 3/12/2021 16:45:03
Latitude: 52.XXXXXX degrees
Longitude: 0.XXXXXX degrees
Fix quality: 1
# satellites: 5
Altitude: 15.3 meters
Speed: 0.46 knots
Track angle: 0.0 degrees
Horizontal dilution: 1.86
Height geo ID: 47.0 meters
========================================
Fix timestamp: 3/12/2021 16:45:04
Latitude: 52.XXXXXX degrees
Longitude: 0.XXXXXX degrees
Fix quality: 1
# satellites: 5
Altitude: 15.4 meters
Speed: 0.28 knots
Track angle: 0.0 degrees
Horizontal dilution: 1.87
Height geo ID: 47.0 meters
========================================
Fix timestamp: 3/12/2021 16:45:06
Latitude: 52.XXXXXX degrees
Longitude: 0.XXXXXX degrees
Fix quality: 1
# satellites: 5
Altitude: 15.7 meters
Speed: 0.42 knots
Track angle: 0.0 degrees
Horizontal dilution: 1.86
Height geo ID: 47.0 meters
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "gps_simpletest.py", line 62, in <module>
  File "adafruit_gps.py", line 245, in update
  File "adafruit_gps.py", line 243, in update
  File "adafruit_gps.py", line 374, in _parse_sentence
  File "adafruit_gps.py", line 349, in _read_sentence
  File "adafruit_gps.py", line 692, in readline
  File "adafruit_gps.py", line 669, in read
  File "adafruit_gps.py", line 664, in read
KeyboardInterrupt: 
>>> 

So at least we've... somewhat narrowed the issue down.

EDIT: in response to the above, since I need to write a PA1010D C++ driver for Pico at some point, I might as well investigate that now since our interests align and it might narrow the scope to an SDK issue if I encounter the same problems.

@dhalbert
Copy link
Collaborator Author

dhalbert commented Mar 12, 2021

I would be grateful if you have time to test the PA1010D with MicroPython or the Pico SDK.

@dhalbert
Copy link
Collaborator Author

Limor has mentioned to me that the "serial" implementation via I2C for the PA1010D is pretty odd. As for TCS34725, it appears you resell our sensor boards, but I don't see a product of your own.

@Gadgetoid
Copy link

Gadgetoid commented Mar 12, 2021

Yeah PA1010D is an odd duck, I think I ended up referring to some Adafruit code to discover that it emits spurious \r or \n or something to that effect, totally breaking sentences if you don't treat only the combination of both as a valid line ending.

In a mostly SMBus world, these low-level i2c implementations always feel weird.

PA1010D results.

Hardware I2C fails miserably:

ret = i2c_read_blocking(i2c0, addr, rxdata, 10, false);

PIO "bitbang" works fine:

ret = pio_i2c_read_blocking(pio, sm, addr, rxdata, 10);

I don't think I'm going to figure out why without a scope or a logic analyser, but at least we can replicate the problem in a couple dozen lines of C.

@Gadgetoid
Copy link

Totally confused myself by posting on this PR instead of the open one 😶

But I’ve created a C++ POC for the problem with PA1010D (plus a comparison using the PIO that works) and raised an issue against the SDK here- raspberrypi/pico-sdk#252

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.

3 participants