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

I2C: Considering adding I2C clock stretching support #141

Closed
r12f opened this issue Dec 28, 2022 · 16 comments
Closed

I2C: Considering adding I2C clock stretching support #141

r12f opened this issue Dec 28, 2022 · 16 comments
Milestone

Comments

@r12f
Copy link
Contributor

r12f commented Dec 28, 2022

Hi Team, I recently ordered a hydrabus for my hobby project and trying to communicate with my PN532 module via I2C. Then it turns out hydrabus doesn't support clock stretching in I2C, hence when PN532 performs clock stretching, hydrabus will keep sending the new data until it is done. But all data will be dropped by PN532 and ends up with bad communication.

Here is an example. We can see PN532 is doing clock stretching, which makes the clock ticks hydrabus sends lower than I2C min threshold. However, hydrabus continues to send until all data is done.
image

To help address this problem, I have did a prototype and added the clock stretching handling with 300 cycle as timeout here: https://github.com/r12f/hydrafw/tree/r12f/i2c-clk-stretch. In this fix, it doesn't only wait before ACK or before the first bit, but on every bit, because from my traces, this could happen in other bits too. And after this fix, we can see everything start working from the result of logical analyzer as below:
image

Please let me know if this idea makes sense! I really like hydrabus and hope this helps too!


Tl;dr - Here are some more details on what I am doing:

Basically I am using I2C talking to PN532 in pull up mode with 100khz frequency, which I found via logical analyzer when using it with MCUs, and 2 2.2K external resistors as pull up resistors.

After entering I2C mode, I am sending a command to PN532, then wait for 300ms and then reading back the response, which will be 7 bytes. The key command is shown as below.

[w 0x48 0x00 0x00 0xff 0x02 0xff 0x6a 0x01 0x1a 0x00] &:300 [0x49 r:7]

Before the fix, the command and output looks like below. There are lots of NACKs, which indicates problems.

i2c1> scan
Device found at address 0x24 (0x48 W / 0x49 R)
i2c1> show
GPIO resistor: pull-up
Mode: master
Frequency: 100khz (50khz, 400khz, 1mhz)
i2c1>  [w 0x48 0x00 0x00 0xff 0x02 0xff 0x6a 0x01 0x1a 0x00] &:300 [0x49 r:7]
I2C START
WRITE: 0x48 ACK 0x00 NACK 0x00 NACK 0xFF NACK 0x02 NACK 0xFF NACK 0x6A NACK 0x01 NACK 0x1A NACK 0x00 NACK
I2C STOP
I2C START
WRITE: 0x49 NACK
READ: 0xFF ACK
READ: 0xFF ACK
READ: 0xFF ACK
READ: 0xFF ACK
READ: 0xFE ACK
READ: 0xFE ACK
READ: 0xFE NACK
I2C STOP

After fix the command output also suggests it is working now:

i2c1> show
GPIO resistor: pull-up
Mode: master
Frequency: 100khz (50khz, 400khz, 1mhz)
i2c1> [w 0x48 0x00 0x00 0xff 0x02 0xff 0x6a 0x01 0x1a 0x00] &:300 [0x49 r:7]
I2C START
WRITE: 0x48 ACK 0x00 ACK 0x00 ACK 0xFF ACK 0x02 ACK 0xFF ACK 0x6A ACK 0x01 ACK 0x1A ACK 0x00 ACK
I2C STOP
I2C START
WRITE: 0x49 ACK
READ: 0x00 ACK
READ: 0x80 ACK
READ: 0x80 ACK
READ: 0x80 ACK
READ: 0x80 ACK
READ: 0x80 ACK
READ: 0x80 NACK

This is the trace when it works with MCU, and we can see the clock stretching also happens and being handled:
image

@bvernoux
Copy link
Member

bvernoux commented Dec 28, 2022

It is the first time I see an I2C chipset (PN532) using that "I2C clock stretching feature" which seems to be also related to a wake-up of the PN532 (which use I2C clock stretching in that case):

I confirm I2C clock stretching is not supported in the hydrafw firmware.
I'm clearly not fan of that "I2C clock stretching" feature (even if it is official from I2C specifications) (a bit like multi-master I2C which is not supported too) which sounds like a buggy implemented protocol on device side (as the clock stretching specification does not have any limit and so a device can force SCL and so freeze/lock the I2C Bus with an unspecified time ...)

Note: I have updated the https://github.com/hydrabus/hydrafw/wiki/HydraFW-I2C-guide to add details on what is not implemented/supported in actual firmware

Do you know an other (simpler) I2C device doing "I2C clock stretching" (that will allow to compare with different devices to be sure the implementation of "I2C clock stretching" is done correctly) ?

We can then move that to PR with your code https://github.com/r12f/hydrafw/tree/r12f/i2c-clk-stretch (when you will have validated it)

@r12f
Copy link
Contributor Author

r12f commented Dec 28, 2022

Hi Benjamin, I am with you and also not a fan of I2C clock stretching (You can probably tell from the comment of the code in my prototype : D). I am updating the firmware for handling it mainly for 2 reasons:

  • I am aware of the PN532 IRQ feature. And i am mainly using Hydrabus as a device debugging tool, so it might be better to handle it in a more generic way instead of doing the device-specific way, since it might not work if we switch to another device. Multi-master is another funky thing, but for debugging device specific problem, we can always narrow the scenario down to 1 device first, so it may not be that problematic.
  • I am also thinking about doing a scenario replay with hydrabus. Say, we found some bugs that requires us to explore a bit figure out how to fix it. So a possible way to do it is to craft a replay script from the logic/protocol analyzer trace, replay them, and give us the shell, so we can run any command at that time. Since we are using serial console to copy paste all commands there, it is better to craft the scripts in a blind fire-and-forget way. With this being handled, implementing the replay will be much easier.

For adding the option, definitely! Currently, the fork is just a prototype that demo the idea and fix and see if it makes sense or not. And will need some polishing before actually move to PR. : )

For testing against other the bad devices, here are some chips having this problem. I checked the devices I have, but didn't find one. I wonder if you happen to have any? (Sorry, I am only an entry level hobbyist with daily bad luck orz, the chip PN532 happens to be one of them.)

@r12f
Copy link
Contributor Author

r12f commented Dec 29, 2022

I have also updated my branch with configuration. It works like below and the default value is set to disabled too.

Note: there is no line break in original output, I put a line break after every show / help command in order to make the change more clear.

i2c
GPIO resistor: pull-up
Mode: master
Frequency: 100khz (50khz, 400khz, 1mhz)
Clock stretch timeout: 0 ticks / 0.00 us (0 = Disabled)

i2c1> help
Configuration: i2c [pull (up/down/floating)] [frequency (value hz/khz/mhz)]
Interaction: [<start>] [<stop>] <read/write (value:repeat)>
   show           Show I2C parameters
   trigger        Setup I2C trigger
   pull           GPIO pull (up/down/floating)
   frequency      Bus frequency
   clock-stretch  Max clock stretch tick count. (0 = Disabled, n = Ticks)
   scan           Scan for connected devices
   sniff          Sniff I2C bus
   start          Start
   stop           Stop
   read           Read byte (repeat with :<num>)
   hd             Read byte (repeat with :<num>) and print hexdump
   write          Write byte (repeat with :<num>)
   <integer>      Write byte (repeat with :<num>)
   <string>       Write string
   [              Alias for "start"
   ]              Alias for "stop"
   &              Delay 1 usec (repeat with :<num>)
   %              Delay 1 msec (repeat with :<num>)
   ~              Write a random byte (repeat with :<num>)
   A              Toggle AUX[0](PC4) high
   a              Toggle AUX[0](PC4) low
   @              Read AUX[0](PC4)
   exit           Exit I2C mode

i2c1> clock-stretch 1
i2c1> show
GPIO resistor: pull-up
Mode: master
Frequency: 100khz (50khz, 400khz, 1mhz)
Clock stretch timeout: 1 ticks / 10.00 us (0 = Disabled)

i2c1> clock-stretch 100
i2c1> show
GPIO resistor: pull-up
Mode: master
Frequency: 100khz (50khz, 400khz, 1mhz)
Clock stretch timeout: 100 ticks / 1000.00 us (0 = Disabled)

i2c1> clock-stretch -1
Invalid value.
i2c1> show
GPIO resistor: pull-up
Mode: master
Frequency: 100khz (50khz, 400khz, 1mhz)
Clock stretch timeout: 100 ticks / 1000.00 us (0 = Disabled)

i2c1> clock-stretch 0
i2c1> show
GPIO resistor: pull-up
Mode: master
Frequency: 100khz (50khz, 400khz, 1mhz)
Clock stretch timeout: 0 ticks / 0.00 us (0 = Disabled)

@bvernoux
Copy link
Member

Very nice feature to configure the "I2C clock stretching" timeout especially as it depends on devices and it is better to be configurable than hard coded (as it will do not match all devices ...)
On my side I have bought following part to check all work fine

  • DollaTek SHT30-D, BlueDot BNO055 de 9 Axis IMU, Adafruit MCP9600, Adafruit LC709203F LiPoly/LiIon, Aideepen PN532 NFC NXP Module RFID V3 Kit

@r12f
Copy link
Contributor Author

r12f commented Dec 29, 2022

Wow!! You mean you have just ordered all these devices?

@bvernoux
Copy link
Member

Yes I have ordered them, they shall arrive quickly (some tomorrow ...)

@r12f
Copy link
Contributor Author

r12f commented Dec 29, 2022

Omg! This is some serious dedication! I have ordered a bno055 too, and they should arrive soon as well. Will see how it looks like tomorrow!

@bvernoux bvernoux added this to the 0.11 milestone Dec 29, 2022
@r12f
Copy link
Contributor Author

r12f commented Dec 30, 2022

My BNO055 has arrived today and it is indeed doing clock stretching too! Yeah! Thanks Adafruit for the nice summarizing article.

I used the sample program for BNO055 on Seeed Xiao BLE sense MCU and captured a trace. The start part of the trace looks like below, and we can see it is clock stretching before every device ack.

image

So I switched to hydrabus with my branch to run the following command to mimic the start sequency like below:

clock-stretch 0
show

[w 0x50] &:10 [w 0x50 0x00] &:10 [0x51 r:1]

clock-stretch 100
show

[w 0x50] &:10 [w 0x50 0x00] &:10 [0x51 r:1]

And this is the test result (again, I inserted a few line breaks to make the log easier to read, but in the real log, the additional line break doesn't exist).


First part, clock-stretch is disabled (by default),

i2c
GPIO resistor: pull-up
Mode: master
Frequency: 100khz (50khz, 400khz, 1mhz)
Clock stretch timeout: 0 ticks / 0.00 us (0 = Disabled)

i2c1> clock-stretch 0
i2c1> show
GPIO resistor: pull-up
Mode: master
Frequency: 100khz (50khz, 400khz, 1mhz)
Clock stretch timeout: 0 ticks / 0.00 us (0 = Disabled)

i2c1> [w 0x50] &:10 [w 0x50 0x00] &:10 [0x51 r:1]
I2C START
WRITE: 0x50 ACK
I2C STOP
I2C START
WRITE: 0x50 NACK 0x00 ACK
I2C STOP
I2C START
WRITE: 0x51 ACK
READ: 0x07 NACK
I2C STOP

We can see a bunch of NACKs and from logical analyzer, the signal doesn't show up right.

image


Second part, the clock-stretch is set to 100. Same operations.

i2c1> clock-stretch 100
i2c1> show
GPIO resistor: pull-up
Mode: master
Frequency: 100khz (50khz, 400khz, 1mhz)
Clock stretch timeout: 100 ticks / 1000.00 us (0 = Disabled)

i2c1> [w 0x50] &:10 [w 0x50 0x00] &:10 [0x51 r:1]
I2C START
WRITE: 0x50 ACK
I2C STOP
I2C START
WRITE: 0x50 ACK 0x00 ACK
I2C STOP
I2C START
WRITE: 0x51 ACK
READ: 0xA0 NACK
I2C STOP

Now the signal looks normal and matching more on what MCU is doing:
image


So I believe this change should make BNO055 work while also can be disabled and will be disabled by default in case messing anything up. I am looking forward to the good news on your side too!

@r12f
Copy link
Contributor Author

r12f commented Dec 30, 2022

Just in case, to make things more clear. I have used oscilloscope to check the signal again. Unlike PN532, which only goes half way down. BNO055 goes to 0 when doing clock stretch.

This is with the setting disabled, which we can see there are many clock ticks went missing:

image

And this is with the setting set to 100:

image

@bvernoux
Copy link
Member

Thanks for all the work & measurements
On my side I have received PN532 board & Adafruit MCP9600 Breakout

  • I'm waiting other I2C device with clock stretching DollaTek SHT30-D, BlueDot BNO055 de 9 Axis IMU and Adafruit LC709203F LiPoly/LiIon

Could you create a PR with your modifications like that I could merge them and test on my side ?

@r12f
Copy link
Contributor Author

r12f commented Dec 30, 2022

Great! Absolutely! Here is the PR: #142. And really appreciate the review in advance!

@bvernoux
Copy link
Member

bvernoux commented Dec 31, 2022

Done to be checked all work fine
I have strange things with my PN532

i2c
clock-stretch 1000
  1. PN532_GetFirmwareVersion()

    • PN532_FIRMWARE_VERSION = 0x02
      [0x48 0x00 0x00 0xff 0x02 0xfe 0xd4 0x02 0x2a 0x00] % [0x49 r r:6 r:6]
      I need to execute the command 2 times to have some data then
      It returns
      StatusOK: 0x01
      ackFrameOK: 0x00 0x00 0xff 0x00 0xff 0x00
      csum: 02
      Data: 2A 01 06 07
      2A => Shall be 0x32 (maybe a fake chipset it is quite strange anyway I have no marking on the PN532 chipset)
  2. PN532_SamConfiguration() (Configure the PN532 to read MiFare cards)

    • PN532_COMMAND_SAMCONFIGURATION = 0x14
      Param: 0x01, 0x14, 0x01 (0x01, normal mode, 0x14, timeout 50ms * 20 = 1 second, 0x01, use IRQ pin)
      [0x48 0x00 0x00 0xff 0x05 0xfa 0xd4 0x14 0x01 0x14 0x01 0x03 0x00] %:1 [0x49 r]
      Return StatusOK: 0x01
      If I execute again that command I have some errors
      "i2c clock stretching timed out. Please consider increase the timeout with clock-stretch command to see if it helps."
      To be checked why...
  3. PN532_ReadPassiveTarget() (with a MifareClassic Tag under the PN532 antenna

    • PN532_IN_LIST_PASSIVE_TARGET=0x4A
      Params = 0x01 (1 Card max 7bytes UID) 0x00(PN532_MIFARE_ISO14443A)
      [0x48 0x00 0x00 0xff 0x04 0xfb 0xd4 0x4a 0x01 0x00 0xE2 0x00] % [0x49 %:500 r %:10 r:6 r:8]
      It does not return the tag UID ...

To be checked if I have not mistake in my commands ...

@r12f
Copy link
Contributor Author

r12f commented Dec 31, 2022

So glad to hear it is working!

For PN532_GetFirmwareVersion, I get exactly the same result from my chip too. The IC is 0x2A for me as well. I have run logic analyzer and confirmed it on the wire too.

i2c1> [0x48 0x00 0x00 0xff 0x02 0xfe 0xd4 0x02 0x2a 0x00] % [0x49 r r:6 r:6]
I2C START
WRITE: 0x48 ACK 0x00 ACK 0x00 ACK 0xFF ACK 0x02 ACK 0xFE ACK 0xD4 ACK 0x02 ACK 0x2A ACK 0x00 ACK
I2C STOP
DELAY: 1 ms
I2C START
WRITE: 0x49 ACK
READ: 0x01 ACK
READ: 0x00 ACK
READ: 0x00 ACK
READ: 0xFF ACK
READ: 0x00 ACK
READ: 0xFF ACK
READ: 0x00 ACK
READ: 0x02 ACK
READ: 0x2A ACK
READ: 0x01 ACK
READ: 0x06 ACK
READ: 0x07 ACK
READ: 0xE8 NACK
I2C STOP

For the timeout, it looks like sometimes the device will go to faulty state, if we send unexpected commands, for example:

[0x48 0x00 0x00 0xff 0x02 0xfe 0xd4 0x02 0x2a 0x00] % [0x49 ] % [0x49 r:7]

And then the clock line will be hold down forever by the device, like below and the timeout actually works, as we can see the SDA line stays low for 100ms, which is what I set - 10000 ticks.
image

now, no matter what we run, they will just fail:

i2c1> [0x48 0x00 0x00 0xff 0x02 0xfe 0xd4 0x02 0x2a 0x00]
I2C START
WRITE: 0x48 ACK
i2c clock stretching timed out. Please consider increase the timeout with clock-stretch command to see if it helps.
WRITE error:3

but for SameConfiguration, I double checked the manual here: https://www.nxp.com/docs/en/user-guide/141520.pdf, it looks fine to me. Also if I run it along multiple times, it runs fine as well, so I guess something else must be triggering this, not the command.

i2c1> clock-stretch 10000
i2c1> [0x48 0x00 0x00 0xff 0x05 0xfa 0xd4 0x14 0x01 0x14 0x01 0x03 0x00] %:1 [0x49 r]
I2C START
WRITE: 0x48 ACK 0x00 ACK 0x00 ACK 0xFF ACK 0x05 ACK 0xFA ACK 0xD4 ACK 0x14 ACK 0x01 ACK 0x14 ACK 0x01 ACK 0x03 ACK 0x00 ACK
I2C STOP
DELAY: 1 ms
I2C START
WRITE: 0x49 ACK
READ: 0x00 NACK
I2C STOP
i2c1> [0x48 0x00 0x00 0xff 0x05 0xfa 0xd4 0x14 0x01 0x14 0x01 0x03 0x00] %:1 [0x49 r]
I2C START
WRITE: 0x48 ACK 0x00 ACK 0x00 ACK 0xFF ACK 0x05 ACK 0xFA ACK 0xD4 ACK 0x14 ACK 0x01 ACK 0x14 ACK 0x01 ACK 0x03 ACK 0x00 ACK
I2C STOP
DELAY: 1 ms
I2C START
WRITE: 0x49 ACK
READ: 0x00 NACK
I2C STOP
i2c1> [0x48 0x00 0x00 0xff 0x05 0xfa 0xd4 0x14 0x01 0x14 0x01 0x03 0x00] %:1 [0x49 r]
I2C START
WRITE: 0x48 ACK 0x00 ACK 0x00 ACK 0xFF ACK 0x05 ACK 0xFA ACK 0xD4 ACK 0x14 ACK 0x01 ACK 0x14 ACK 0x01 ACK 0x03 ACK 0x00 ACK
I2C STOP
DELAY: 1 ms
I2C START
WRITE: 0x49 ACK
READ: 0x00 NACK
I2C STOP

For the third one, yes, that is the thing I was trying to debug, since it doesn't work when I uses MCU... my guess is something related to IRQ, so the reply might get delayed and need some way to handle it, but so far I didn't get a successful read via I2C from MCU, so I am quite confused too. I thought it could be me being bad luck again and got a bad chip or somehow damaged it when doing soldering. But seems like you are also seeing the same thing, then it should be something expected, but we might be calling it in a wrong way.

@bvernoux
Copy link
Member

bvernoux commented Dec 31, 2022

I have found my mistakes to interpret the PN532 datasheet ...
Correct way to use the PN532 with HydraBus console (which works perfectly)
i2c
clock-stretch 100

  1. PN532_GetFirmwareVersion

    • PN532_FIRMWARE_VERSION = 0x02
      [0x48 0 0 0xff 2 0xfe 0xd4 2 0x2a 0] % [0x49 r:7][0x49 r:6][0x49 r:6]
      [0x48 0 0 0xff 0xff 0 0][0x49 r:14]
      It returns correctly the PN532 version
      Read: ... 03 32 01 06 07 E8 00
      32 means PN532 and firmware version 01.06
  2. PN532_SamConfiguration (Configure the PN532 to read MiFare cards)

    • PN532_COMMAND_SAMCONFIGURATION = 0x14
      Param: 0x01, 0x14, 0x01 (0x01, normal mode, 0x14, timeout 50ms * 20 = 1 second, 0x01, use IRQ pin)
      [0x48 0 0 0xff 5 0xfb 0xd4 0x14 1 0x14 1 2 0] % [0x49 r:7][0x49 r:6][0x49 r:6]
      [0x48 0 0 0xff 0xff 0 0][0x49 r:10]
      Read: ... 15 16 00
  3. PN532_ReadPassiveTarget (with a MifareClassic Tag under the PN532 antenna)

    • PN532_IN_LIST_PASSIVE_TARGET=0x4A
      Params = 0x01 (1 Card max 7bytes UID) 0x00(PN532_MIFARE_ISO14443A)
      [0x48 0 0 0xff 4 0xfc 0xd4 0x4a 1 0 0xE1 0] % [0x49 r:7][0x49 r:6] %:100 [0x49 r:6]
      [0x48 0 0 0xff 0xff 0 0][0x49 r:20]
      Read: ... 4B 01 01 00 04 08 04 13 53 B6 12 A0 00
    • Which means:
      ATQA: 0x04 SAK: 0x08
      Found an ISO14443A card
      UID Length: 4 bytes
      UID Value: 13 53 B6 12
  4. Full sequence to read Firmware version, ConfigureSAM & Read MifareClassic UID:

  • This sequence wait only 20ms to read the MifareClassic Tag UID... (the minimum seems to be 15ms) increase it if you have any issue (i2c clock stretching timed out...)
  • Warning: Last command to retrieve Tag UID info will fail (i2c clock stretching timed out...) if there is not MifareClassic Tag under the PN532, then you will need to PowerOff/PowerOn the PN532
i2c
clock-stretch 100
[0x48 0 0 0xff 2 0xfe 0xd4 2 0x2a 0] % [0x49 r:7][0x49 r:6][0x49 r:6]
[0x48 0 0 0xff 0xff 0 0][0x49 r:14]
[0x48 0 0 0xff 5 0xfb 0xd4 0x14 1 0x14 1 2 0] % [0x49 r:7][0x49 r:6][0x49 r:6]
[0x48 0 0 0xff 0xff 0 0][0x49 r:10]
[0x48 0 0 0xff 4 0xfc 0xd4 0x4a 1 0 0xE1 0] % [0x49 r:7][0x49 r:6] %:20 [0x49 r:6]
[0x48 0 0 0xff 0xff 0 0][0x49 r:20]

@r12f
Copy link
Contributor Author

r12f commented Jan 1, 2023

nice!!! and thank you so much for the 3rd item too, Benjamin! With this, I think I am close to my problem. I probably didn't place my tiny NFC card properly sometimes, so it could timed out the first read and then get stuck there, and I will need to power recycle it to get the chip back.

The reason is actually caused by PN532 lib here: https://github.com/Seeed-Studio/PN532/blob/40fa7418636da5e03b0126e38c221b143b24b5a4/PN532_I2C/PN532_I2C.cpp#L136. It has a maximum retry count on getResponseLength. When timed out, this function returns -1, and will be used as length to read the response, which ends up making the device being stuck.

Thank you so much again!

@bvernoux
Copy link
Member

bvernoux commented Jan 1, 2023

Test also done with success with "DollaTek SHT30-D Temperature Humidity Sensor"
I2C 50KHz/100KHz/400KHz/1000KHz Clock Stretching 20ms max (high repeatability)
Temp/Humidity measurement duration max 15ms (High repeatability)

i2c
pull floating
frequency 50k
clock-stretch 1000
[0x88 0x2c 0x06][0x89 r:6]
frequency 100k
clock-stretch 2000
[0x88 0x2c 0x06][0x89 r:6]
frequency 400k
clock-stretch 8000
[0x88 0x2c 0x06][0x89 r:6]
frequency 1000k
clock-stretch 20000
[0x88 0x2c 0x06][0x89 r:6]

Issue solved by commit 0038d20
Minor modifications done in commit 8687d22 & c3d97ee

@bvernoux bvernoux closed this as completed Jan 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants