New feature: Non blocking tone queue #3995
Conversation
| delay(5); | ||
| this->tick(); | ||
| #if ENABLED(USE_WATCHDOG) | ||
| watchdog_reset(); |
There was a problem hiding this comment.
That's perverse.
Why do we have the watchdog?
Because we want to guarantee the heaters are checked and refreshed at least every now and then. The K-constants are made for calling the PID algorithm ~5 times a second. Calling it less often makes it unstable.
Resetting the watchdog timer without reason is madness.
There was a problem hiding this comment.
Sorry, I might not have reviewed this properly. I assumed it was calling thermalManager.manage_heater() after it was announced as ready.
Calling it less often makes it unstable.
Do you mean "more often"? If the tone queue is full, it will call watchdog_reset() many more than 5 times per second.
Resetting the watchdog timer without reason is madness
Madness? In the kill() function we have…
while (1) {
#if ENABLED(USE_WATCHDOG)
watchdog_reset();
#endif
} // Wait for reset…and watchdog_reset() is called every time updateTemperaturesFromRawValues is called, which I assume is also pretty often…?
There was a problem hiding this comment.
If there is noting to do manage_heater() returns immediately. You can't call it too often.
void Temperature::manage_heater() {
if (!temp_meas_ready) return;
...
In kill() we turned off the heaters as well as we can before we go into this loop.
updateTemperaturesFromRawValues() is exactly the place where we want the watchdog timer to be restarted.
It's called when the temperature interrupt was able to set temp_meas_ready and the 'normal program' was able to call manage_heater(). (Or in PID_autotune() )
There was a problem hiding this comment.
I called the watchdog reset vector and not the thermal by intent.
I feel we're painting the lily here.. who in is own mind will buzz the robot for 5S.. for more than 20S (4x5) ? Calling the watchdog here is more than reasonable because the counter part is to make the buzzer dependent of the thermal..
There was a problem hiding this comment.
@jbrazio
You have written this stuff, but you don't know how it works?
It needs only one, more than 4s long tone, when you are waiting for it's end so it will make a new place available in the queue.
Alternatively you could limit the length of the tone to 3.5s - but that would also not update the heaters, but at least not trigger the watchdog.
To be worried about the sound, but making the tone in idle()is completely ridicules. Have you tried to do big fast circles with G2/G3 and playing a tone at the same time. Good luck with hearing a 2.5Hz tone.
There was a problem hiding this comment.
You have written this stuff, but you don't know how it works?
It needs only one, more than 4s long tone, when you are waiting for it's end so it will make a new place available in the queue.
Do yourself a favor and try to understand what others are trying to explain you, the question here is not technical but rather practical, my point since the beginning is Marlin is not supposed to be a tune box.. buzzer is used to beep on the menus, before it was blocking now it's not.
Have you tried to do big fast circles with G2/G3 and playing a tone at the same time
You are going wild about this.. in fact ridicule is the degree of extremism you are taking your examples to. Who will try to make Marlin play music while printing ? Shit.. who will try to make Marlin play something other than for fun ?
There was a problem hiding this comment.
Exactly. That's why i don't understand why blocking tones have removed at all. Exchanging blocking tones with a complex, big, not working in all cases, and not less harmful, but abusing the watchdog timer reset, solution, while you have been warned for that makes me sad. (#3913 (comment)) In a RC-phase it makes me raging.
Please reconsider calling manage_heater() in this blocking situation.
There was a problem hiding this comment.
Stop, you're making yourself look a schizofrenik. The same person who said "it's a reasonable compromise, if you don't try crazy things" for the existing buzzer with the definition if crazy being "if I set the time to 2000 it takes 2 sec to go into the menu" now wants to say my code is bad because it's not possible to "big fast circles with G2/G3 and playing a tone at the same time".
But if you want to go that path, when you have time, please explain me on the previous buzzer implementation where the manage_heater() was being called for those long beeps you complain about with my code, here is the relevant source code for it:
unsigned int delay = 1000000 / freq / 2;
int i = duration * freq / 1000;
while (i--) {
WRITE(BEEPER_PIN, HIGH);
delayMicroseconds(delay);
WRITE(BEEPER_PIN, LOW);
delayMicroseconds(delay);
}There was a problem hiding this comment.
manage_heater() is not called her, but there is no reason not to do so.
unsigned int delay = 1000000 / freq / 2;
int i = duration * freq / 1000;
while (i--) {
WRITE(BEEPER_PIN, HIGH);
delayMicroseconds(delay);
WRITE(BEEPER_PIN, LOW);
delayMicroseconds(delay);
+ thermalManager.manage_heater();
}
would have been a reasonable patch.
Playing the the 'Imperial March' is exactly the kind of crazy things i meant.
Ok. Now we can play the 'Imperial March' under some circumstances.
Please reconsider calling manage_heater() in this blocking situation.
There was a problem hiding this comment.
You're being square on purpose right ?
ghost
mentioned this pull request
Squashed and adjusted #3962 by @jbrazio.
BQ_LCD_SMART_CONTROLLERpinsBQ_LCD_SMART_CONTROLLERto be used with any RAMPSOriginal writeup:
This PR fixes #3913 by rewriting from scratch the existing
buzzer.cppto be non-blocking, introducing a tone queue. The length of the tone queue is set atbuzzer.hwith a default of#define TONE_QUEUE_LENGTH 8, I haven't added it to any of theConfiguration.hfiles because I don't believe it will require to be changed for standard Marlin operation, in fact we could even lower the default to4.Of course if you want to play the Imperial March
I advise you touse the following G-Code as a macro on#define TONE_QUEUE_LENGTH 35andPronterface(edit: macros are buggy, use Repetier Host instead). You will require aSPEAKERand not aBUZZER.The timing is a bit "wavy" due to the low priority and non periodic call to
tick(), this is specially noticeable on the higher pitch notes which have a smaller period thus require more precise timing.There is a "untouched" feature of the old
buzzer.cppwhich isLCD_USE_I2C_BUZZER, I don't have hardware to test it and from my research thebuzzer()is delegated into theLCDobject defined byLiquidCrystal_I2C.h-- Please comment bellow if you have more information about this topic.