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

Add a timeout to busio_i2c_scan #5908

Closed
wants to merge 1 commit into from
Closed

Conversation

prplz
Copy link

@prplz prplz commented Jan 22, 2022

@UnexpectedMaker recently discovered an annoyance where i2c scan can hang for over a minute on esp32 (#5906).

It goes something like this:

  • Construct an i2c with floating pins
  • The i2c pullup check in common_hal_busio_i2c_construct passes most of the time despite floating pins
  • Start an i2c scan
  • busio_i2c_scan tries to probe 110 addresses, each probe times out after one second (see this comment), making cpy hang for an unacceptably long time

I have added a two five second deadline to busio_i2c_scan. I hope this will not effect normal operation and will only cover this strange case.

Tested on esp32-s3 hardware. With pullups the scan run in about one second, without the pullups it raises an exception after about two or three five or six seconds.

@prplz
Copy link
Author

prplz commented Jan 22, 2022

Force pushed: changed timeout to 5s.

@dhalbert
Copy link
Collaborator

We can also reduce the actual I2C timeouts to something much shorter.

@prplz
Copy link
Author

prplz commented Jan 23, 2022

Discussed with dan in discord and there's a lot more work to be done with i2c.

@prplz prplz closed this Jan 23, 2022
@dhalbert
Copy link
Collaborator

We can keep this open for discussion now. I will just make it a draft.

@dhalbert dhalbert reopened this Jan 23, 2022
@dhalbert dhalbert marked this pull request as draft January 23, 2022 02:59
@jepler
Copy link
Member

jepler commented Jan 24, 2022

Another possibility is to check for KeyboardInterrupt at each trip through the loop, so that a scan can be ctrl-c'd

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.

Could we make the probe fast instead? https://github.com/adafruit/circuitpython/blob/main/ports/espressif/common-hal/busio/I2C.c#L144

We shouldn't need to wait at all to determine it.

@prplz
Copy link
Author

prplz commented Jan 24, 2022

  • Remove busio_i2c_scan changes
  • Change espressif i2c probe timeout from 1s to 10ms (1 freertos tick, unsure what behaviour zero would have)
  • Change espressif i2c probe timeout from 10s to 1s and use freertos macro to avoid confusion

@prplz
Copy link
Author

prplz commented Jan 24, 2022

This is not working, it looks like i2c_master_cmd_begin caps the delay at 1 second minimum: https://github.com/adafruit/esp-idf/blob/circuitpython/components/driver/i2c.c#L1172

@microdev1
Copy link
Collaborator

esp-idf was updated recently, a merge from main should fix the CI.

@dhalbert
Copy link
Collaborator

dhalbert commented Feb 3, 2022

This is not working, it looks like i2c_master_cmd_begin caps the delay at 1 second minimum: https://github.com/adafruit/esp-idf/blob/circuitpython/components/driver/i2c.c#L1172

This was a reported as a bug:
espressif/arduino-esp32#5934
espressif/esp-idf#4999

went back in time, and when I2C_CMD_ALIVE_INTERVAL_TICK was first introduced, it was a ceiling, not a floor:
https://github.com/espressif/esp-idf/blame/9075b507b54a99ce732fc2819ba17274d9a69ab5/components/driver/i2c.c

        TickType_t wait_time = (ticks_to_wait < (I2C_CMD_ALIVE_INTERVAL_TICK) ? ticks_to_wait : (I2C_CMD_ALIVE_INTERVAL_TICK));

(Commented as such here: espressif/esp-idf#4999 (comment))

dhalbert referenced this pull request in adafruit/esp-idf Feb 3, 2022
- fix i2c crash
- fix usb when enabling wifi

Co-authored-by: Jeff Epler <jepler@gmail.com>
Co-authored-by: Scott Shawcroft <scott@adafruit.com>
@dhalbert
Copy link
Collaborator

dhalbert commented Mar 1, 2022

I think the next step here is to submit one or more PR's (depending on versions) to es to fix espressif/esp-idf#4999. We can also patch this in https://github.com/adafruit/esp-idf for now, since we are already using that fork.

@dhalbert
Copy link
Collaborator

dhalbert commented Mar 8, 2022

@prplz Shall we close this now that you fixed the pullup problem? I think it's still true that ESP-IDF really needs a fix for #5908 (comment). We could open a separate issue about that.

@prplz
Copy link
Author

prplz commented Mar 8, 2022

Sure

@prplz prplz closed this Mar 8, 2022
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.

5 participants