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

v2.x: Fix RMT mutex unlock using incorrect channel number in rmtDeinit #10034

Merged
merged 1 commit into from
Jul 16, 2024
Merged

v2.x: Fix RMT mutex unlock using incorrect channel number in rmtDeinit #10034

merged 1 commit into from
Jul 16, 2024

Conversation

paccerdk
Copy link

Description of Change

This pull request fixes crashes caused by rmtDeinit function by improving how the RMT channel is handled.

  • Added int channel = rmt->channel; at the beginning of rmtDeinit.
  • Used channel consistently throughout the function, including for mutex operations.

This change mirrors the approach used in the other RMT hal functions such as rmtLoop, rmtWrite, rmtWriteBlocking and others, ensuring consistency across the codebase.

Previously, the line g_rmt_objects[from].channel = 0; would set rmt->channel to 0 before RMT_MUTEX_UNLOCK(rmt->channel) was called. This caused the mutex to always be unlocked with an incorrect 0 as channel number, leading to crashes for channels > 0.

Tests scenarios

Tested on Lolin32 with Arduino-esp32 core v2.0.16, PlatformIO, VS Code, on Linux.
By calling:

rmt1 = rmtInit(21, RMT_TX_MODE, RMT_MEM_64);
rmt2 = rmtInit(22, RMT_TX_MODE, RMT_MEM_64);
rmtDeinit(rmt1); // no crash (channel 0)
rmtDeinit(rmt2); // crash (channel 1), caused by RMT_MUTEX_UNLOCK(0)

This crashed the ESP32 before the pull request (even with proper error checking), and no longer crashes after the changes of the pull request

Related links

There doesn't seem to be any issues discussing this problem

@CLAassistant
Copy link

CLAassistant commented Jul 15, 2024

CLA assistant check
All committers have signed the CLA.

@VojtechBartoska VojtechBartoska added the Status: Review needed Issue or PR is awaiting review label Jul 16, 2024
Copy link
Member

@P-R-O-C-H-Y P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, thank you for the fix.

@P-R-O-C-H-Y P-R-O-C-H-Y added the Area: Peripherals API Relates to peripheral's APIs. label Jul 16, 2024
@me-no-dev me-no-dev added Status: Pending Merge Pull Request is ready to be merged and removed Status: Review needed Issue or PR is awaiting review labels Jul 16, 2024
@me-no-dev
Copy link
Member

cc @SuGlider

@me-no-dev me-no-dev merged commit 54c4b0c into espressif:release/v2.x Jul 16, 2024
48 of 53 checks passed
@paccerdk paccerdk deleted the fix-rmt-deinit branch July 16, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Peripherals API Relates to peripheral's APIs. Status: Pending Merge Pull Request is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants