-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Adding frequency output mode for buzzer #8994
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't hack into the Lights API. This will mess the light settings.
Also keep in mind that you don't control the PWMFrequency that can be 233Hz or 977Hz depending on the Tasmota version.
Edit: I believe you should definitely use the GPIO_BUZZER and leave lights alone, they are applying Gamma Correction so you won't get 50% ratio anyways. Use the low-level analogWrite()
on GPIO_BUZZER and you're done. You will still have the PWM Frequency issue but I can't think of a simple solution here. The PWM Frequency is global to all PWMs.
static uint8_t last_state = 0; | ||
static uint8_t current_color = 0; | ||
if (last_state != state) { | ||
if (state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why you are messing with the lights structure for something that is not a light at all. Doing this way you're just make the Red channel unusable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing. Let me answer this one by one.
- Messing with light control?
No, see xdrv_24_buzzer.ino line 95:
// If we use multi-channel PWM and have a PWM1 output, we can use PWM mode for buzzer output if enabled.
if ((Settings.flag4.buzzer_pwm_mode) && // SetOption102 - Enable PWM mode for buzzer, uses PWM1 if enabled
(Settings.flag3.pwm_multi_channels) && // SetOption68 - Enable multi-channels PWM instead of Color PWM
PinUsed(GPIO_PWM1)) {
Buzzer.pwm_mode = 1;
}
else {
Buzzer.pwm_mode = 0;
}
The PWM buzzer mode depends on Buzzer.pwm_mode which can only be enabled if multi-channels PWM with SetOption68 is also enabled. IOW, you can't use light control and PWM buzzer at the same time.
I asked in the TasmotaUsers group how to best control on/off an individual PWM channel in multi-channel PWM mode directly, and using "light control" is the most high-level and recommended way to do it. analogWrite() did not work and was pointed out to potentially break a lot of things:
https://groups.google.com/g/sonoffusers/c/GxOUfRmHLSg/m/jR-2MawzAgAJ
If you enable multi-channel PWM, the web UI offers to turn on/off the individual channels interactively, the same way I do it here programmatically, so I wouldn't expect this to break anything and be quite safe, actually.
-
Why PWM?
Piezoelectric (cheap) buzzers need an oscillating input. See
https://en.wikipedia.org/wiki/Buzzer#Piezoelectric_2 -
Can't control the PWMFrequency?
Not true - one can use PwmFrequency command: https://tasmota.github.io/docs/Commands/
The duty cycle setting for PWM1 should be left the default 50% but I found that changing it gives slightly different sound, so one can adjust it to make the buzzer sound best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for you answer. I don't understand why you decided that Buzzer and Lights should not be used at the same time. If we are to make this feature available to anyone, we shouldn't have this constraint.
Please just use analogWrite()
on GPIO_BUZZER
. This would make it much easier to understand for users, not needing to change SO68 and not conflicting with PWM lights. This way also you make sure you that Gamma correction will not interfere.
When you say analogWrite()
did not work, I assume it is because you did it on GPIO_PWM1
which was in direct conflict of the light management code.
As mentioned, the only caveat is that the PWMFrequency
should be the same for lights and buzzer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I did not realize that one could use use analogWrite() on GPIO_BUZZER and the platform would then apply PWM to it, I thought that a PWM output could only work on a configured GPIO_PWMx pin. Of course if that works it is the much better option. I will try this and get back to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now changed and works well. Again, thanks for the suggestion.
Measuring the output, it looks like this:
The slightly inaccurate timing has nothing to do with this change, I filed it as #9265.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PWM frequency and generation itself is rock-solid, I agree. The timing issue lies solely in the buzzer implementation. As you can see in the bug report I filed, the exact same issue is present in the current firmware when using the Buzzer command, not using PWM/frequency output at all but static on/off output.
It seems more related to how often or when the buzzer module is called or what timing reference is used to call it. As far as I observed, the 'on' phase can become shorter and the 'off' phase can become longer, but never the other way around, at least with the non-inverted buzzer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, my buzzer does not need PWM. Just 5V and if beeps at his own frequency. What do we see in the diagram? Is it 1second and 10 times on/off or should it be the frequency of the buzzer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is the first I agree. There is no guarantee in TASMOTA to get every50ms control over something. Sometimes the step is just missed if there is too much load. But in this case also a PWM does not help. To shift the on/off into a RTC timer is a bit overengineered but would solve the missing 50ms loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the Buzzer -1 command, the buzzer GPIO pin is turning on/off every 100 ms indefinitely, this is what I used. In your case using the current implementation, the output would look like this:
Every high and low phase is approx. 100 ms here. This is turning an active buzzer such as yours on and off. When using a passive piezoelectric buzzer, this won't do anything, though, because the buzzer doesn't generate the sound itself but is more like a loudspeaker so you need to feed it the frequency it should use to generate a beeping sound. That's what the new option does. In this diagram:
the solid bars are again 100 ms wide but in this case, during the 'on' phase the 50% PWM frequency signal is generated to generate the buzzer sound. You can't see the PWM signal because of the scaling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PWM here is not meant to solve the timing issue, maybe a misunderstanding here. This pull request is only to add the support for passive buzzers.
tasmota/settings.h
Outdated
@@ -121,7 +121,7 @@ typedef union { // Restricted by MISRA-C Rule 18.4 bu | |||
uint32_t zerocross_dimmer : 1; // bit 17 (v8.3.1.4) - SetOption99 - Enable zerocross dimmer on PWM DIMMER | |||
uint32_t remove_zbreceived : 1; // bit 18 (v8.3.1.7) - SetOption100 - Remove ZbReceived form JSON message | |||
uint32_t zb_index_ep : 1; // bit 19 (v8.3.1.7) - SetOption101 - Add the source endpoint as suffix to attributes, ex `Power3` instead of `Power` if sent from endpoint 3 | |||
uint32_t spare20 : 1; | |||
uint32_t buzzer_freq_mode : 1; // bit 20 (v8.3.1.8) - SetOption102 - Use frequency output for buzzer pin instead of on/off signal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to catch up with setoptions now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now using the free SetOption110, but I am unsure about the version note in the comment and just copied the one from the previous new SetOption. Is it going to be adjusted on merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caught up with setoptions and is now using SetOption111.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks better, thanks!
…is should now pass all tests.
Add command ``SetOption111 1`` to enable frequency output for buzzer GPIO (#8994)
Thank you for merging! Please remove the 'on hold' label, thanks. |
Thanks, this works great with my buzzer. It also allows playing simple melodies, e.g. using Backlog PWMFrequency 1396; Buzzer 1,4; PWMFrequency 1760; Buzzer 1,4; PWMFrequency 1396; Buzzer 1,4; PWMFrequency 1396; Buzzer 1,4; PWMFrequency 1976; Buzzer 1,4; PWMFrequency 1396; Buzzer 1,4; PWMFrequency 1318; Buzzer 1,4; PWMFrequency 1396; Buzzer 1,4; PWMFrequency 1046; Buzzer 1,4; PWMFrequency 1396; Buzzer 1,4; PWMFrequency 1396; Buzzer 1,4; Perhaps frequency can be included in the Buzzer command, or an extended Buzzer command if the syntax is not simple to extend? |
The buzzer command format is not easy to extend without breaking it or make it unwieldy to use but I'll take a look at it. A better option yet would be to use the Tone.cpp module in the Arduino framework and libraries such as EasyBuzzer that would support generating melodies without using PWM and analogWrite() at all. But this would add to the code size with not too many people using it so this could push it into a build option that is excluded for regular releases. |
Actually, tone() did not work for my buzzer (needs the analogWrite() method). I had a look at EasyBuzzer, and, while not that much code (basically a wrapper with timers), it also employs tone() for generation (https://github.com/evert-arias/EasyBuzzer/blob/master/src/EasyBuzzer.cpp#L149). Not sure if this is true for all passive buzzers or only the model I happen to have (https://www.kjell.com/se/produkter/el-verktyg/arduino/moduler/passiv-summer-for-arduino-p87887 ) For another project, I did a short function mytone() that incorporates the frequency setting and the analogWrite:
However a problematic part is the delay() that blocks execution. It was fine for that simple project, but for tasmota it needs some other timing - e.g. using EasyBuzzer's update() method and checking millis(). And when playing a melody, it's quite evident when there are other things blocking execution and delaying update() for even a slight time :) Anyway, adding a Melody command could be an option, accepting an array of notes and durations, calling and employing a millis() delay strategy. I'll have a stab at an implementation if I get some time in the near future. |
Yes, you can't use delay() in Tasmota and even the millis() strategy won't give you enough accuracy, see issue #9265. Without using a timer-based mechanism you won't get accurate enough timing for melody playback I am afraid. And based on the shot caller's comments including arendst's, this isn't likely to be accepted in the main branch. Of course one can always fork... |
sorry to bother in, use it like here :: -> is that correct? i have the dev-branch installed (Tasmota EN 11.1.0.2 on ESP32-D0WDQ6), i can use the same buzzer if i connect it to a pwm-configured pin and get (loud) sounds with:
(and user the buzzer-cmd in the backlog-command only for shorter delay timings) edit: i found "buzzerPwm"... is that the right command? |
Allow to use passive buzzers (piezo) by having the buzzer module turn on/off a 50% duty cycle PWM signal on the buzzer pin. (Edited to reflect changed functionality after review)
Description:
Right now the Buzzer command and pin configuration only allows to control active buzzers that generate the sound themselves, so it is a simple turn on/off logic. Some hardware has only passive buzzers based on piezo crystals that need a frequency input. The PWM logic has all capabilities to generate such a frequency. The proposed change causes the existing buzzer logic to turn on/off a 50% PWM frequency at its assigned pin. A new SetOptions flag turns on/off this functionality. (Edited to reflect changed functionality after review)
Code changes are minimal. While I haven't tested it against the ESP32 there is no hardware dependency that could make it fail IMO.
Checklist:
NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass