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

Add tone generation for ESP32 #20704

Merged
merged 4 commits into from
Jan 14, 2021
Merged

Add tone generation for ESP32 #20704

merged 4 commits into from
Jan 14, 2021

Conversation

EvilGremlin
Copy link
Contributor

Description

Add tone generation for ESP32.
Bump up platform version, becuse why not? It looks like nothing's broken.
Someone who actually have an idea how it all works, please check.

Benefits

#define SPEAKER now works
Yay M300 we have cool tunes!

And more sensible speed values
Yay M300, now we have some tunes! Still kinda wurbly because i have no idea what i'm doing.
@thinkyhead
Copy link
Member

Still kinda wurbly…

If the amplitude is irregular, then possibly there's less current to the speaker during certain activities, but most boards should have that pretty well isolated. But if the frequency is uneven, we can try boosting the timer priority (or choosing a timer index with a naturally higher priority) to see if that helps. I'm not sure exactly where to make that change… yet.

Note that currently our ESP32 implementation of HAL_timer_disable_interrupt doesn't do anything. I assume that is by design, and I doubt that has any negative effect on timing.

@EvilGremlin
Copy link
Contributor Author

Ah, TOGGLE is nice, missed that.
There is noticeable stutter every half second or so. Gotta try without webui... But i need to improve circuit first as simple NPN to ground isn't proper driver for piezo beeper.

@thinkyhead thinkyhead merged commit 8049db2 into MarlinFirmware:bugfix-2.0.x Jan 14, 2021
@simon-jouet
Copy link
Contributor

So I just saw this MR. Using timers in the ESP32 works okay but the number of interrupts can cause issues with the WiFi (or vice versa depending in the priorities). I think the tone generation should have been done with the ledc code (like PWM) to use the hardware to generate the tone. I think using the api ledc_set_fade_with_time most of the tone generation can be done in hardware.

@EvilGremlin
Copy link
Contributor Author

Yep, it's just a quick and dirty copypaste, i never expected it to be good. But i made it like that so i could use i2s, and it kinda works. I will use 555 to make pulse lenghts actually usable for sound and report back.

@vivian-ng
Copy link
Contributor

Sorry I haven't been trying out the latest changes to Marlin bugfix. Didn't realize that the tone generation uses I2S. Will this affect the I2S stepper stream? If it affects I2S stepper stream, the ledc idea may be a more compatible solution that doesn't conflict with I2S or Wifi.

@EvilGremlin
Copy link
Contributor Author

Who knows? I need to get it printing first, to see if anything affected. Then again, it won't really play tunes while actually printing and menu beeps may be not sufficient for testing.

@simon-jouet
Copy link
Contributor

So I don't think using I2S for the tone generation is the right thing to do (it will require a software pulse generator for this to happen). Just using the ledc driver and a normal GPIO is the best approach I think. I might have a look at some point but I've never used a buzzer on any of my controllers so this hasn't been very high prio

kpishere pushed a commit to kpishere/Marlin that referenced this pull request Feb 19, 2021
zillarob pushed a commit to zillarob/Marlin that referenced this pull request Feb 25, 2021
W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021
thinkyhead pushed a commit to thinkyhead/Marlin that referenced this pull request Apr 29, 2021
thinkyhead pushed a commit that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants