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

Esp32 s3 touch fix #3798

Merged
merged 10 commits into from
Apr 7, 2024
Merged

Esp32 s3 touch fix #3798

merged 10 commits into from
Apr 7, 2024

Conversation

DedeHai
Copy link
Collaborator

@DedeHai DedeHai commented Mar 6, 2024

fixes touch for S2 and S3 boards. As touch does not work at all without this fix, even though not thoroughly tested, it cannot make things any worse I think.

touch is implemented differently on S2 and S3, these changes make touch buttons work on S2 and S3
now better fits the default threshold value of 32
@blazoncek
Copy link
Collaborator

blazoncek commented Mar 6, 2024

Thanks. Can you also update set.cpp?

EDIT: From lines 239 onward.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Mar 7, 2024

Thanks. Can you also update set.cpp?

EDIT: From lines 239 onward.

I see no change necessary, the digitalPinToTouchChannel() works as is for S2 and S3 (according to comment above the function): " this function works for ESP32, ESP32-S2 and ESP32-S3 - including the C3, it will return -1 for any pin"

@blazoncek
Copy link
Collaborator

I see no change necessary,

But where does then ISR get attached when user sets up a new button? He/she would need to bounce WLED. I do not want that.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Mar 8, 2024

I see no change necessary,

But where does then ISR get attached when user sets up a new button? He/she would need to bounce WLED. I do not want that.

ISR does not actively need to be attached, its a native interrupt vector. pinmode() should take care of that. it works on my S3 so it seems interrupt is set correctly.
what do you mean by 'bounce WLED'?
You can test if it runs on an S2 as well if you have the time. I don't own an S2. If there is any test you want me to run, let me know.

wled00/cfg.cpp Outdated Show resolved Hide resolved
-added minimum threshold, had some crashes when setting threshold to zero before
-moved interrupt detach to GPIO deallocation where it belongs
-added check for touchbutton before detaching interrupt
-moved thochThreshold readout up so it gets updated before passing it to the system call
Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

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

Deataching interrupt handlers in PinManager is only an option if PinManager was also responsible for attaching such handlers.

IMO you can work to incorporate attaching handler into PinManager or remove detaching from PinManager and detach handler in set.cpp in the same place where attaching happens (I would prefer this second option).

wled00/pin_manager.cpp Outdated Show resolved Hide resolved
wled00/set.cpp Show resolved Hide resolved
Compile fix
@blazoncek blazoncek merged commit bd60fe5 into Aircoookie:0_15 Apr 7, 2024
17 checks passed
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