New feature: Non blocking tone queue #3962

jbrazio · 2016-06-04T20:23:39Z

This PR fixes #3913 by rewriting from scratch the existing buzzer.cpp to be non-blocking, introducing a tone queue. The length of the tone queue is set at buzzer.h with a default of #define TONE_QUEUE_LENGTH 8, I haven't added it to any of the Configuration.h files because I don't believe it will require to be changed for standard Marlin operation, in fact we could even lower the default to 4.

Of course if you want to play the Imperial March ~~I advise you to #define TONE_QUEUE_LENGTH 35 and~~ use the following G-Code as a macro on ~~Pronterface~~ (edit: macros are buggy, use Repetier Host instead). You will require a SPEAKER and not a BUZZER.

M300 P300 S220
M300 P301 S0
M300 P300 S220
M300 P301 S0
M300 P300 S220
M300 P301 S0
M300 P225 S174
M300 P226 S0
M300 P75 S261
M300 P76 S0

M300 P300 S220
M300 P301 S0
M300 P225 S174
M300 P226 S0
M300 P75 S261
M300 P76 S0
M300 P600 S220
M300 P601 S0

M300 P300 S329
M300 P301 S0
M300 P300 S329
M300 P301 S0
M300 P300 S329
M300 P301 S0
M300 P225 S349
M300 P226 S0
M300 P75 S261
M300 P76 S0

M300 P300 S207
M300 P301 S0
M300 P225 S174
M300 P226 S0
M300 P75 S261
M300 P76 S0
M300 P600 S220
M300 P601 S0

M300 P300 S440
M300 P301 S0
M300 P225 S220
M300 P226 S0
M300 P75 S220
M300 P76 S0
M300 P300 S440
M300 P301 S0
M300 P225 S415
M300 P226 S0
M300 P75 S392
M300 P76 S0

M300 P75 S369
M300 P76 S0
M300 P75 S329
M300 P76 S0
M300 P150 S349
M300 P151 S0
M300 P151 S0
M300 P150 S233
M300 P151 S0
M300 P300 S311
M300 P301 S0
M300 P225 S293
M300 P226 S0
M300 P75 S277
M300 P76 S0

M300 P75 S261
M300 P76 S0
M300 P75 S246
M300 P76 S0
M300 P150 S261
M300 P151 S0
M300 P151 S0
M300 P150 S174
M300 P151 S0
M300 P300 S207
M300 P301 S0
M300 P225 S174
M300 P226 S0
M300 P75 S220
M300 P76 S0

M300 P300 S261
M300 P301 S0
M300 P225 S220
M300 P226 S0
M300 P75 S261
M300 P76 S0
M300 P600 S329
M300 P601 S0

M300 P300 S440
M300 P301 S0
M300 P225 S220
M300 P226 S0
M300 P75 S220
M300 P76 S0
M300 P300 S440
M300 P301 S0
M300 P225 S415
M300 P226 S0
M300 P75 S392
M300 P76 S0

M300 P75 S369
M300 P76 S0
M300 P75 S329
M300 P76 S0
M300 P150 S349
M300 P151 S0
M300 P151 S0
M300 P150 S233
M300 P151 S0
M300 P300 S311
M300 P301 S0
M300 P225 S293
M300 P226 S0
M300 P75 S277
M300 P76 S0

M300 P75 S261
M300 P76 S0
M300 P75 S246
M300 P76 S0
M300 P150 S261
M300 P151 S0
M300 P151 S0
M300 P150 S174
M300 P151 S0
M300 P300 S207
M300 P301 S0
M300 P225 S174
M300 P226 S0
M300 P75 S261
M300 P76 S0

M300 P300 S220
M300 P301 S0
M300 P225 S174
M300 P226 S0
M300 P75 S261
M300 P76 S0
M300 P600 S220
M300 P601 S0
M300 P1200 S0

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.cpp which is LCD_USE_I2C_BUZZER, I don't have hardware to test it and from my research the buzzer() is delegated into the LCD object defined by LiquidCrystal_I2C.h -- Please comment bellow if you have more information about this topic.

thinkyhead · 2016-06-04T21:28:12Z

Marlin/pins_BQ_ZUM_MEGA_3D.h

@@ -60,19 +60,34 @@
 #define PS_ON_PIN         81 // External Power Supply

 #if ENABLED(BQ_LCD_SMART_CONTROLLER) // Most similar to REPRAP_DISCOUNT_SMART_CONTROLLER


I'm thinking this block should just move to pins_RAMPS_14.h with the rest of the LCD controller pins. Then we don't need the #undef lines, and this controller will be usable on regular RAMPS setups.

Will submit a specific PR for this one.

thinkyhead · 2016-06-04T22:17:47Z

It all seems like it should work, but are you saying that if the buffer is too small, then it will play multiple tones incorrectly? For example, with a 4-tone buffer size, I can only play 4 tones before adding new tones will overwrite tones that haven't been played yet? If so, perhaps it makes sense to actually block inside the tone method (calling tick() frequently) as long as the queue is full.

I posted a few comments on the code. If you could make a few tweaks then it can be merged. (Don't worry about making the classes static if that seems like too much work for too little gain.)

We should probably add opt_enable SPEAKER someplace in the Travis test too…

jbrazio · 2016-06-04T22:22:51Z

No override, the tones get ignored if queue is full, but of course as soon as one slot on the buffer queue gets free you may inject more tones to the tail.

thinkyhead · 2016-06-04T22:28:33Z

@jbrazio Well that's no good. GCode files are distributed that contain nothing but music. This change would break the ability to play those files. If the queue simply blocks, then it would be just like the old behavior, basically, since delay() is totally blocking, and that would be ok.

jbrazio · 2016-06-04T22:47:37Z

@thinkyhead Maybe I did not understand your second part of the comment.. but it is non-blocking as tones not fitting the queue are "just" discarded, Marlin will not stop anything when the ring buffer is full. I can improve this since enqueue() return a boolean either it was able to fit the item on the queue, we can at least echo a serial error or.. IMO the solution would be to implement the CircularQueue as an ResizingQueue but his means C++'s new and delete.. if everyone is OK it that.

We should probably add opt_enable SPEAKER someplace in the Travis test too…

Will do.

thinkyhead · 2016-06-05T01:51:44Z

We cannot use new or delete.

What I would like is for nothing to ever be discarded, so that M300 exhibits no change in behavior. We shouldn't be throwing errors for M300. If the tone queue is full, the tone function should wait in a blocking loop until a space opens up in the queue, and only then add the tone to the queue and return. This will retain the current behavior, where you can play as many tones as you want, but still allow single tones and very short sequences, smaller than the ring buffer, to be non-blocking.

jbrazio · 2016-06-05T02:01:27Z

Something like this ?

    void tone(uint16_t const &duration, uint16_t const &frequency = 0) {
      while(!this->buffer.enqueue((tone_t) { duration, frequency })) delay(10);
    }

Blue-Marlin · 2016-06-05T02:08:56Z

@jbrazio
No. idle() instead of delay(). Else you don't get a tone.

thinkyhead · 2016-06-05T02:09:06Z

That won't work, because the buffer won't be getting processed. (Unless it's being processed in an ISR.)

[edit] Right. What @Blue-Marlin said…

jbrazio · 2016-06-05T02:10:33Z

Or I could call this->tick().

thinkyhead · 2016-06-05T02:12:28Z

I think this may be better:

while (this->buffer.isFull()) { this->tick(); delay(10); }
this->buffer.enqueue((tone_t) { duration, frequency });

Blue-Marlin · 2016-06-05T02:17:55Z

Right. It would be more useful to do idles, but we are in danger to get a memory overflow when the next commands are all consecutive M300's. On the other side we really want to update the heaters. (G2/G3 problem)

jbrazio · 2016-06-05T15:46:05Z

Note to self: Never use macros with Pronterface, they're buggy as hell.

thinkyhead · 2016-06-09T03:24:54Z

On the other side we really want to update the heaters.

As long as there are no excessively long beeps (several seconds) then it should be ok.

jbrazio · 2016-06-09T14:41:55Z

@Blue-Marlin @thinkyhead we're limiting the max beep to 5S (M300) but it's possible for a dev calling directly buzzer.tone() to overcome this limitation.

Blue-Marlin · 2016-06-09T16:59:52Z

5 seconds? Good luck with the watchdog.

Blue-Marlin · 2016-06-09T17:09:10Z

Marlin/buzzer.h

+     * @param frequency Frequency of the tone in hertz
+     */
+    void tone(uint16_t const &duration, uint16_t const &frequency = 0) {
+      while (buffer.isFull()) this->tick();


Please call thermalManager.manage_heater(); here.

jbrazio · 2016-06-09T17:10:13Z

What's wrong with the watchdog, this is a non-blocking tone() implementation remember ?

Blue-Marlin · 2016-06-09T20:52:17Z

Unblocking until the buffer is full and you are blocking here for up to 5 seconds. The Watchdog will give its alarm after 4.

thinkyhead · 2016-06-09T22:57:29Z

manage_heater

@jbrazio In case it's not obvious, calling this keeps the watchdog alive also.

So perhaps when the queue is full, call this->tick(), thermalManager.manage_heater(), and then perhaps a short delay(5)…?

…o the new libs

Inline comments on the BEEPER_PINT was causing the compiler to barf: pins_RAMPS_14.h:221: error: pasting "/* Beeper on AUX-4*/" and "_RPORT" does not give a valid preprocessing token

jbrazio · 2016-06-10T01:39:17Z

@thinkyhead ready to merge.

jbrazio added Bug: Confirmed ! PR: Bug Fix labels Jun 4, 2016

jbrazio added this to the 1.1.0 milestone Jun 4, 2016

jbrazio mentioned this pull request Jun 4, 2016

Long "buzzes" block Marlin #3913

Closed

thinkyhead reviewed Jun 4, 2016
View reviewed changes

jbrazio force-pushed the feature/nonblocking-buzzer branch 2 times, most recently from c28d0e1 to 0db8fe7 Compare June 5, 2016 15:39

Blue-Marlin reviewed Jun 9, 2016
View reviewed changes

jbrazio added 6 commits June 10, 2016 00:34

New object: Circular Queue (aka ring buffer)

6941d28

Fixes BQ ZUM Mega warnings when changing the BEEPER pin

27297bc

New objects Buzzer and Speaker for non-blocking tone playback

7e76850

Moves LCD_FEEDBACK stuff into Conditionals.h and updates the buzzer t…

4198a7c

…o the new libs

Makes use of the new non-blocking tone lib

64694e0

Removes inline comments from BEEPER_PIN #define due to FastIO issues

a500aa0

Inline comments on the BEEPER_PINT was causing the compiler to barf: pins_RAMPS_14.h:221: error: pasting "/* Beeper on AUX-4*/" and "_RPORT" does not give a valid preprocessing token

jbrazio force-pushed the feature/nonblocking-buzzer branch from e433ef8 to 63f10eb Compare June 9, 2016 23:48

jbrazio added 5 commits June 10, 2016 00:48

Less wavy sound produce by Speaker due to optimization

6ecd397

Makes CircularQueue statically allocate memory

773964e

Adds travis test for SPEAKER

c0e8151

Documents the Buzzer, Speaker and CircularQueue classes

63f10eb

Buzzer::tone() now calls the watchdog reset

079ec35

jbrazio added the S: Please Merge label Jun 10, 2016

thinkyhead mentioned this pull request Jun 10, 2016

New feature: Non blocking tone queue #3995

Merged

thinkyhead closed this Jun 10, 2016

jbrazio deleted the feature/nonblocking-buzzer branch June 11, 2016 22:27

jbrazio mentioned this pull request Jul 25, 2016

M300 doesn't work correctly #4394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New feature: Non blocking tone queue #3962

New feature: Non blocking tone queue #3962

jbrazio commented Jun 4, 2016 •

edited

Loading

thinkyhead Jun 4, 2016 •

edited

Loading

jbrazio Jun 10, 2016

thinkyhead commented Jun 4, 2016 •

edited

Loading

jbrazio commented Jun 4, 2016

thinkyhead commented Jun 4, 2016 •

edited

Loading

jbrazio commented Jun 4, 2016 •

edited

Loading

thinkyhead commented Jun 5, 2016 •

edited

Loading

jbrazio commented Jun 5, 2016

Blue-Marlin commented Jun 5, 2016

thinkyhead commented Jun 5, 2016 •

edited

Loading

jbrazio commented Jun 5, 2016

thinkyhead commented Jun 5, 2016 •

edited

Loading

Blue-Marlin commented Jun 5, 2016 •

edited by thinkyhead

Loading

jbrazio commented Jun 5, 2016

thinkyhead commented Jun 9, 2016

jbrazio commented Jun 9, 2016

Blue-Marlin commented Jun 9, 2016

Blue-Marlin Jun 9, 2016

jbrazio commented Jun 9, 2016

Blue-Marlin commented Jun 9, 2016 •

edited by thinkyhead

Loading

thinkyhead commented Jun 9, 2016 •

edited

Loading

jbrazio commented Jun 10, 2016

		@@ -60,19 +60,34 @@
		#define PS_ON_PIN 81 // External Power Supply

		#if ENABLED(BQ_LCD_SMART_CONTROLLER) // Most similar to REPRAP_DISCOUNT_SMART_CONTROLLER

New feature: Non blocking tone queue #3962

New feature: Non blocking tone queue #3962

Conversation

jbrazio commented Jun 4, 2016 • edited Loading

thinkyhead Jun 4, 2016 • edited Loading

Choose a reason for hiding this comment

jbrazio Jun 10, 2016

Choose a reason for hiding this comment

thinkyhead commented Jun 4, 2016 • edited Loading

jbrazio commented Jun 4, 2016

thinkyhead commented Jun 4, 2016 • edited Loading

jbrazio commented Jun 4, 2016 • edited Loading

thinkyhead commented Jun 5, 2016 • edited Loading

jbrazio commented Jun 5, 2016

Blue-Marlin commented Jun 5, 2016

thinkyhead commented Jun 5, 2016 • edited Loading

jbrazio commented Jun 5, 2016

thinkyhead commented Jun 5, 2016 • edited Loading

Blue-Marlin commented Jun 5, 2016 • edited by thinkyhead Loading

jbrazio commented Jun 5, 2016

thinkyhead commented Jun 9, 2016

jbrazio commented Jun 9, 2016

Blue-Marlin commented Jun 9, 2016

Blue-Marlin Jun 9, 2016

Choose a reason for hiding this comment

jbrazio commented Jun 9, 2016

Blue-Marlin commented Jun 9, 2016 • edited by thinkyhead Loading

thinkyhead commented Jun 9, 2016 • edited Loading

jbrazio commented Jun 10, 2016

jbrazio commented Jun 4, 2016 •

edited

Loading

thinkyhead Jun 4, 2016 •

edited

Loading

thinkyhead commented Jun 4, 2016 •

edited

Loading

thinkyhead commented Jun 4, 2016 •

edited

Loading

jbrazio commented Jun 4, 2016 •

edited

Loading

thinkyhead commented Jun 5, 2016 •

edited

Loading

thinkyhead commented Jun 5, 2016 •

edited

Loading

thinkyhead commented Jun 5, 2016 •

edited

Loading

Blue-Marlin commented Jun 5, 2016 •

edited by thinkyhead

Loading

Blue-Marlin commented Jun 9, 2016 •

edited by thinkyhead

Loading

thinkyhead commented Jun 9, 2016 •

edited

Loading