-
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
No, see xdrv_24_buzzer.ino line 95:
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()
onGPIO_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 onGPIO_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.