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

ESP32-S2 I2C: lengthy gaps (about 10 ms) between transactions. (IDFGH-7169) #8770

Closed
dhalbert opened this issue Apr 12, 2022 · 32 comments
Closed
Assignees
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally

Comments

@dhalbert
Copy link

dhalbert commented Apr 12, 2022

  • Development Kit: none (Adafruit boards)
  • Module or chip used: ESP32-S2-WRONGER
  • IDF version
    v4.4-351-g121ddb87e5
  • Build System: [Make|CMake|idf.py]
  • Compiler version
    xtensa-esp32-elf-gcc (crosstool-NG esp-2021r2-patch3) 8.4.0
  • Operating System: Linux
  • Using an IDE?: No
  • Power Supply: USB

Problem Description

Using I2C at 100_000 Hz (standard clock rate) cause ~10ms delays between I2C transactions. Raising to 125_000 causes the delays to go away. In between, only some I2C speeds work; others produce glitches that cause the I2C device to fail. Testing on a PCA9685. A colleague tried another device and was able to get it to work at 110_000.

Expected Behavior

There should not be excessive delays at 100kHz. I was suspicious that 10ms is close to the FreeRTOS tick interval, but the gaps vary sometimes between 9.something and occasionally as low as 7.something, so that may be a red herring.

Steps to reproduce

Tested in CircuitPython, which currently uses the ESP-IDF version shown above.

Code to reproduce this issue

See adafruit/circuitpython#6263. These forum posts report what sounds like the same issue:

https://www.esp32.com/viewtopic.php?p=95404
https://www.esp32.com/viewtopic.php?p=95405

I have Saleae traces if that is of interest.

I spent quite a bit of time inside the IDF I2C code, adjusting timeouts (ticks_to_wait), printing things, and trying various tweaks, and I cannot locate the source of this issue. I am wondering if it is hardware or hardware-setup related.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Apr 12, 2022
@github-actions github-actions bot changed the title ESP32-S2 I2C: lengthy gaps (about 10 ms) between transactions. ESP32-S2 I2C: lengthy gaps (about 10 ms) between transactions. (IDFGH-7169) Apr 12, 2022
@dhalbert
Copy link
Author

dhalbert commented Apr 14, 2022

I believe this is related to CONFIG_FREERTOS_HZ. A common default value of CONFIG_FREERTOS_HZ is 100, and is what we use in CircuitPython. ESP32 Arduino and MicroPython both use CONFIG_FREERTOS_HZ=1000. Arduino typically shows a 1ms. If I change CircuitPython to use CONFIG_FREERTOS_HZ=1000, the 10ms delay also becomes 1ms.

If I raise the I2C frequency only slightly, then the large gap goes away. For instance, at 125000, the gap reduces from 10ms to 600-700ish microseconds. So there is something unusual about the 100kHz rate compared to higher rates.

@o-marshmallow
Copy link
Collaborator

Hi @dhalbert ,

I tried on my side with the following parameters:

  • an ESP32
  • FreeRTOS frequency set to 100Hz
  • Tried with IDF master, release/v4.4 and v4.4-351-g121ddb87e5, same result
  • I2C frequency set to 100KHz (effective frequency of about 87KHz)
  • Two transactions
    Here is the result:
    image

The code I used is heavily inspired by the one you linked from ESP Forum. So I don't know if this problem only hits the ESP32-S2, I don't have one right now to test.

Please, could you try to reproduce it with IDF and share your example with me?

@dhalbert
Copy link
Author

@o-marshmallow What I2C peripherals do you have available? I would like a real I2C device to communicate with, so I can get ACKs. I'll do a simple example with an MSA301 or PCA9685 or something like that. Thanks.

@o-marshmallow
Copy link
Collaborator

Hi @dhalbert ,
I connected an Atmel 24C16 EEPROM for the test. I also have a TCA8418. That's all I have at the moment.

@dhalbert
Copy link
Author

dhalbert commented Apr 16, 2022

