Skip to content

machine_i2c: Add I2C bus clear support. #13281

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PepperoniPingu
Copy link

I have some software that is using I2C on the rp2040. Over a couple of months of continuous running there have been some failed transactions resulting in exceptions. Since these are rare, and, the i2c waveform is not too pretty when looked at with an oscilloscope, I presume my hardware implementation is at fault. Nevertheless, since this system is running 24/7 in a critical environment it is essential to be able to recover from these crashes. The proper way to do this is to send an I2C bus clear sequence (a.k.a. software reset) and retry. To do the bus clearing you simply send 10 negative pulses on SCL. This clocks out potential stuck data in the output registers of peripherals.

This PR implements:

  • API backend support for sending an I2C bus clear sequence
    • accessible from python by i2c.clear_bus()
  • said function in the:
    • soft I2C driver
    • rp2 I2C driver

Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:  +136 +0.035% PYBV10
     mimxrt:   +76 +0.021% TEENSY40
        rp2:  +264 +0.079% RPI_PICO
       samd:  +128 +0.049% ADAFRUIT_ITSYBITSY_M4_EXPRESS

This commit adds API support for implementing the sending of an I2C bus
clear sequence, and, implements said function in the soft I2C driver.

Signed-off-by: Hugo Frisk <hugopaf@outlook.com>
Since the rp2040 hardware I2C block does not support sending a bus clear
sequence this is accomplished by using the soft I2C implementation of bus
clearing.

Signed-off-by: Hugo Frisk <hugopaf@outlook.com>
@robert-hh
Copy link
Contributor

You could to that with the machine.Pin class without changing the I2C driver.

Copy link

codecov bot commented Dec 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9feb068) 98.40% compared to head (418653a) 98.40%.
Report is 15 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #13281   +/-   ##
=======================================
  Coverage   98.40%   98.40%           
=======================================
  Files         159      159           
  Lines       21088    21088           
=======================================
  Hits        20752    20752           
  Misses        336      336           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PepperoniPingu
Copy link
Author

That is true. But it would fragment the driver code, and, it'd be nice to have it standardized. Furthermore, many I2C hardware blocks (unfortunately not the rp2) have this function built in. So bit banging the bus clear on those adds unnecessary init and deinit delays.

If the code size is the problem we could perhaps add a define flag?

@robert-hh
Copy link
Contributor

For the given PR I would see it more as a problem, that it's just added for SoftI2C and the RP2 port. After I2C was harmonized across all ports, port-specific changes should be avoided.
Did you consider to extend the machine.I2C class with Python code by creating a I2C class derived from machine.I2C? Like the skeleton below.

import machine

class I2C(machine.I2C):
    def __init__(self, *args, **kwargs):
        # get & store the scl pin from kwargs here

    def clear_bus(self):
        #  Add the code to clear the bus.
        return

@PepperoniPingu
Copy link
Author

Thank you for the suggestion! That will have to do for now.

Do you see any other problems with this solution? I.e. if one were to implement this feature on all ports would it be merged?

@robert-hh
Copy link
Contributor

I cannot tell whether such a change would be accepted. And it is quite some work to implement it for all ports.

@dpgeorge dpgeorge added the extmod Relates to extmod/ directory in source label Jan 5, 2024
@projectgus
Copy link
Contributor

projectgus commented Jan 23, 2024

@PepperoniPingu Thanks for explaining the use case for this and providing this implementation.

It's 9 clock cycles, not 10, isn't it? As per I2C specification NXP Semiconductors UM10204 section 3.1.16 Bus clear.

The main concern for adding this in the driver is implementing it across all ports, and the code size increase. 100-200 bytes doesn't seem like a lot, but these increases can add up!

If arbitrary clock sequences succeed in clearing the bus, is it possible to clear the bus by writing to an address you know isn't on the bus, and ignoring the result? Something like i2c.writeto(0x7F, b'', stop=False) or similar? This should be 9 clock cycles (7 address bits, R/W bit, ACK bit) .

@PepperoniPingu
Copy link
Author

Hi!

I ran in to the same question as you did with 9 vs 10 clock cycles. I tried with 9 cycles first but still had problems which was solved by changing it to 10. More cycles can't hurt so I went with it. But when I looked into this now I found this article which says 16 cycles are needed? Don't know what's up with that.

A simple i2c.writeto(0x7F, bytes([0xFF])) should suffice which is what I decided on in the end. Refraining from sending the stop bit is probably not great since the peripherals state machines could end up thinking the bus is busy with other transactions.

The address 0x7F is by the i2c specification reserved so there could be some undefined behavior with some peripherals? I also tried with the address 0x00 but that ended up rewriting the address of some of the peripherals as is defined by the spec. Did not enjoy trying to reverse that :)

A perfect bus clear in my world would be to first clock scl 10 times, then send a start followed by a stop condition to reset the state machines.

@dpgeorge
Copy link
Member

Furthermore, many I2C hardware blocks (unfortunately not the rp2) have this function built in

Which MCUs do you know of that have I2C clear built-in as a hardware feature? How do they implement it (how many clocks, do they send start/stop conditions)?

@projectgus
Copy link
Contributor

The address 0x7F is by the i2c specification reserved so there could be some undefined behavior with some peripherals? I also tried with the address 0x00 but that ended up rewriting the address of some of the peripherals as is defined by the spec. Did not enjoy trying to reverse that :)

Ouch, that's unfortunate! 😵 There is some reasoning behind choosing address 0x7F, though:

Recall that I2C pins are all open drain (driver pulls low only), so a logical 1 bit in the address means the SDA pin is doing nothing while SCL transitions. Sending 0x7F as the address is the same (during the address phase) as pulsing the SCL pin by itself without touching SDA. (This is, presumably, is why this address is reserved in the spec.)

There is one small difference between sending this fake I2C transfer (without a STOP) versus only clocking the SCL pin, which is that there is a single "START" transition of the SDA pin from high to low mid-clock. However, as I understand it, the problem that the bus clear is trying to resolve is that another device is already pulling the SDA pin down already - so I think it's reasonable to expect that first "START" transition won't appear on the bus at all, as SDA is already held low by the device at this time.

A simple i2c.writeto(0x7F, bytes([0xFF])) should suffice which is what I decided on in the end.

Does that mean that this works as an option for resolving the bus issue you were seeing, at least?

A perfect bus clear in my world would be to first clock scl 10 times, then send a start followed by a stop condition to reset the state machines.

Would something along these lines emulate this?

i2c.writeto(0x7F, bytes([0xFF], stop=False)
i2c.writeto(0x7F, b'')

It may seem unnecessary to be looking hard at these possible workarounds when you've supplied working bus reset code already. The reason is entirely to do with saving binary size if possible (and, to a lesser extent, having to implement bus clear across all the different MCU controllers.)

@PepperoniPingu
Copy link
Author

Which MCUs do you know of that have I2C clear built-in as a hardware feature? How do they implement it (how many clocks, do they send start/stop conditions)?

Not quite sure. I though there were a bunch but couldn't really find any concrete examples now, sorry for misleading. The rp2040s datasheet says that it sends out 9 clock cycles and than a stop. Though as previously established this is not implemented.

However, as I understand it, the problem that the bus clear is trying to resolve is that another device is already pulling the SDA pin down already - so I think it's reasonable to expect that first "START" transition won't appear on the bus at all, as SDA is already held low by the device at this time.

It doesn't have to be that way. The peripheral could be stuck "sending" HIGH but when a new transaction clocks to the next bit in the output register the peripheral pulls down SDA. So the fault can be hidden until you start clocking data, or even a couple of clocks in.

Would something along these lines emulate this?

i2c.writeto(0x7F, bytes([0xFF], stop=False)
i2c.writeto(0x7F, b'')

I don't believe it's any better to split it up in to two commands. Works fine with one.

Does that mean that this works as an option for resolving the bus issue you were seeing, at least?

Yep, using timeouts, retries and these dummy I2C sends seems to have solved the issue. Thanks for the help!

For future reference, here are some outtakes from my new I2C wrapper class:

def writeTo(self, address, byte):
        import time
        error = None
        for i in range(self._retries + 1):
            try:
                self._i2c.writeto(address, byte)
                break
            except OSError as _error:
                error = _error
                self._clear_bus()
                if error.errno != 110: # 110 = ETIMEDOUT, unnecessary to wait two times
                    time.sleep_us(self._us_between_retries)
        if error is not None:
            raise error
        return

def _clear_bus(self):
        try:
            self._i2c.writeto(0x7F, bytes([0xFF]))
        except OSError as err:
            if err.errno != 5 and err.errno != 19: # 5 = EIO, 19 = ENODEV - the two error codes that can occur on nack
                raise err

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extmod Relates to extmod/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants