Skip to content

Fix ESP32-S3 LEDC channel mapping for multiple servo support #73

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

forsslund
Copy link

I had issues with the original library not controlling all my 6 servos, only first 2 and last, and had Claude 4.0 debug and fix it. Completely vibe coded be aware. But solves it for me.

LLM-written description below:

Problem

The ESP32Servo library had a critical bug when using multiple servos on ESP32-S3 boards. Only the first two servos and the last servo would function correctly, while servos in the middle positions would not receive proper PWM signals.

Root Cause

The issue was in the timerAndIndexToChannel() function in ESP32PWM.cpp. The original code assumed all ESP32 variants had the same LEDC channel configuration as the ESP32 Classic (16 channels), but ESP32-S3 only has 8 LEDC channels (0-7) instead of 16.

The incorrect channel mapping caused the library to attempt using non-existent channels 8-15 on ESP32-S3, resulting in failed PWM generation for servos assigned to these channels.

Solution

This PR implements ESP32-S3-specific channel mapping using conditional compilation:

  • Timer 0: channels 0, 1
  • Timer 1: channels 2, 3
  • Timer 2: channels 4, 5
  • Timer 3: channels 6, 7

Changes Made:

  1. timerAndIndexToChannel(): Added ESP32-S3-specific logic with explicit channel mapping
  2. allocatenext(): Limited to 2 channels per timer for ESP32-S3 (vs 4 for other variants)
  3. allocateTimer(): Updated to handle correct channel limits per ESP32 variant

Backward Compatibility

The fix is fully backward compatible - other ESP32 variants (ESP32 Classic, ESP32-S2, ESP32-C3) continue to use the original logic and are unaffected.

Testing

Verified with 5 servos on ESP32-S3 (Adafruit Feather ESP32-S3 compatible).

ESP32-S3 has only 8 LEDC channels (0-7) instead of 16 like ESP32 Classic.
This fix ensures proper channel allocation for ESP32-S3:
- Timer 0: channels 0, 1
- Timer 1: channels 2, 3
- Timer 2: channels 4, 5
- Timer 3: channels 6, 7

Fixes issue where only first two and last servo would work correctly.
@madhephaestus
Copy link
Owner

madhephaestus commented May 25, 2025

maxChannelsPerTimer needs to be a class variable not a local private variable inside allocateTimer it shoud be set in the header with the compiler directives and not re-set in executing code. In fact is should probibally be a #define, not a variable, since it is not possible for that value to change in the hardware at runtime.

@forsslund
Copy link
Author

Thanks for feedback! I moved to a #define. I only have the s3 to test with but noted that the number of timers varies: https://docs.espressif.com/projects/arduino-esp32/en/latest/api/timer.html maybe add more defines, now we have for S2, S3 and C3.

@madhephaestus
Copy link
Owner

Can you do a compilation test targeting each of the platforms, as well as the ESP32 Dev Module, and add the compilation result print statements as comments, one per target?

@forsslund
Copy link
Author

I tried with my AI (Cline/Claude) but that was a bit too challenging for it and unfortunately I don't have time to dig so deep into this myself right now. Feel free to change the code as you wish and thanks for the library to start with. Maybe I can contribute more later.

@madhephaestus
Copy link
Owner

Testing is not an LLM task, it is verifying that you, yourself, tested that the code changes actually compile, and the result works on your hardware. then verify that the other targets also can still compile without changes to the code. this needs to be done in the Arduino environment, not in an LLM.

LLM code assistance is cool to help get past blockers, but ultimately needs you, the developer, to be able to test and verify the code changes. Just to be clear, no maintainer of any code base would merge changes that were not at least tested. If a code change affects multiple targets, then that change needs to be tested on all of those variants. I need the compiler to check it, not the LLM, because after 2 years of near daily LLM use for programming, i know for a fact that it is going to make huge and blocking mistakes, daily. That means that LLM's are good at being coding interns, but can not be the main developer, because the testing is totally outside of scope for an LLM.

Open Source development is 80% social, and 20% actual code changes. To get changes merged into mainline, you need to convince the maintainer (me) that you have verified that the changes you made will not negatively affect the 10's of thousands of other users that rely on this package. I take the responsibility to maintaining stability for the other folks very seriously.

@forsslund
Copy link
Author

You are right, it was quite a premature pull request straight into main from a new developer straight out of nowhere. I was just happily surprised the LLM found a bug/workaround in the lib and wanted to be good citizen and report it (more as a heads up/suggestion than proper contribution). Perhaps it should better be filed in issues? I saw a thread I think relates to this: #35
Or the development branch? How would you prefer these kind of things be reported in the future?

Let me see if I can do the proper testing anyway coming days to make things proper.

@madhephaestus
Copy link
Owner

the LLM ought to be able to walk you through the testing process, setting up Arduino and the tool-chains, cloning your fork into the Libraries folder, and running compile check against each of the processors. You can even feed the compilation errors into the LLM and have it correct its own mistakes in an iterative process.

My full time job is an elementary teacher, I no longer work at the school where i used this library in my classes and therefore had time to do extensive testing myself. I need to rely on the PR's being tested by the domain experts in order to incorporate changes. I don't have any S3 chips at all, and cant go out and get one each of the affected platforms, so i really can not actually test the solution myself.

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.

2 participants