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

My solution to solve the watchdog issues with custom sound generation #3310

Open
wants to merge 7 commits into
base: MK3
Choose a base branch
from

Conversation

bmuessig
Copy link

@bmuessig bmuessig commented Dec 7, 2021

As discussed issue #2045, long sound durations often cause the watchdog timer to reset the printer, as the sound generation is currently using regular delays that don't feed the watchdog timer.
I am proposing a hybrid solution that uses regular delays for short beeps and a modified version of delay_keep_alive that doesn't update the LCD for long delays that exceed 1.1 seconds.

@bmuessig
Copy link
Author

bmuessig commented Dec 7, 2021

Having hit PR number 3310 in a sound-related PR, the classic Nokia 3310 ringtone came to mind: https://www.youtube.com/watch?v=h6iIFVTwKeE

Firmware/Marlin.h Outdated Show resolved Hide resolved
// Waits for the duration and uses the most appropriate delay type
void Sound_Delay(uint16_t ms)
{
if (ms < (SOUND_KEEPALIVE_MIN_MS))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if doing something like this is really needed. You could just use delay_keep_alive(noUIupdates) regardless of duration. As long as no sounds are generated, delay_keep_alive() is just fine at all times, with the added bonus of managed heaters and inactivity messages on uart so that the host doesn't time out.

Copy link
Author

Choose a reason for hiding this comment

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

I was not sure how much delay the update routine adds (which might also vary), so I thought it would make sense to keep the length of short beeps more accurate. But if this is not required, this could be simplified indeed 👍

@@ -13,6 +13,9 @@ typedef enum
typedef enum
{e_SOUND_CLASS_Echo,e_SOUND_CLASS_Prompt,e_SOUND_CLASS_Confirm,e_SOUND_CLASS_Warning,e_SOUND_CLASS_Alert} eSOUND_CLASS;

// The minimum number of milliseconds of sound duration, to use the keep-alive method instead of the default delay
// This is used to allow the printer to retain control in long-running sound output
#define SOUND_KEEPALIVE_MIN_MS 1100
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really relevant considering the points above, but just a quick observation:
Instead of doing if (delay < 1100) delay(); else delay_keep_alive();, do if (delay > 1000) delay_keep_alive(); else delay();

Copy link
Author

Choose a reason for hiding this comment

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

I didn't just choose 1100 ms to exclude one second long beeps, but I added 100 ms in case someone somewhere used a delay around 1 second, but not exactly.

@bmuessig
Copy link
Author

bmuessig commented Dec 7, 2021

Alright, I have now combined the multiple methods into one with the second optional parameter controlling whether to update the LCD.

Now, what's the best minimum threshold to trigger delay_keep_alive instead of the regular delay?
We could lower it from the current 1100 ms to e.g. 260 or 510 ms, but I want to avoid that regular beeps vary in length.
Many long running update operations might produce an audible difference, so I would suggest keeping the regular delay method for the short default beeps.

@leptun
Copy link
Collaborator

leptun commented Dec 8, 2021

I'll look at the updated PR soonTM (maybe tomorrow). In the meantime, here is a gcode with music:
WII.zip
You can try playing with the treshold and see if it negatively affects the file. Please also use the stock firmware first to get a baseline (it's kinda crappy already).

I think delay>1000 is a good enough threshold instead of >1100. It doesn't really matter at that point if it's 1000 or 1050ms. This threshold might only improve things when quick beeps are produced. WII.gco includes some of these in a quick sequence in the second half of the song.

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