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.

Sign up for GitHub

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 #3995

Merged
merged 3 commits into from
Jun 10, 2016
Merged

New feature: Non blocking tone queue #3995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b5aa15
Non-blocking buzzer
jbrazio Jun 4, 2016
a82c755
Fix BQ_LCD_SMART_CONTROLLER pin compiler warnings
thinkyhead Jun 10, 2016
d13c86f
Adds travis test for SPEAKER
jbrazio Jun 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,15 @@ script:
- opt_enable_adv DUAL_X_CARRIAGE
- build_marlin
#
# Test SPEAKER with BOARD_BQ_ZUM_MEGA_3D and BQ_LCD_SMART_CONTROLLER
#
- restore_configs
- opt_set MOTHERBOARD BOARD_BQ_ZUM_MEGA_3D
- opt_set LCD_FEEDBACK_FREQUENCY_DURATION_MS 10
- opt_set LCD_FEEDBACK_FREQUENCY_HZ 100
- opt_enable BQ_LCD_SMART_CONTROLLER SPEAKER
- build_marlin
#
### LCDS ###
#
#
Expand Down
30 changes: 23 additions & 7 deletions Marlin/Conditionals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@
#endif

#ifndef CONFIGURATION_LCD // Get the LCD defines which are needed first

#define CONFIGURATION_LCD
#define CONFIGURATION_LCD

#define LCD_HAS_DIRECTIONAL_BUTTONS (BUTTON_EXISTS(UP) || BUTTON_EXISTS(DWN) || BUTTON_EXISTS(LFT) || BUTTON_EXISTS(RT))

Expand Down Expand Up @@ -154,11 +153,6 @@
#define ENCODER_STEPS_PER_MENU_ITEM 1
#endif

#if ENABLED(LCD_USE_I2C_BUZZER)
#define LCD_FEEDBACK_FREQUENCY_HZ 1000
#define LCD_FEEDBACK_FREQUENCY_DURATION_MS 100
#endif

#define ULTIPANEL
#define NEWPANEL
#endif
Expand Down Expand Up @@ -806,5 +800,27 @@
#endif
#endif

/**
* Buzzer/Speaker
*/
#if ENABLED(LCD_USE_I2C_BUZZER)
#ifndef LCD_FEEDBACK_FREQUENCY_HZ
#define LCD_FEEDBACK_FREQUENCY_HZ 1000
#endif
#ifndef LCD_FEEDBACK_FREQUENCY_DURATION_MS
#define LCD_FEEDBACK_FREQUENCY_DURATION_MS 100
#endif
#elif PIN_EXISTS(BEEPER)
#ifndef LCD_FEEDBACK_FREQUENCY_HZ
#define LCD_FEEDBACK_FREQUENCY_HZ 5000
#endif
#ifndef LCD_FEEDBACK_FREQUENCY_DURATION_MS
#define LCD_FEEDBACK_FREQUENCY_DURATION_MS 2
#endif
#else
#ifndef LCD_FEEDBACK_FREQUENCY_DURATION_MS
#define LCD_FEEDBACK_FREQUENCY_DURATION_MS 2
#endif
#endif
#endif //CONFIGURATION_LCD
#endif //CONDITIONALS_H
11 changes: 11 additions & 0 deletions Marlin/Marlin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,4 +373,15 @@ extern uint8_t active_extruder;

void calculate_volumetric_multipliers();

// Buzzer
#if HAS_BUZZER
#if ENABLED(SPEAKER)
#include "speaker.h"
extern Speaker buzzer;
#else
#include "buzzer.h"
extern Buzzer buzzer;
#endif
#endif

#endif //MARLIN_H
41 changes: 29 additions & 12 deletions Marlin/Marlin_main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
#include "language.h"
#include "pins_arduino.h"
#include "math.h"
#include "buzzer.h"

#if ENABLED(USE_WATCHDOG)
#include "watchdog.h"
Expand Down Expand Up @@ -354,6 +353,15 @@ static millis_t stepper_inactive_time = (DEFAULT_STEPPER_DEACTIVE_TIME) * 1000UL
Stopwatch print_job_timer = Stopwatch();
#endif

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

static uint8_t target_extruder;

#if ENABLED(AUTO_BED_LEVELING_FEATURE)
Expand Down Expand Up @@ -1233,7 +1241,7 @@ inline bool code_value_bool() { return code_value_byte() > 0; }

#if ENABLED(TEMPERATURE_UNITS_SUPPORT)
inline void set_input_temp_units(TempUnit units) { input_temp_units = units; }

float code_value_temp_abs() {
switch (input_temp_units) {
case TEMPUNIT_C:
Expand Down Expand Up @@ -5689,10 +5697,13 @@ inline void gcode_M226() {
* M300: Play beep sound S<frequency Hz> P<duration ms>
*/
inline void gcode_M300() {
uint16_t beepS = code_seen('S') ? code_value_ushort() : 110;
uint32_t beepP = code_seen('P') ? code_value_ulong() : 1000;
if (beepP > 5000) beepP = 5000; // limit to 5 seconds
buzz(beepP, beepS);
uint16_t const frequency = code_seen('S') ? code_value_ushort() : 260;
uint16_t duration = code_seen('P') ? code_value_ushort() : 1000;

// Limits the tone duration to 0-5 seconds.
NOMORE(duration, 5000);

buzzer.tone(duration, frequency);
}

#endif // HAS_BUZZER
Expand Down Expand Up @@ -6173,7 +6184,7 @@ inline void gcode_M428() {
SERIAL_ERRORLNPGM(MSG_ERR_M428_TOO_FAR);
LCD_ALERTMESSAGEPGM("Err: Too far!");
#if HAS_BUZZER
buzz(200, 40);
buzzer.tone(200, 40);
#endif
err = true;
break;
Expand All @@ -6190,8 +6201,8 @@ inline void gcode_M428() {
report_current_position();
LCD_MESSAGEPGM(MSG_HOME_OFFSETS_APPLIED);
#if HAS_BUZZER
buzz(200, 659);
buzz(200, 698);
buzzer.tone(200, 659);
buzzer.tone(200, 698);
#endif
}
}
Expand Down Expand Up @@ -8076,17 +8087,23 @@ void idle(
bool no_stepper_sleep/*=false*/
#endif
) {
thermalManager.manage_heater();
lcd_update();
host_keepalive();
manage_inactivity(
#if ENABLED(FILAMENTCHANGEENABLE)
no_stepper_sleep
#endif
);
host_keepalive();
lcd_update();

thermalManager.manage_heater();

#if ENABLED(PRINTCOUNTER)
print_job_timer.tick();
#endif

#if HAS_BUZZER
buzzer.tick();
#endif
}

/**
Expand Down
57 changes: 0 additions & 57 deletions Marlin/buzzer.cpp
View file
Open in desktop

This file was deleted.

115 changes: 109 additions & 6 deletions Marlin/buzzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,114 @@
*
*/

#ifndef BUZZER_H
#define BUZZER_H
#ifndef __BUZZER_H__
#define __BUZZER_H__

#if HAS_BUZZER
void buzz(long duration, uint16_t freq);
#endif
#include "fastio.h"
#include "watchdog.h"
#include "circularqueue.h"

#endif //BUZZER_H
#define TONE_QUEUE_LENGTH 4

/**
* @brief Tone structure
* @details Simple abstration of a tone based on a duration and a frequency.
*
*/
struct tone_t {
uint16_t duration;
uint16_t frequency;
};

/**
* @brief Buzzer class
*/
class Buzzer {
private:
struct state_t {
tone_t tone;
uint32_t timestamp;
} state;

protected:
CircularQueue<tone_t, TONE_QUEUE_LENGTH> buffer;

/**
* @brief Inverts the sate of a digital PIN
* @details This will invert the current state of an digital IO pin.
*/
void invert() {
WRITE(BEEPER_PIN, !READ(BEEPER_PIN));
}

/**
* @brief Turn off a digital PIN
* @details Alias of digitalWrite(PIN, LOW) using FastIO
*/
void off() {
WRITE(BEEPER_PIN, LOW);
}

/**
* @brief Turn on a digital PIN
* @details Alias of digitalWrite(PIN, HIGH) using FastIO
*/
void on() {
WRITE(BEEPER_PIN, HIGH);
}

/**
* @brief Resets the state of the class
* @details Brings the class state to a known one.
*/
void reset() {
this->off();
this->state.timestamp = 0;
}

public:
/**
* @brief Class constructor
*/
Buzzer() {
SET_OUTPUT(BEEPER_PIN);
this->reset();
}

/**
* @brief Add a tone to the queue
* @details Adds a tone_t structure to the ring buffer, will block IO if the
* queue is full waiting for one slot to get available.
*
* @param duration Duration of the tone in milliseconds
* @param frequency Frequency of the tone in hertz
*/
void tone(uint16_t const &duration, uint16_t const &frequency = 0) {
while (buffer.isFull()) {
delay(5);
this->tick();
#if ENABLED(USE_WATCHDOG)
watchdog_reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@thinkyhead thinkyhead Jun 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@Blue-Marlin Blue-Marlin Jun 11, 2016 •
edited by thinkyhead
Loading

Choose a reason for hiding this comment

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

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() )

Copy link
Contributor

@jbrazio jbrazio Jun 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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..

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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);
           }

Copy link
Contributor

@Blue-Marlin Blue-Marlin Jun 11, 2016 •
edited by thinkyhead
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jbrazio jbrazio Jun 11, 2016 •
edited by thinkyhead
Loading

Choose a reason for hiding this comment

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

You're being square on purpose right ?

#endif
}
this->buffer.enqueue((tone_t) { duration, frequency });
}

/**
* @brief Loop function
* @details This function should be called at loop, it will take care of
* playing the tones in the queue.
*/
virtual void tick() {
if (!this->state.timestamp) {
if (this->buffer.isEmpty()) return;

this->state.tone = this->buffer.dequeue();
this->state.timestamp = millis() + this->state.tone.duration;
if (this->state.tone.frequency > 0) this->on();
}
else if (millis() >= this->state.timestamp) this->reset();
}
};

#endif
Loading