Sound hesitation bug fix, event triggered sounds, host sounds

rondlh commented 1 year ago

Description

This pull request consists out of 3 parts, here they are in order of importance:

1. Print hesitation bugfix. The current TFT sound playback system has a queue of 5 slots that stores the sounds to be played before playback starts. The problem with the current implementation is that if the queue is full, the sound system will block all processes until a sound queue slot becomes available. During this time even printing will be paused because the system is just waiting for a sound slot to become available. This behavior can cause hesitations in the printing process. This PR solves this issue by prioritizing the printing process over sound playback system so no print hesitation can occur because of sound playback.

2. Event driven sounds, no polling. Currently sounds are stored in the sound queue by gcode commands or if the system wants to playback a notification/error/warning sound. These events don't actually start the playback of the sounds. This happens in the main processLoop, where a check is done to see if there are sounds ready for playback in the queue. This process is called polling. This PR makes this process event driven, which means that the arrival of a sound for playback immediately start the sound playback, and the playback of any sounds that arrived afterwards. No check is required in the main loopProcess anymore. This will result in a small efficiency/speed improvement, and can serve as a example of how to efficiently implement event driven processes. Polling should be avoided when possible.

3. Host sounds. Most printer motherboards do not have a speaker/buzzer, one can be added of course, buy why not use the buzzer of the TFT? This process is supported by the Marlin ExtUI interface and works by forwarding the sound that should be played by the motherboard to the TFT. The TFT then takes care of the playback. This PR adds this capability.

4. Remote sound mute/unmute. Additional functionality is added to give the user remote sound control by allowing sounds to be enabled and disabled via the M300 commands. A ESP3D can be used to mute or unmute sounds remotely without using the touch screen.

Benefits

Preventing possible printing hesitations causes by the sound playback when the sound queue is full.
Improving the TFT efficiency by not needing to poll the buzzer queue anymore, and showing how event triggering can be used to prevent polling.
TFT is capable of playing back sounds for the motherboard.
Remote mute/unmute sound control (via Wifi)

Note

This PR affects hardware dependent code (interrupts), which requires testing to make sure the code works on all supported hardware platforms. Actually the changes made are quite small, sounds were already using interrupts. The main difference is that when a sound playback is completed, the process doesn't just exit to let the loopProcess restart it, instead the process check if more sounds need to be played and starts this task immediately, so no attention from loopProcess is needed. This code has been tested on STM32F1xx, STM32F2xx, STM32F4xx and GD32F2xx.

resolves https://github.com/bigtreetech/BIGTREETECH-TouchScreenFirmware/issues/2845

digant73 commented 1 year ago

Just a minor cleanup on indentation (tabs, spaces etc.) and removed some comments. SOUND_SILENCE was missing. Added in SOUND typedef in BuzzerControl.h as (if it was your intention).

buzzer.zip

rondlh commented 1 year ago

Just a minor cleanup on indentation (tabs, spaces etc.) and removed some comments. SOUND_SILENCE was missing. Added in SOUND typedef in BuzzerControl.h as (if it was your intention).

buzzer.zip

Thanks, very appreciated. You did a lot of small cleanup work, you even killed my Easter egg. I will try to follow you example. You are right, I have SOUND_SILENCE defined in BuzzerControl.h, including a few more features. Anything useful in there?

Sound enable/disable control via M300 commands. I use this to be able to switch off/on the sound remotely (ESP3D). M300 S0 to switch off all sounds, except the keypress sound. M300 S1 to switch on the sounds. The P-parameter could even be used to define which sounds, but I do not need this level of control. On and off is good enough for me.
When you switch a sound off, the sounds is replaced by the keypress sound, and only completely switch off when also the keypress sound is also disabled. This way I still have a quiet sound when something happens that doesn't wake up everyone.
Now the keypress sound is a very high frequency (10548HZ, 20ms), this generated interrupts at 21KHz and is difficult to hear for older people. A louder but very similar replacement is: 100Hz 10ms "M300 S100 P10". Perhaps you could try?

digant73 commented 1 year ago

Just a minor cleanup on indentation (tabs, spaces etc.) and removed some comments. SOUND_SILENCE was missing. Added in SOUND typedef in BuzzerControl.h as (if it was your intention). buzzer.zip

Thanks, very appreciated. You did a lot of small cleanup work, you even killed my Easter egg. I will try to follow you example. You are right, I have SOUND_SILENCE defined in BuzzerControl.h, including a few more features. Anything useful in there?

Sound enable/disable control via M300 commands. I use this to be able to switch off/on the sound remotely (ESP3D). M300 S0 to switch off all sounds, except the keypress sound. M300 S1 to switch on the sounds. The P-parameter could even be used to define which sounds, but I do not need this level of control. On and off is good enough for me.

When you switch a sound off, the sounds is replaced by the keypress sound, and only completely switch off when also the keypress sound is also disabled. This way I still have a quiet sound when something happens that doesn't wake up everyone.

Now the keypress sound is a very high frequency (10548HZ, 20ms), this generated interrupts at 21KHz and is difficult to hear for older people. A louder but very similar replacement is: 100Hz 10ms "M300 S100 P10". Perhaps you could try?

ok understood. You can simply override your files with mine and apply your changes on it. Use an editor not providing tabs or trailing empty spaces. Use two spaces for Inline comments (with //)

rondlh commented 1 year ago

ok understood. You can simply override your files with mine and apply your changes on it. Use an editor not providing tabs or trailing empty spaces. Use two spaces for Inline comments (with //)

Clear, thanks. Did you try "M300 S100 P10"? It a suitable as a keypress sound in your view? I usually use Notepad++ for editing code. Before uploading I can execute a few checks to handle the issues you mentioned, perhaps I can even define a macro to do all checks/replacements in 1 go.

digant73 commented 1 year ago

not yet tried. Shall I issue command M300 S100 P10 from ESP3D?

rondlh commented 1 year ago

not yet tried. Shall I issue command M300 S100 P10 from ESP3D?

Yes, or in the terminal. Or if you really want to try how this feels change the values in Configuration.h

#define BUZZER_FREQUENCY_DURATION_MS    10  // in ms. Default: 10
#define BUZZER_FREQUENCY_HZ            100  // in Hz (100Hz to 8000Hz).

digant73 commented 1 year ago

not yet tried. Shall I issue command M300 S100 P10 from ESP3D?

Yes, or in the terminal. Or if you really want to try how this feels change the values in Configuration.h
#define BUZZER_FREQUENCY_DURATION_MS    10  // in ms. Default: 10
#define BUZZER_FREQUENCY_HZ            100  // in Hz (100Hz to 8000Hz).

ok understood. I will make the test this night

digant73 commented 1 year ago

here a review. Please overrides your files with the attached ones. In interfaceCmd.c you forgot to send "ok\n" in case the M300 was issued by a supplementary port (e.g. WiFi). I do not hear any difference changing value for P (P1 or P1000 no difference. P0 no sound). Of course with S19 or above no sound due the command is sent to mainboard not configured to support M300

rondln_pr_review.zip

digant73 commented 1 year ago

This PR is implementing #2845, so in your PR description simply provide the following line so the FR will be automatically closed once the PR is merged:

resolves #2845

EDIT: also remove void loopBuzzer(void); from buffer.h if it is no more externally invoked

rondlh commented 1 year ago

Thanks again, great fixes like usual, I will use more brackets, even for one-liners, and I will add the "resolves" line, currently I'm still awaiting a workflow approval of the maintainer.

Indeed, there is not much difference in sound for the keypress sound when using the lower frequencies, it's a bit louder in my view, but the "feeling" is the same.

I replaced all files, except 1 item.

You swapped the position of SOUND_SILENCE and SOUND_KEYPRESS, but this causes SOUND_SILENCE not to be "played" anymore because it can directly go to the tone() function. I can make it work when swapped, if you think it's important.

You can use the keyword "TFT" to play the sound on the TFT: "M300 TFT P1000 S500".

digant73 commented 1 year ago

use no brackets when you have both statements in the if else with a single line. Use brackets for both statements when at least one has more than one line. So, simply avoid to use bracket for one statement and no bracket for the other statement I didn't swap any line for the SOUND. It seems it was present at origin Yes, I know I can use TFT to play the sound on TFT

rondlh commented 1 year ago

use no brackets when you have both statements in the if else with a single line. Use brackets for both statements when at least one has more than one line. So, simply avoid to use bracket for one statement and no bracket for the other statement I didn't swap any line for the SOUND. It seems it was present at origin Yes, I know I can use TFT to play the sound on TFT

OK, understood, thanks, very appreciated!

digant73 commented 1 year ago

if your PR is ready to be merged exit from draft mode so BTT can consider the PR as ready to be merged. Also, for SOUND_KEYPRESS I would use the current sound. The new sound seems to me too much flat (a sort of puff more than beep) especially in one of my TFTs

ETE-Design commented 10 months ago

@rondlh Is it ready for merge yet?

digant73 commented 10 months ago

@rondlh I'm using your buzzer implementation since some time now and it seems properly working. I see that the function: void loopBuzzer(void) is currrently externally used only on ConnectionSetting.c where it is necessary to avoid a freeze in case you exit from the disconnect menu. If possible I would fix the issue there avoiding the need to externally use the loopBuzzer function so you can remove it from buzzer.h file (of course in buzzer.c file you have to move the function on top of the file to avoid compilation errors). Also you have to remove the Draft flag on this PR in order it can be considered by BTT for a merge

EDIT:

Forget the issue on ConnectionSetting.c. It's not due to the buzzer. I found the root cause and I will fix it on a PR with other fixes/cleanup
I made a review of you buzzer implementation:
- removing on BuzzerControl.c unneeded/wrong code. I left it more similar to the current logic. Only renamed Buzzer_play() to Buzzer_Play() (just to follow the same naming convention used on other APIs). I removed MUTE/UNMUTE (they should need also to implement related commands on remote host API) and the other sounds (e.g. SOUND_STARTUP) not used at all in the TFT. There is time in the future to add them if properly supported/used
- cleanup on buzzer.c/h:
- renamed and sorted functions
- minor code optimization
- removed code line: buzzer.frequency[buzzer.wIndex] = MIN(8000, frequency); // prevent interrupting too fast on Buzzer_AddPlay()
- removed code lines:
```
if (freq > SOUND_KEYPRESS)
tone(freq, duration);
else
Buzzer_play(freq);
```
  on Buzzer_GetSound()
Unless the above code is needed for something else I'm missing, I made the code easy (no pre-processing of a sound). What is defined in BuzzerControl.c (e.g. freq. and duration) are used AS IS in buzzer.c
- Restored current BUZZER_FREQUENCY_DURATION_MS and BUZZER_FREQUENCY_HZ values (e.g. used for Keypress sound) on Configuration.h. It sounds better (better sound and less loud) in particular on TFT35v3. IMHO there is time in the future for tuning/changing sound, tone etc...

I created a PR on your repo with all the above changes. It should be enough to merge my PR and it should be ok also here

rondlh commented 9 months ago

Conflicts were fixed by @digant73, implemented in #2897, well done, thanks a lot!

bigtreetech / BIGTREETECH-TouchScreenFirmware