I wrote a short test but could not reproduce the problem. I think it is in some way related to FreeRTOS task handling. I may close or revise this as I debug further.

@o-marshmallow
Copy link
Collaborator

@dhalbert, you are right, if you have ay other task that has the same or higher priority than the one performing the I2C one, it is possible that FreeRTOS pre-empts your I2C task to let another one, of same of higher priority, run. In this case, you will see a delay between the two transfers.

Regarding the frequency, my hypothesis is that increasing the I2C clock frequency will let the transfer finish earlier, thus you'll still have time to start the second transfer before being pre-empted by another task.

So, what you can do is raise the priority of the I2C task before initiating both your transfers, thanks to vTaskPrioritySet, and restore it after finishing. Or, you could prepare both transfers inside the same i2c_cmd_handle_t.

@dhalbert
Copy link
Author

dhalbert commented Apr 20, 2022

@o-marshmallow I have managed to reproduce this with a very simple example. It is not a task issue, as far as I can tell. The key is that I set the clock speed to 240 MHz instead of 160 MHz. Here is the simple ESP-IDF example, which is a cut-down version of the I2C example in the esp-idf.

i2c_delay.zip

This example builds for an Adafruit Metro ESP32-S2 board, which is based on the WROVER module, but with only slight changes it should be usable on an Espressif or other dev board.

It writes 4 bytes (could be less) to a register on an I2C device over and over again. I picked an arbitrary device, an MSA311, but it does not really matter what device it is, as long as it will ACK writes.

When the clock frequency is 240 MHz (the choice in sdkconfig in the zip file above), there are gaps of almost 10 ms between I2C transactions. If you change CONFIG_FREERTOS_HZ, the gap size will change accordingly. Here is a Saleae trace:
ksnip_20220419-223438

When the clock frequency is 160 MHz, the gaps disappear:
ksnip_20220419-223717

I also discovered that adding a very small delay at a crucial point in the ESP-IDF I2C code will fix the problem. The very end of i2c_master_cmd_begin_static() calls i2c_hal_trans_start():

i2c_hal_update_config(&(i2c_context[i2c_num].hal));
i2c_hal_trans_start(&(i2c_context[i2c_num].hal));
return;
}

If I add a small delay immediately after that, the problem goes away:

    i2c_hal_update_config(&(i2c_context[i2c_num].hal));
    i2c_hal_trans_start(&(i2c_context[i2c_num].hal));
    for (size_t i = 0; i < 50; i++) {         // ADD DELAY HERE
        asm("");
    }
    return;
}

The delay must be after i2c_hal_trans_start(). I discovered the need for the delay after adding some pin-toggling instrumentation code elsewhere, which mysteriously fixed the problem. I worked backwards to this point.

So, the key to producing the problem is the higher clock speed, and the key to the fix is a slight delay, which perhaps is related to the I2C hardware in some way.

@o-marshmallow o-marshmallow self-assigned this Apr 20, 2022
@o-marshmallow
Copy link
Collaborator

o-marshmallow commented Apr 20, 2022

@dhalbert Thanks for your example!

I was able to reproduce the issue on an ESP32-S2 I just got. This issue also happens on the S3 and C3even with a CPU frequency of 160MHz. But your fix doesn't work though. Adding a delay at the end of i2c_master_cmd_begin_static() didn't fix it.

After investigating, I have found that the problem is not linked to the I2C hardware IP but to FreeRTOS. In fact, after starting an I2C transfer, we wait for any I2C event (NACK/END/etc...), you can see this on the line 1490:

        // In master mode, since we don't have an interrupt to detective bus error or FSM state, what we do here is to make
        // sure the interrupt mechanism for master mode is still working.
        // If the command sending is not finished and there is no interrupt any more, the bus is probably dead caused by external noise.
        portBASE_TYPE evt_res = xQueueReceive(p_i2c->cmd_evt_queue, &evt, wait_time);

The counterpart of it is in the interrupt handler, line 527:

i2c_cmd_evt_t evt = {
      .type = I2C_CMD_EVT_ALIVE
};
xQueueSendFromISR(p_i2c->cmd_evt_queue, &evt, &HPTaskAwoken);

but also, and mainly, line 1318 and 1327:

        // Notify the task caller that the transfer is done
        evt.type = I2C_CMD_EVT_DONE;
        xQueueOverwriteFromISR(p_i2c->cmd_evt_queue, &evt, &HPTaskAwoken);

For some reasons, the function xQueueOverwriteFromISR doesn't notify the main task (which is waiting on xQueueReceive), that an element has been introduced in the queue. Thus, the main task will only be aware of it at the
next FreeRTOS tick, when checking the FIFO size. This is why there was a 1 tick delay.

I have tried to get rid of the xQueueOverwriteFromISR function and simply use xQueueSendFromISR in the ISR, and that worked for me, here is the patch. Please, have a try, and tell me if this works for you too:
i2c.patch.txt

You can apply it with:

cd $IDF_PATH
patch -p1 < i2c.patch.txt

I haven't checked in depth yet why xQueueOverwriteFromISR doesn't notify the task waiting on the queue whereas xQueueSendFromISR does. Anyway, have a try and tell me if it works properly. I will try to get this patch merged on master.

@dhalbert
Copy link
Author

dhalbert commented Apr 20, 2022

Thank you @o-marshmallow!

Were you testing against master or against v4.4?

I was testing against c29343e, the tip of release/v4.4 when I last pulled from esp-idf. When I try your fix on my i2c_delay example, and it works. But I also tested your patch against https://github.com/adafruit/circuitpython; circuitpython is also using v4.4, though it's a little behind, at 121ddb8. In circuitpython, my test runs for a few seconds, but then I get an I2C timeout, and I see that the SDA line is stuck high.

I experimented with lowering the CPU frequency to 160 MHz to see if that was the issue. That did not help. With your patch, re-adding my small delay (as above) also did not help. But when I went back to the original i2c.c code with just my delay patch and not your changes, it works reliably again. It also works reliably at 160 MHz in CircuitPython without my delay patch.

I tried advancing circuitpython to master, but there are many deprecations and other changes we have to deal with, and I cannot yet get it to build. It would take some more time to bring it forward.

So I am not sure how to continue testing in the short run. Your patch works on v4.4 on the i2c_delay example, but not in circuitpython, which has another, higher-priority task, some core affinity preferences, etc. If you backport your current fix to v4.4, it may work for your test, but would not work in our case. Perhaps there are FreeRTOS changes you know about from v4.4 to master that might explain the issues.

You might test my i2c_delay against v4.4, if you have not done so. I am not sure what you would find out. If you get different results than I did, that would be surprising, and we'd need to check why. But it might explain why my delay patch works for me, even though it did not work for you (against master?).

Other minor notes

Your patch did not quite apply, because the IRAM_ATTR had been removed from the function declarations in the first hunk of the patch. But that was easy to fix.

BTW, I got this compilation warning, which you probably know about:

home/halbert/repos/esp-idf/components/driver/i2c.c: In function 'i2c_master_cmd_begin_static':
/home/halbert/repos/esp-idf/components/driver/i2c.c:1278:19: warning: unused variable 'HPTaskAwoken' [-Wunused-variable]
     portBASE_TYPE HPTaskAwoken = pdFALSE;
                   ^~~~~~~~~~~~

@o-marshmallow
Copy link
Collaborator

Hi @dhalbert ,

Yesterday I did my test on master, but today I did it on v4.4, with both commits you gave and it works great.

Unfortunately, I can't help with circuitpython implementation, once I get these fixes in IDF, I think it will help more to open an issue CircuitPython's Github project.

In order to track this issue in IDF, I toggled a GPIO as a way to "see" what is going on before and during the I2C transfer. When I toggle the GPIO after the xQueueReceive in i2c_master_cmd_begin, I noticed that it was being toggled 9ms after the I2C transfer was started, this is how I found that the queue had a weird behavior. Maybe you can do the same with CircuitPython in order to see where the I2C transfer is being blocked? Where does the task hang most of the time?

In any case, I am going to create an internal fix for this bug in master and release/v4.4

@dhalbert
Copy link
Author

dhalbert commented Apr 21, 2022

In order to track this issue in IDF, I toggled a GPIO as a way to "see" what is going on before and during the I2C transfer. When I toggle the GPIO after the xQueueReceive in i2c_master_cmd_begin, I noticed that it was being toggled 9ms after the I2C transfer was started, this is how I found that the queue had a weird behavior. Maybe you can do the same with CircuitPython in order to see where the I2C transfer is being blocked? Where does the task hang most of the time?

I did almost exactly the same thing: I turned a pin on before the XQueueReceive() and turned it off afterward. But it became a "Heisenbug": when I did that, the 10ms delay went away! My original code was:

        gpio_set_direction(5, GPIO_MODE_DEF_OUTPUT);   // Redundant after the first time.
        gpio_set_level(5, true);
        portBASE_TYPE evt_res = xQueueReceive(p_i2c->cmd_evt_queue, &evt, wait_time);
        gpio_set_level(5, false);

If I removed the gpio_set_direction() from that spot and did it much earlier, the toggling then did not fix the problem. Eventually I tried ets_delay_us(1), which also fixed the problem, and removed the pin toggling. Then I moved the delay back into i2c_master_cmd_begin_static(), found out where it was necessary, and made it just a loop, to remove any other possibilities.

I also found that running at 160 MHz instead of 240 MHz fixed the problem. This is why I suspected a hw timing problem.

I just want to make sure that you tested your fix at 240 MHz. I am wondering if there are two bugs here: one hw timing, and one task-related.

I am one of the core developers of CircuitPython, so I opened an issue already and assigned it to myself. 🙂 A user found this problem originally, while trying to use an I2C motor controller at high speed.

@o-marshmallow
Copy link
Collaborator

Hi @dhalbert ,

I confirm I tested with both 160MHz and 240MHz, the delay between two transfers is slightly less with the latter.
Delay with 240MHz:
240mhz

Delay with 160MHz:
160mhz

The device is still the 24C16 EEPROM.

As you said, maybe there are two bugs here and we only solved one but I can't get the second one to occur with IDF only.
Do you have a ready-to-test script, repo, docker image which reproduces your problem with CircuitPython and IDF so that I could give a try?

@dhalbert
Copy link
Author

dhalbert commented Apr 28, 2022

Do you have a ready-to-test script, repo, docker image which reproduces your problem with CircuitPython and IDF so that I could give a try?

Apologies, I missed your reply until today. Which ESP32-S2 board(s) do you have, and are they WROVER or WROOM? I can give you a .bin or similar with the problem, and also a .zip or similar of the repo snapshot.

@o-marshmallow
Copy link
Collaborator

@dhalbert Currently, I only have a single one, it's an ESP32-S2-WROOM.
A .zip should be ok, as long as I can reproduce the problem and debug it, it will help 😄

@dhalbert
Copy link
Author

I think I have reproduced the problem I was seeing in CircuitPython by modifying my original example to add another task. This should be a lot easier to debug.

Test program

The test program is now in this public repo: https://github.com/dhalbert/i2c_delay.

I added a second task to this program. It doesn't do anything but delay for a very short time and then yield for 1 tick. It runs at priority 5, though I think that is not essential. This roughly mimics a high-priority task CircuitPython uses to do TinyUSB work servicing.

I am building https://github.com/dhalbert/i2c_delay with esp-idf/master at commit a82e6e6, which is very recent. I am testing on a Saola ESP32-S2 WROOM, using an MSA311 as the test I2C device.

Below is what I see without and with your patch. I am not including my delay patch at all.

Without your patch

  1. When building with vanilla esp-idf/master at commit a82e6e6, with the I2C bus speed at 100 kHz, I see the 10 ms delays reported originally.
  2. When I increase the bus speed to 400 kHz, the 10ms delays go away, and there is about 22us between I2C transactions, which is good.

With your patch

  1. Like 1, but including your patch from above. See tag other-task-causes-timeouts in my test repo. I see frequent ESP_ERR_TIMEOUTs from the I2C write in the serial log output:
E (2832) i2c-delay: i2c_write() status: ESP_ERR_TIMEOUT
E (4112) i2c-delay: i2c_write() status: ESP_ERR_TIMEOUT
E (5142) i2c-delay: i2c_write() status: ESP_ERR_TIMEOUT
...

These timeouts are like the errors I was seeing in CircuitPython.

  1. Like 2 (400 kHz I2C bus speed), with your patch, I still see these ESP_ERR_TIMEOUTs. See tag other-task-causes-timeouts-400k.

So it appears your patch works in the simplest case, but not when there is another task present that is doing some work.

If I reduce the timeout in the other task to 0, by doing ets_delay_us(0), then the problem does not appear.

A note on I2C devices

I was able to reproduce your results of not being able to get rid of the original 10 ms delays by using a different I2C device, a 24LC32, which is an EEPROM very similar to your 24C16: same commands, same I2C address. I think these I2C EEPROM's may have unusual timing characteristics. For instance, the data sheets say they are non-responsive when an EEPROM write is in progress. I think it might be better to test with the TCA8418, and just do a simple write to some innocuous state register (like setting pullups or something)., kind of like what I am doing with the MSA311. If you can get some typical sensor like an MSA301 or MSA311 eventually, that would also be a good test. I realize you may be at home with little opportunity to get more devices at the moment.

In the long run, it would also be good to test with devices that are known to do clock stretching. We have a short list here.

Thank you for working on this.

@readmodifywrite
Copy link

I am also seeing this issue. Maybe some extra data points will help.

IDF 4.4.1
ESP32 dual core
240 MHz
RTOS tick at 1000 Hz
I2C set to 400 KHz with an effective clock of 357 KHz.

There is an approximate 335 us delay between the call to start the I2C transaction and the actual start of the bits on the wire. That is around 80,000 CPU clocks of doing nothing. In case anyone is tempted to suggest that 335 us is not a lot of time, I would point out that on a battery powered application that is a truly massive waste of power. It's enough of a problem that I may have to consider bit banging directly (which might also correct the incorrect I2C clock rate, which is arguably a second bug).

@o-marshmallow I tried your patch, but it kernel panics on the idle task.
@dhalbert I tried your delay trick, that does nothing in my configuration.

@o-marshmallow
Copy link
Collaborator

Hi @readmodifywrite , your problem is not linked to the one @dhalbert is encountering. In fact, ESP32 doesn't have this bug.

While checking your issue, I've found a potential bug for the frequency issue you are encountering. One part of the fix is to use a smaller pull-up resistor. When I did my test locally, I also got a frequency of 357KHz, I was using a 4.7KOhm resistor, I switched to a 1KOhm one and now I get 370KHz.
The second part of the fix is this patch:
i2c_freq.patch.txt
After applying this patch, you should get about 394KHz

Regarding your 335us delay I suspect you are using i2c_cmd_link_create, i2c_master_write_byte, ..., i2c_cmd_link_delete in each loop iteration, right?
If that's the case, a lot of time is spent for allocating and freeing memory from the heap. I wouldd suggest you to use i2c_master_write_to_device, i2c_master_read_from_device, i2c_master_write_read_device functions which don't use heap memory. The biggest your transaction is, the more time you will save.
Second thing you can try is to activate the compiler optimization (if not already), you will gain something like 5 or 6% of time between each transactions. You can activate this in the menuconfig, option COMPILER_OPTIMIZATION_PERF

@o-marshmallow
Copy link
Collaborator

Hi @dhalbert ,

Sorry for the delay. Thank you very much for your example, I was able to reproduce the issue and as you said, using the 24C16
was a bad idea as when a write is going on, it just stops responding on the bus, so I used a DS1307 RTC (I remember I had it too) to reproduce the issue.

I first tested with the fix I gave earlier and I also got the timeouts. These issue are really volatile, as soon as I add a small delay, everything goes back to normal... My hypothesis is that there was a race condition between the moment the I2C task is about to enter the critical section inside xQueueReceive and the moment where the task is pre-empted to let the higher priority task run.

I didn't dig too long this fix and instead I got back to the original code that is on master, the problem was that when we enqueue a value inside the I2C event queue in the ISR, the I2C task didn't get the notification. And indeed, the notification/yield needs to be done manually in the ISR. I came up with this fix which works on my side with the DS1307, I hope it'll work for you too!

Here is the patch you can apply directly on top of master:
i2c_delay.patch.txt

@dhalbert
Copy link
Author

dhalbert commented May 6, 2022

@o-marshmallow: i2c_delay.patch.txt works for me! I tested it in CircuitPython, altering the patch slightly to make it work on v4.4:
i2c_delay.v4.4.patch.txt
Testing an MSA301 or MSA311 at 100khZ, 110kHz, and 400kHz shows no excessive delays. I also tested with a BNO055, which does clock-stretching, and also saw no timing problems.

Thank you very much! I think we will incorporate this into our fork of esp-idf for now, and then move back to espressif/esp-idf after it is merged in.

@readmodifywrite
Copy link

@o-marshmallow I'll try your patch out when I can get around to it. But could we just get that fixed in an official release? I'd like to avoid doing manual patching in my builds if I can avoid it.

You got me on the i2c_cmd_link_create, etc! That's exactly how I was doing it. I didn't realize that was doing a heap allocation each time, that's definitely not what I want. Thanks very much!

@o-marshmallow
Copy link
Collaborator

@readmodifywrite Indeed, it will be fixed officially. The patch is already under review, I only provided it to you so you could test right now without waiting mster branch to be updated 😄

No problem, happy to help you!

Note: If your transactions don't differ a lot from one to the other (only the data buffer changes), you can even re-use the command link, so no need to free and re-create. In that case it would be even faster than i2c_master_write_to_device/i2c_master_read_from_device.
For example, let's say you are using an I2C EEPROM which can write at most 8 bytes at once (it's its page size), then, in order to initialize several pages as fast as possible, you could do:

/* I will ignore the error check, to make it easier to understand */
#define I2C_PAGE_SIZE 8

/* Allocate the 10 bytes buffer: 2 bytes for the page address, I2C_PAGE_SIZE bytes for data */
uint8_t data[2 + I2C_PAGE_SIZE] = { 0 };

/* Create the command handle we will use accross the whole init */ 
i2c_cmd_handle_t handle = i2c_cmd_link_create();
i2c_master_start(handle);
i2c_master_write_byte(handle, DEVICE_ADDRESS << 1 | I2C_MASTER_WRITE, true);
i2c_master_write(handle, data, sizeof(data), true);
i2c_master_stop(handle);

/* Let's say we will do this for all the first 256 pages */
for (int i = 0; i < 256; i++) {
            i2c_master_cmd_begin(I2C_MASTER_NUM, handle, YOUR_TIMEOUT / portTICK_PERIOD_MS);
            /* Finished, preapre the next transaction by modifying buffer. Indeed, the I2C transaction command doesn't copy the pointer content, it simply saves it. */
            data[0] = (i * I2C_PAGE_SIZE) >> 8; // Big-endian page number
            data[1] = (i * I2C_PAGE_SIZE) & 0xff;
}

In such cases, you can simply modify the pointer of data, the I2C command link can be reused. This is feature available since IDF v4.4 (commit 7dd499d)

@o-marshmallow
Copy link
Collaborator

@dhalbert Awesome! I am happy to hear that!
I am going to create internal fixes for master, then I will backport it to previous versions (v4.4, v4.3, ...)

@readmodifywrite
Copy link

Thanks @o-marshmallow!

@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new Resolution: NA Issue resolution is unavailable labels May 23, 2022
tannewt pushed a commit to adafruit/esp-idf that referenced this issue Jun 28, 2022
@AxelLin
Copy link
Contributor

AxelLin commented Jun 30, 2022

@dhalbert Awesome! I am happy to hear that! I am going to create internal fixes for master, then I will backport it to previous versions (v4.4, v4.3, ...)

@o-marshmallow
The fix 872a431 is only available in master, still not available in v4.4 and v4.3.

BTW, as I mentioned before that there are some missing i2c fixes in stable branches so it's difficult to trace the issues.
e.g. #8358 (comment) is still not in v4.3 even though you mentioned that it will be fixed #8358 (comment) .

@o-marshmallow
Copy link
Collaborator

@AxelLin An internal MR is on-going for 872a431 for both v4.4 and v4.3.

Moreover, v4.3 MR also includes the fix da12db2

@litui
Copy link

litui commented Jul 14, 2022

Interesting. Just stumbled upon this thread. I admit I'm new to low level troubleshooting of I2C and don't fully understand the cause, but I've found that simply increasing the ESP32 I2C timeout to around 66% of the maximum (easier to talk in % than numbers as different chip revisions store the timeout in different numbers of bits) seems to guarantee success in my applications. Explanation in my PR, but this does not require an esp-idf change if anyone needs a fix in the meantime. adafruit/circuitpython#6594

@o-marshmallow
Copy link
Collaborator

Hi @litui ,

This issue didn't concern ESP32 boards, only newer versions such as the ESP32-S2, ESP32-S3, etc.
The symptoms were that, using a logic analyzer, we could see that the minimum amount of time between two distinct I2C transaction was 1 FreeRTOS tick (10ms by default). It was not related to I2C controller's timeout but to FreeRTOS ISR handling. So the problem you encountered maybe be another issue?

I checked your issue, PR and repo and I am not sure to undertstand the problem you were having.
Did the functions i2c_master_write_read_device/i2c_master_write_to_device/i2c_master_cmd_begin return a timeout error? If yes, have you tried increasing the timeout parameter passed to these function? In my opinion, 10/portTICK_PERIOD_MS seems a bit low, mainly if FreeRTOS default config is used (100Hz tick).

@litui
Copy link

litui commented Jul 14, 2022

@o-marshmallow yes, the timeout provided to each of those functions did not register a change. It was only by changing the overall timeout value that I saw success with my devices.

@AxelLin
Copy link
Contributor

AxelLin commented Jul 27, 2022

@AxelLin An internal MR is on-going for 872a431 for both v4.4 and v4.3.

Moreover, v4.3 MR also includes the fix da12db2

@o-marshmallow
I cannot find above fixes in v4.3/v4.4. What's wrong?

@mitchellcairns
Copy link

I do not believe this issue is resolved. Testing 4.4 on ESP32 and I'm getting 19ms between master writes. I'm recycling the same cmd link and just starting it again. If there's something I'm missing I'm open to suggestions.

@AxelLin
Copy link
Contributor

AxelLin commented Oct 5, 2022

I do not believe this issue is resolved. Testing 4.4 on ESP32 and I'm getting 19ms between master writes. I'm recycling the same cmd link and just starting it again. If there's something I'm missing I'm open to suggestions.

The fix cb411a8 is committed after v4.4.2. Update to latest v4.4 branch to verify it.

@mitchellcairns
Copy link

mitchellcairns commented Oct 5, 2022

I do not believe this issue is resolved. Testing 4.4 on ESP32 and I'm getting 19ms between master writes. I'm recycling the same cmd link and just starting it again. If there's something I'm missing I'm open to suggestions.

The fix cb411a8 is committed after v4.4.2. Update to latest v4.4 branch to verify it.

Yes, I am using the latest 4.4 branch and getting the same results. Not 4.4.2.

As previously mentioned, changing the Freertos tickrate has an impact on this. I'm trying to get 1ms intervals. Changing the tickrate from 100hz to 1000hz gets me there, but ideally this would not be the case.

I've opened a new issue as this may be a separate issue with the code impacting ESP32 and not ESP32-S2?
#9919

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests

7 participants