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: Remove short-write bitbang from I2C #4378

Merged
merged 1 commit into from
Mar 26, 2021

Conversation

Gadgetoid
Copy link

I came across this while sleuthing for a way to enable I2C pull-ups for Keybow 2040.

The I2C.c for rp2040 included a special case for writes <=2 bytes, claiming that the hardware does not support these.

I'm guessing this was a workaround for an early SDK bug, since I can't find any evidence this is true.

Deleting this code did not adversely affect register reads from an is31l3731 which involve a single byte of data written to set up the address pointer, followed by a read.

I am unsure if there are any other implications to this, or any other vestiges of this codepath, but it was - alongside some small hacks to enable pull-ups - the magic that got my Keybow 2040 working.

@Gadgetoid
Copy link
Author

Okay I've been directed at #4315 where @dhalbert has performed far more extensive testing within CircuitPython than I have.

However I still submit our entire library of C++ Breakout Garden drivers - including working C++ implementations of BME280 - as evidence that 1-byte and 2-byte I2C writes are fine in general. Indeed these are a cornerstone of SMBus so I'd suspect some citation or fairly clear errata if the Pico were broken in such a fundamental way.

Is it some weird edge-case clock stretching or other I2C quirk of these sensors that the hardware has trouble with?

I guess we've got this fun in our future if we keep cranking out drivers, but I'm not yet convinced it's not some quirk of MicroPython/CircuitPython. I have PA1010D and MSA301 on my TODO list though.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[crossed with your second comment] Do not delete this code. it's completely necessary for 0-length writes. See raspberrypi/pico-sdk#238 for a recent query of mine about this.

MicroPython uses bitbang for 0-2 bytes (besides the address). The reason for 1 or 2 I don't know, and have asked, but haven't yet received an answer. I did the same thing in our code. I may reduce it to zero-length bytes in the future.

The internal pullups are not very strong.

@dhalbert
Copy link
Collaborator

If you can figure out why the TCS34725 doesn't work that would be very interesting :). It was my main test for a long time, and eventually I realized it was blocking progress.

@Gadgetoid
Copy link
Author

Thank you for clarifying.

I'm trying to figure out what we've stuck the TCS34725 on, since I recall the sensor name and seem to have written a Python driver for it once upon a time. I'm a bit officeless at the moment, though, so my ability to do any useful sleuthing is somewhat hampered.

I have a PA1010D, an MSA301 and a BME280 from your list- basically working with what we've got Breakout Garden versions of.

From experience we seem to have these Keybow 2040 boards running pretty well with internal pull-ups, I guess strength is relative and on a board that's less controlled than Keybow 2040 (IE: where you might see a 1MHz i2c baudrate for, I dunno, a quirky IR camera 😆 ) they could be an issue.

In the interest of full disclosure- we have populated external pull-ups, it's just they don't work reliably due to production issues and my sleuthing was to see if I could find a happy medium to save a bad batch without making any untoward changes to I2C. The bitbang code was doing its own thing with pull-ups, however, and failing quite spectacularly when the hardware would otherwise work fine. A shift to zero-byte only would work if I could throw my hat in the ring for that to change, but I appreciate that's a huge can of worms.

Would it make sense to adjust the code comments to remove the scary proclamations? I can pivot this PR so we get a useful result out of this.

@dhalbert
Copy link
Collaborator

dhalbert commented Mar 10, 2021

I would expect you to have problems with the "no pullups on SDA or SCL" checking; was that not an issue?

If you are willing to test a number of sensors with if (len == 0), and submit that, I'd be happy to entertain that. I re-tested that, but only on one sensor, and it needs more vetting on several more.

(Also if anyone wants to run MicroPython on the Keybow, I guess they will have the same issue 🙂 ?)

@Gadgetoid
Copy link
Author

I had disabled the check for SDA and SCL pull-ups and fudged in some code to enable the onboard ones, but yes I originally ran into the infamous "check your wiring" error.

I can enable pull-ups for the I2C with an #ifdef and regular Pico SDK commands in the I2C init.

However, in this case bitbangio setup:

    shared_module_bitbangio_i2c_construct(&self->bitbangio_i2c, scl, sda,
                                          frequency, timeout);

Will still fail with:

Traceback (most recent call last):
 File "<stdin>", line 1, in <module>
 File "code.py", line 73, in <module>
TimeoutError: Clock stretch too long
>>>

Even if I set the pins up beforehand, because bitbangio (necessarily) does not respect pin state.

And while the following works for me, I'm apprehensive that it's a little too invasive an ask of upstream to correct what's effectively a manufacturing error:

diff --git a/ports/raspberrypi/boards/pimoroni_keybow2040/mpconfigboard.h b/ports/raspberrypi/boards/pimoroni_keybow2040/mpconfigboard.h
index 234be27c2..494e58517 100644
--- a/ports/raspberrypi/boards/pimoroni_keybow2040/mpconfigboard.h
+++ b/ports/raspberrypi/boards/pimoroni_keybow2040/mpconfigboard.h
@@ -1,6 +1,10 @@
 #define MICROPY_HW_BOARD_NAME "Pimoroni Keybow 2040"
 #define MICROPY_HW_MCU_NAME "rp2040"
 
+#undef CIRCUITPY_REQUIRE_I2C_PULLUPS
+#define CIRCUITPY_REQUIRE_I2C_PULLUPS 0
+#define RP2040_ONBOARD_I2C_PULLUPS
+
 #define MICROPY_HW_SW0 (&pin_GPIO21)
 #define MICROPY_HW_SW1 (&pin_GPIO20)
 #define MICROPY_HW_SW2 (&pin_GPIO19)
diff --git a/ports/raspberrypi/common-hal/busio/I2C.c b/ports/raspberrypi/common-hal/busio/I2C.c
index cb0d73e75..9b122cc9f 100644
--- a/ports/raspberrypi/common-hal/busio/I2C.c
+++ b/ports/raspberrypi/common-hal/busio/I2C.c
@@ -103,8 +103,10 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
     // set up as GPIO by the bitbangio.I2C object.
     //
     // Sets pins to open drain, high, and input.
+#ifndef RP2040_ONBOARD_I2C_PULLUPS
     shared_module_bitbangio_i2c_construct(&self->bitbangio_i2c, scl, sda,
                                           frequency, timeout);
+#endif
 
     self->baudrate = i2c_init(self->peripheral, frequency);
 
@@ -115,6 +117,12 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
 
     gpio_set_function(self->scl_pin, GPIO_FUNC_I2C);
     gpio_set_function(self->sda_pin, GPIO_FUNC_I2C);
+
+#ifdef RP2040_ONBOARD_I2C_PULLUPS
+    gpio_set_pulls(self->scl_pin, true, false);
+    gpio_set_pulls(self->sda_pin, true, false);
+#endif
+
 }
 
 bool common_hal_busio_i2c_deinited(busio_i2c_obj_t *self) {
@@ -158,8 +166,8 @@ 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 <= 2) {
-        // The RP2040 I2C peripheral will not do writes 2 bytes or less long.
+    if (len == 0) {
+        // The RP2040 I2C peripheral will not do 0 byte writes.
         // So use bitbangio.I2C to do the write.
 
         gpio_set_function(self->scl_pin, GPIO_FUNC_SIO);

And, of course, this change to handle only zero byte reads as bitbanged IO would need some fairly comprehensive testing.

@Gadgetoid Gadgetoid force-pushed the patch-remove-rp2040-i2c-bitbang branch from ace3b58 to 3b9e1b8 Compare March 10, 2021 22:14
@Gadgetoid
Copy link
Author

I've switched to 0-bytes only and tweaked the comments a little to better reflect what I think the situation is. This is pending testing which I'll have to do tomorrow.

@Gadgetoid Gadgetoid force-pushed the patch-remove-rp2040-i2c-bitbang branch from 3b9e1b8 to 4ce3999 Compare March 10, 2021 22:17
@dhalbert
Copy link
Collaborator

dhalbert commented Mar 10, 2021

#define RP2040_ONBOARD_I2C_PULLUPS

If you rename this to CIRCUITPY_BUSIO_I2C_INTERNAL_PULLUPS, that would be fine, and you would just turn it on for your board. We can make it complain for ports other than RP2040. The CIRCUITPY_REQUIRE_I2C_PULLUPS was already motivated by someone else's unusual board. Originally it was not switchable.

EDIT: For convenience, we should add something like CIRCUITPY_BUSIO_I2C_INTERNAL_PULLUPS_AVAILABLE, make it default to 0, and make it 1 only in the RP2040 part. It can be tested if CIRCUITPY_BUSIO_I2C_INTERNAL_PULLUPS = 1 and throw an error if they are not available.

@dhalbert dhalbert mentioned this pull request Mar 12, 2021
The I2C.c for RP2040 included a special case for writes <=2 bytes to match the MicroPython implementation,
however RP2040 does support 1 and 2 byte reads, with only 0 bytes being the exception.

Signed-off-by: Philip Howard <phil@pimoroni.com>
@Gadgetoid Gadgetoid force-pushed the patch-remove-rp2040-i2c-bitbang branch from 4ce3999 to 1f6ec28 Compare March 17, 2021 16:30
@Gadgetoid
Copy link
Author

After some amount of testing I've got a part-complete solution, but I'm running into roadblocks with bitbangio.

It, and internal I2C pull-ups seem to be mutually exclusive. If it's initialised as part of the i2c setup then I will inevitably get an error along these lines:

>>> import busio, board
>>> i2c = busio.I2C(board.SCL, board.SDA)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TimeoutError: Clock stretch too long

Afaict this is because common_hal_digitalio_digitalinout_construct sets the pin pull state to PULL_NONE so no matter where or when I enable internal pull-ups, bitbangio will inevitably trample them.

That aside, I've added CIRCUITPY_BUSIO_I2C_INTERNAL_PULLUPS_AVAILABLE but a cursory search over the codebase doesn't suggest a place to:

  1. Define a default value for CIRCUITPY_BUSIO_I2C_INTERNAL_PULLUPS_AVAILABLE
  2. Check the value along with CIRCUITPY_REQUIRE_I2C_PULLUPS

@dhalbert
Copy link
Collaborator

That aside, I've added CIRCUITPY_BUSIO_I2C_INTERNAL_PULLUPS_AVAILABLE but a cursory search over the codebase doesn't suggest a place to:

  1. Define a default value for CIRCUITPY_BUSIO_I2C_INTERNAL_PULLUPS_AVAILABLE
  2. Check the value along with CIRCUITPY_REQUIRE_I2C_PULLUPS

In py/circuitpy_mpconfig.mk add some code like this, alphabetically is possible:

CIRCUITPY_BUSIO_I2C_INTERNAL_PULLUPS_AVAILABLE ?= 0
CFLAGS += -DCIRCUITPY_BUSIO_I2C_INTERNAL_PULLUPS_AVAILABLE=$(CIRCUITPY_BUSIO_I2C_INTERNAL_PULLUPS_AVAILABLE)

That will make the default be 0. Then in the raspberrypi/mpconfigport.mk, you can do:

CIRCUITPY_BUSIO_I2C_INTERNAL_PULLUPS_AVAILABLE ?= 0

which will override the default setting above.

As for checking it along with CIRCUITPY_REQUIRE_I2C_PULLUPS, you can use boolean expressions in #if, or if it's in a .mk file, use nested ifeq()s.

@dhalbert
Copy link
Collaborator

Afaict this is because common_hal_digitalio_digitalinout_construct sets the pin pull state to PULL_NONE so no matter where or when I enable internal pull-ups, bitbangio will inevitably trample them.

Do you feel that that common_hal_digitalio_digitalinout_construct() should perhaps take an extra argument specifying the pullup and/or the initial value? I'm not sure if that would help or not based on your program flow.

@dhalbert
Copy link
Collaborator

Did you finish this up? We can get it in when the defaulting, etc. is all squared away.

@Gadgetoid
Copy link
Author

Getting there, sorry! Usual catastrophe of things competing for my attention.

@Gadgetoid Gadgetoid force-pushed the patch-remove-rp2040-i2c-bitbang branch 2 times, most recently from 390ee79 to b8d4f96 Compare March 25, 2021 22:15
@Gadgetoid
Copy link
Author

I've refined this PR to its original clear purpose: removing the short-write bitbang for 1-2 byte writes, supporting only 0-byte writes.

I have raised a new PR #4488 for the i2c pullup changes.

@dhalbert
Copy link
Collaborator

Thanks! I am going to test this change almost immediately with several sensors, in conjunction with some other I2C testing I am doing.

@dhalbert dhalbert self-requested a review March 25, 2021 22:31
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this on several sensors and an SSD1306 display. All work fine. Thank you!

@dhalbert dhalbert merged commit 155b61f into adafruit:main Mar 26, 2021
@scath999
Copy link

scath999 commented Dec 9, 2021

Facing a similar problem, and found this with the help of @dhalbert.

Not sure how to use this solution with my board. Any guidance is appreciated.

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