Skip to content

ledc.c: Fix analogWrite() last channel available verification #8509

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 29, 2023
Merged

ledc.c: Fix analogWrite() last channel available verification #8509

merged 2 commits into from
Aug 29, 2023

Conversation

rlipas
Copy link
Contributor

@rlipas rlipas commented Aug 10, 2023

Description of Change

The last channel allocated is number 0, which conflicted with the value given to an uninitialized pin, giving the "No more analogWrite channels available!" error when trying to use it.

Pins are now given the value -1 to indicate that they are not used so channel 0 can be used without errors.

Tests scenarios

Tested on a custom board with ESP32-S3-WROOM-1 module (8 ledc channels available.)

@CLAassistant
Copy link

CLAassistant commented Aug 10, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

P-R-O-C-H-Y
P-R-O-C-H-Y approved these changes Aug 11, 2023
View reviewed changes
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, TY @rlipas

@P-R-O-C-H-Y P-R-O-C-H-Y added Area: Peripherals API Relates to peripheral's APIs. Status: Pending Merge Pull Request is ready to be merged labels Aug 11, 2023
@me-no-dev
Copy link
Member

This would not work though, because only the first element of the pin_to_channel array will be -1. The rest will be 0. I suggest to keep zero, but use the channel number incremented by 1, so that if pin_to_channel[pin] equals 1, then channel is 0, and if it equals 5, then channel would be 4, so on. This would ensure that all channels are used and that 0 is handled as unused channel

@rlipas
Copy link
Contributor Author

rlipas commented Aug 13, 2023 •
edited
Loading

Oof... I totally forgot how array initializations worked 😅
I fixed this by incrementing the channel as suggested. Sorry for the mix up.
Tomorrow I'll have access to an oscilloscope and I'll be able to properly test this.

Update: All 8 channels seems to be working as expected

@VojtechBartoska VojtechBartoska added this to the 2.0.12 milestone Aug 16, 2023
rlipas and others added 2 commits August 26, 2023 10:02
@rlipas
Fix analogWrite channel available verification
9075978 
The last channel allocated is number 0, which conflicted with the value given to an uninitialized pin, giving the "No more analogWrite channels available!" error when trying to use it
Pins are now given the value -1 to indicate that they are not used so channel 0 can be used without errors.
@rlipas
Fix incorrect array initialization
d4d7a68 
Keeping array of zeros for `pin_to_channel` and shifting stored channel
values by +1 to keep the pin with channel 0 from being interpreted as unused.

ref: #8509 (comment)
@me-no-dev me-no-dev merged commit 2173373 into espressif:master Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@P-R-O-C-H-Y P-R-O-C-H-Y P-R-O-C-H-Y approved these changes

Assignees
No one assigned
Labels
Area: Peripherals API Relates to peripheral's APIs. Status: Pending Merge Pull Request is ready to be merged
Projects
Arduino ESP32 Core Project Roadmap
Status: Done
Milestone
2.0.12
Development

Successfully merging this pull request may close these issues.
5 participants
@rlipas @CLAassistant @me-no-dev @P-R-O-C-H-Y @VojtechBartoska