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

Delete i2c cmd_mux semaphore more cleanly (v4.3 branch) (IDFGH-5061) #6846

Closed

Conversation

makermelissa
Copy link
Contributor

See adafruit#1 for more info.

@CLAassistant
Copy link

CLAassistant commented Apr 8, 2021

CLA assistant check
All committers have signed the CLA.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Apr 8, 2021
@github-actions github-actions bot changed the title Delete i2c cmd_mux semaphore more cleanly Delete i2c cmd_mux semaphore more cleanly (IDFGH-5061) Apr 8, 2021
@makermelissa makermelissa changed the title Delete i2c cmd_mux semaphore more cleanly (IDFGH-5061) Delete i2c cmd_mux semaphore more cleanly (v4.3 branch) (IDFGH-5061) Apr 8, 2021
@dhalbert
Copy link

dhalbert commented Apr 8, 2021

This change fixes an problem we have seen in CircuitPython between i2c_driver_delete() and esp_wifi_start(). It is not a hard crash, but in our case we lose the USB CDC connection, and things stop running: it may be a runaway set of interrupts or some other loop. The usual IDF threads that are running when the issue happens look the same as in the good case.

The essence of the problem is this sequence:

i2c_driver_delete(NULL);   // does nothing
i2c_param_config();
i2c_driver_install();
i2c_driver_delete();   // This is crucial.

esp_netif_init();               
esp_event_loop_create_default();
xEventGroupCreateStatic();
esp_event_handler_instance_register();
esp_event_handler_instance_register();
esp_wifi_init();
esp_wifi_set_mode();
esp_wifi_start();

I arrived at this change by divide and conquer, seeing which part of the i2c.c was causing the issue.Unfortunately esp_wifi_start(), which is where the problem seems to occur, is in the closed-source wifi stack.

The critical piece of the original i2c_driver_delete() code is:

xTakeSemaphore();
vSemaphoreDelete();

If the xTakeSemaphore() is completely removed, or if the xGiveSemaphore() is added, then we don't see the problem. I arrived at adding the xGiveSemaphore() by examining other semaphore cleanup code in the IDF.

I have tried to create a minimal example in ESP-IDF. I have one I, but the only difference I see immediately is that the DEBUG traces between the "good" and the "bad" cases. It would be great if someone with access could look inside esp_wifi_start() to see what the actual issue might be.

Other drivers (e.g., SPI, etc.) do not affect esp_wifi_start() in this way. As mentioned, there is more information in adafruit#1 and a longer discussion in adafruit/circuitpython#4387.

@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution.

@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels May 31, 2021
@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution, and sorry for late reply, we have put the changes into our internal review queue, and the PR will be updated once the changes are available on GitHub. Thanks.

@espressif-bot espressif-bot added the Awaiting Response awaiting a response from the author label Jul 16, 2021
@L-KAYA
Copy link
Collaborator

L-KAYA commented Jul 19, 2021

Thanks for your contribution again! This PR has been merged into release/v4.3 (commit:2ac59cc8). Could you help to check whether the issue has been fixed.

@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution.

@espressif-bot espressif-bot added Resolution: Done Issue is done internally Status: Done Issue is done internally and removed Status: In Progress Work is in progress labels Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Response awaiting a response from the author Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants