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

New feature: Non blocking tone queue #3962

Closed

Conversation

@jbrazio
Copy link
Member

commented Jun 4, 2016

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.

@jbrazio jbrazio added this to the 1.1.0 milestone Jun 4, 2016

@jbrazio jbrazio referenced this pull request Jun 4, 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

This comment has been minimized.

Copy link
@thinkyhead

thinkyhead Jun 4, 2016

Member

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.

This comment has been minimized.

Copy link
@jbrazio

jbrazio Jun 10, 2016

Author Member

Will submit a specific PR for this one.

uint16_t frequency;
};

class Buzzer {

This comment has been minimized.

Copy link
@thinkyhead

thinkyhead Jun 4, 2016

Member

If these classes are going to be singletons, we should probably make all their data and methods static for the reduced code size and overhead. The same should apply to the mesh_bed_leveling class, etc. It's a minor hassle, because we have to re-declare all the static data members in the class .cpp files (but maybe only if used in the .cpp file?).

We have a minor "mandate" to reduce code size and try to keep Marlin fitting on smaller (64M) boards. Maybe that just means 64M boards can't use most extra features…?

This comment has been minimized.

Copy link
@jbrazio

jbrazio Jun 4, 2016

Author Member

Scott I believe we have to check if our definition of Singleton is the same.. so far I don't believe we have a single Singleton (excuse the word redundancy) on Marlin.. we have classes which we, by a code of honor, only instantiate once.

Concerning the static, have a look #3680 (comment).

This comment has been minimized.

Copy link
@thinkyhead

thinkyhead Jun 5, 2016

Member

That's pretty much it. Singletons (in our usage) are classes meant to be instantiated only once. And no, we don't get super-formal (e.g., including an "Instance" class method) because we instantiate them with a single declaration. And being a "closed ecosystem" we don't intend to re-use these classes in a variety of systems.

The main reason I've been favoring using classes lately is to get better encapsulation, reduce the amount of globals, and get prettier semantics. But we're still dealing with slow and modest AVR hardware, so wherever we can get static linkage, it is the best idea. It does produce slightly smaller and faster code, and we should take every opportunity where we can shave off 10 bytes or 2 cycles here or there.

uint32_t beepP = code_seen('P') ? code_value_long() : 1000;
if (beepP > 5000) beepP = 5000; // limit to 5 seconds
buzz(beepP, beepS);
uint16_t const frequency = code_seen('S') ? code_value_short() : 110;

This comment has been minimized.

Copy link
@thinkyhead

thinkyhead Jun 4, 2016

Member

What is the effect of const here? I would expect it to produce a compiler error because const needs to be assigned at compile-time. Or is that only static const inside a function?

This comment has been minimized.

Copy link
@jbrazio

jbrazio Jun 4, 2016

Author Member

Const here is a compiler hint and a best practice, when you have a dynamic var which you know will not change after being assigned, it can [or should] be declared const. Also it's worth noting an optimization that can be done on the function's argument list, consider the following function prototype:

bool enqueue(T const &item) ();

I'm not only declaring that my function code will not change the item variable as forcing the compiler not to duplicate the original variable and just pass me the address of the item.

Of course this is valid only for arguments that will not be changed by my function's code.

This comment has been minimized.

Copy link
@thinkyhead

thinkyhead Jun 5, 2016

Member

So this is uint16_t const as opposed to const uint16_t?

[edit] Hmm… I see that apparently these amount to the same thing.

This comment has been minimized.

Copy link
@thinkyhead

thinkyhead Jun 5, 2016

Member

Why T const &item instead of const T &item? Reference to const as opposed to const reference?

[edit] …and apparently these amount to the same thing too.

This comment has been minimized.

Copy link
@jbrazio

jbrazio Jun 5, 2016

Author Member

It's a question of personal preference.

This comment has been minimized.

Copy link
@thinkyhead

thinkyhead Jun 5, 2016

Member

In such matters it's probably best to follow whatever the most common style in Marlin is, if that can be determined.

// Buzzer
#if HAS_BUZZER
#if ENABLED(SPEAKER)
extern Speaker buzzer;

This comment has been minimized.

Copy link
@thinkyhead

thinkyhead Jun 4, 2016

Member

When I made the other singleton classes, I actually included the extern line in the class headers. It forces the user of the class to use the designated name for the instance. If you feel like doing that instead, it's a smidgen more "encapsulated."

This comment has been minimized.

Copy link
@jbrazio

jbrazio Jun 5, 2016

Author Member
In file included from sketch\Marlin.h:72:0,

                 from sketch\M100_Free_Mem_Chk.cpp:43:

speaker.h:30: error: conflicting declaration 'Speaker buzzer'

 extern Speaker buzzer;

                ^

In file included from sketch\Marlin.h:70:0,

                 from sketch\M100_Free_Mem_Chk.cpp:43:

buzzer.h:33: error: 'buzzer' has a previous declaration as 'Buzzer buzzer'

 extern Buzzer buzzer;

               ^

exit status 1
conflicting declaration 'Speaker buzzer'

I am having issues due to inheritance, I could have on the base class a "#if !ENABLED(SPEAKER) but then the base class "knows" about the "top" class which breaks encapsulation also. Any ideas ?

@@ -247,7 +248,8 @@
#endif
#else // !NEWPANEL (Old-style panel with shift register)

#define BEEPER_PIN 33 // No Beeper added
// No Beeper added
#define BEEPER_PIN 33

This comment has been minimized.

Copy link
@thinkyhead

thinkyhead Jun 4, 2016

Member

Do you not like end-of-line comments? I'm not sure that these comments are vital. But end-of-line comments give us more lines on-screen at once, and they are visible in the results when you use Find in your IDE or text editor.

This comment has been minimized.

Copy link
@jbrazio

jbrazio Jun 4, 2016

Author Member

I have nothing against them, 15470dc:

Removes inline comments from BEEPER_PIN #define due to FastIO issues
Inline comments on the BEEPER_PIN 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

This comment has been minimized.

Copy link
@thinkyhead

thinkyhead Jun 4, 2016

Member

I think it's ok to use // comments. Maybe only /* */ comments are an issue.

Well, that is weird. The compiler should totally ignore that stuff. Hmm…

This comment has been minimized.

Copy link
@jbrazio

jbrazio Jun 4, 2016

Author Member

A did the fastest quick fix I had on hand..
If we find a better one I will revert all this garbage back.

This comment has been minimized.

Copy link
@thinkyhead

thinkyhead Jun 5, 2016

Member

Why doesn't the compiler complain about all the many others, I wonder…?

uint32_t duration = code_seen('P') ? code_value_long() : 1000;

// Limits the tone duration to 5 seconds.
if (duration > 5000) duration = 5000;

This comment has been minimized.

Copy link
@thinkyhead

thinkyhead Jun 4, 2016

Member

NOMORE(duration, 5000);

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Jun 4, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2016

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

This comment has been minimized.

Copy link
Member

commented Jun 4, 2016

@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

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2016

@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

This comment has been minimized.

Copy link
Member

commented Jun 5, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2016

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

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Jun 5, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2016

Or I could call this->tick().

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Jun 5, 2016

I think this may be better:

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

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2016

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 jbrazio force-pushed the jbrazio:feature/nonblocking-buzzer branch 2 times, most recently from c28d0e1 to 0db8fe7 Jun 5, 2016

@jbrazio

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2016

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

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Jun 9, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2016

@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

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2016

5 seconds? Good luck with the watchdog.

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

This comment has been minimized.

Copy link
@Blue-Marlin

Blue-Marlin Jun 9, 2016

Contributor

Please call thermalManager.manage_heater(); here.

@jbrazio

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2016

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

@Blue-Marlin

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2016

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

This comment has been minimized.

Copy link
Member

commented Jun 9, 2016

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)…?

jbrazio added 6 commits Jun 4, 2016
Removes inline comments from BEEPER_PIN #define due to FastIO issues
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 jbrazio force-pushed the jbrazio:feature/nonblocking-buzzer branch from e433ef8 to 63f10eb Jun 9, 2016

jbrazio added 5 commits Jun 5, 2016
@jbrazio

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2016

@thinkyhead ready to merge.

@thinkyhead thinkyhead closed this Jun 10, 2016

@jbrazio jbrazio deleted the jbrazio:feature/nonblocking-buzzer branch Jun 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.