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

M300 doesn't work correctly #4394

Closed
BuddaBob opened this issue Jul 24, 2016 · 29 comments

Comments

@BuddaBob
Copy link

commented Jul 24, 2016

I have a problem with the M300 command. At the end of the print I use M300 to play a song like A-Team or Star Wars. This helps me to know when the printer is done. But now when I play the code, it plays only noise and not the song.

I tried it with the RC5, RC6 and RCbugfix Firmware. The stable 1.0.x works fine.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Jul 24, 2016

Have you enabled the SPEAKER option in Configuration.h?

@BuddaBob

This comment has been minimized.

Copy link
Author

commented Jul 25, 2016

I tried both.
With Speaker on, comes only one big noise.
With Speaker off comes some diffrent tones

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

@jbrazio This looks like a job for the tone master.

@jbrazio

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

Do you mind posting the gcode do I can test it on my system ?
Just slice a blank piece or a test cube.
Thanks.

@BuddaBob

This comment has been minimized.

Copy link
Author

commented Jul 25, 2016

I´m at Work now. So i can´t slice a file. But i can give you the End_code that i use.

M104 S0 ; turn off extruder
M140 S0 ; turn off bed
M127 ; Stopp Fan
G1 Z120
M84 ; disable motors
; A-Team
M300 S2489 P480
M300 S2093 P240
M300 S2489 P960
M300 S0 P120
M300 S1661 P240
M300 S2093 P480
M300 S1244 P720
M300 S0 P240
M300 S1567 P120
M300 S2093 P120
M300 S2489 P240
M300 S2093 P240
M300 S2793 P240
M300 S2489 P960
M300 S0 P120
M300 S2217 P360
M300 S2093 P120
M300 S2093 P120
M300 S1661 P360
M300 S2093 P960
@jbrazio

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

That is enough, thanks.
I'm not near my machine so I can't test it right now but looking at your G-Code I'd say the frequency is too high. Have a look on a test tune here, the frequencies (S argument) use smaller values of at least one order of magnitude.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

We used to use delay after starting a tone and would wait for it to stop before doing further provessing, but now we start the tone and keep on processing.

@jbrazio Could this be making it impossible to play higher frequencies without getting noise?

We might try a test where it inserts a full delay call for notes that are (a) short and (b) above a certain frequency.

But then, did we even make this change in RC5?

@jbrazio

This comment has been minimized.

Copy link
Member

commented Jul 26, 2016

I will try a couple of ideas I have and propose an improvement.
Do you have #define SPEAKER enabled or is it commented ?

Ah ! I now understand your question Scott.. 😓
This change was not implemented on RC5, not even on RC6..
It was implemented during the buildup for RC7.

@jbrazio

This comment has been minimized.

Copy link
Member

commented Jul 28, 2016

I've tested a lot of stuff..

To start Marlin idle() allows us to have a sample rate of about 6KHz, it's not ideal, it's not stable.. but still allows tones to play "buzz sounds" on a 3D printer (our aim is not to be the next Yamaha's beatbox hardware).

I started with some sort of latency compensation test, meaning I would keep testing for latency and adjust the period after each cycle to the latest latency value. I'ts not a complete job as I do not adjust duration for the increase/decrease period thus notes will come longer/shorter.. but I was just checking if the concept could work for us.

    /**
     * @brief Loop function
     * @details This function should be called at loop, it will take care of
     * playing the tones in the queue.
     *
     * Sketch uses 84,808 bytes (33%) of program storage space. Maximum is 253,952 bytes.
     * Global variables use 3,877 bytes (47%) of dynamic memory, leaving 4,315 bytes for
     * local variables. Maximum is 8,192 bytes.
     */
    virtual void tick() {
      static uint32_t before = micros();
      const  uint32_t now    = micros();

      this->latency -= latency >>3;
      this->latency += (now - before) >>3;
      before = now;

      if (!this->state.duration) {
        if (this->buffer.isEmpty()) return;

        this->reset();
        this->state.tone = this->buffer.dequeue();

        const uint32_t clock = 1 / (float)(this->latency / (float)1000000L);
        this->state.duration = (this->state.tone.duration * clock) / (float)1000;
        this->state.counter  = 1; // to force calculation on next tick()
      }
      else {
        if (--this->state.counter == 0) {
          if (this->state.tone.frequency > 0) this->invert();

          // Readjust for latency
          // T = (1 / f) / (1 / Clock) - 1 <=> T = (Clock / f) -1
          const uint32_t clock = 1 / (float)(this->latency / (float)1000000L);
          this->state.period   = clock / (float)this->state.tone.frequency -1;
          this->state.period >>= 1;
          this->state.counter  = this->state.period;
        }

        if (--this->state.duration == 0)
          this->off();
      }
    }

Then I asked myself why the hell are we reinventing the wheel ? Yes Arduino has it's own non-blocking tone() function.

    /**
     * @brief Loop function
     * @details This function should be called at loop, it will take care of
     * playing the tones in the queue.
     *
     * Sketch uses 86,456 bytes (34%) of program storage space. Maximum is 253,952 bytes.
     * Global variables use 3,905 bytes (47%) of dynamic memory, leaving 4,287 bytes for
     * local variables. Maximum is 8,192 bytes.
     */
    virtual void tick() {
      const uint32_t now = millis();

      if (now >= this->state.next) {
        if (this->buffer.isEmpty()) return;

        this->reset();
        this->state.tone = this->buffer.dequeue();
        this->state.next = now + this->state.tone.duration;
        ::tone(BEEPER_PIN, this->state.tone.frequency, this->state.tone.duration);
      }
    }

And lastly we have our existing code:

    /**
     * @brief Loop function
     * @details This function should be called at loop, it will take care of
     * playing the tones in the queue.
     *
     * Sketch uses 85,012 bytes (33%) of program storage space. Maximum is 253,952 bytes.
     * Global variables use 3,877 bytes (47%) of dynamic memory, leaving 4,315 bytes for
     * local variables. Maximum is 8,192 bytes.
     */
    virtual void tick() {
      if (!this->state.duration) {
        if (this->buffer.isEmpty()) return;

        this->reset();
        this->state.tone = this->buffer.dequeue();

        // Period is uint16, min frequency will be ~16Hz
        this->state.period = 1000000UL / this->state.tone.frequency;

        this->state.duration =
          (this->state.tone.duration * 1000L) / this->state.period;

        this->state.period   >>= 1;
        this->state.duration <<= 1;

      }
      else {
        const  uint32_t  now = micros();
        static uint32_t next = now + this->state.period;

        if (now >= next) {
          --this->state.duration;
          next = now + this->state.period;
          if (this->state.tone.frequency > 0) this->invert();
        }
      }
    }

The best sounding code block is the native tone() due to it's use of TIMER2 ISR thus breaking PWM output on PINS 3 and 11 (on boards other than the Mega). The next best one is our current code, and the least stable one is the latency compensation block.

What makes the buzzer sound bad is the incosistent time between tick()calls when we rely on the idle() function. that's the reason why the native tone() works better.

BUT

What really affects the "sound" quality is #define SPEAKER when the Piezzo Buzzer has its own oscillator circuit.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Jul 29, 2016

I see. Older versions of Marlin seem to have already had this sorted out, because back in the olden days of 2015 I never had to do anything special for my G3D_PANEL to play very nice sounding tones. Later I had to enable SPEAKER otherwise it would only play a monotone beep. That is still the case, as far as I know. I still need to enable SPEAKER to get any kind of tone. However, now all tones sound like a stir-fry of dying sparrows.

So we need to distinguish between those piezos that can only do a single tone, the piezos that can be given a pitch and have their own oscillator, and those which need software-based oscillation to achieve a tone. Personally, I think we should throw our custom oscillator in the trash.

@jbrazio

This comment has been minimized.

Copy link
Member

commented Jul 29, 2016

I would ditch our oscillator only because we cannot guarantee the sample rate and go with the native tone() and keep the simple buzzer/speaker defines as we have now. We're overlooking something which its main purpose is to play 200Hz sound when navigating the menu.

If you'd like to test on your G3D_PANEL before going live have a look at my branch.

@jbrazio

This comment has been minimized.

Copy link
Member

commented Jul 29, 2016

I have here a comparison on "how it sounds like" the different methods:
https://www.youtube.com/watch?v=tsKZx0vHJ2o

If you wish to switch from custom oscillator to tone() the PR is ready at #4456.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Jul 29, 2016

I'll be heading to the studio pretty soon and will follow up on this and any other testable issues in the hopper.

@jbrazio

This comment has been minimized.

Copy link
Member

commented Jul 30, 2016

One thing is certain.. both non-blocking sound better than the blocking one.
😆

@zanthor

This comment has been minimized.

Copy link

commented Sep 8, 2016

I just updated both my Taz5's to the RCBugfix branch and with #speaker defined I get no audio at all, without it the M300 respects duration but not frequency... used to respect frequency on the default Taz branch of Marlin...

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Sep 8, 2016

@zanthor What are the electronics? Is there an LCD controller?

@zanthor

This comment has been minimized.

Copy link

commented Sep 8, 2016

@thinkyhead #define REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Sep 8, 2016

Hmm. Last time I checked the SPEAKER setting was working for me with that controller. Unfortunately I don't have it handy to test with at this time.

@zanthor

This comment has been minimized.

Copy link

commented Sep 8, 2016

Is there anything I can do to help troubleshoot? I'm fully capable of following instructions :).

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Sep 10, 2016

@zanthor I'm not exactly sure where to begin. The code simply uses the tone function provided by the Arduino library to do a square wave in a PWM fashion. It's not really a part of Marlin at that point. One thing you could try is to stick the command noTone(BEEPER_PIN) into the buzzer class in buzzer.h. It's about the only thing I can come up with.

    void off() {
      WRITE(BEEPER_PIN, LOW);
      noTone(BEEPER_PIN);
    }
@zanthor

This comment has been minimized.

Copy link

commented Sep 14, 2016

No dice...

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

@zanthor For the TAZ5 the board seems to be RAMBo, so I presume you have set #define MOTHERBOARD BOARD_RAMBO. The BEEPER_PIN depends on the controller type. For VIKI it's pin 44. For other controllers it's 79. With no controller it's 33. So stick this code at the end of SanityCheck.h and when you try to build it will tell us how the beeper pin is being set:

#if BEEPER_PIN == 33
  #error "Beeper Pin 33"
#elif BEEPER_PIN == 44
  #error "Beeper Pin 44"
#elif BEEPER_PIN == 79
  #error "Beeper Pin 79"
#else
  #error "Beeper Pin ??"
#endif
@zanthor

This comment has been minimized.

Copy link

commented Sep 14, 2016

#error "Beeper Pin 79"

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

So, the pin appears to be correct, then. The piezo speaker is installed on the graphical display, and connected to a pin on AUX-4.

@jbrazio As the official beepmaster, do you have any ideas about this one?

@zanthor If you change the pin from 79 to 33 in pins_RAMBO.h it will use the speaker on the RAMBo instead. Does that result in tones working?

@zanthor

This comment has been minimized.

Copy link

commented Sep 14, 2016

Just to clarify - the buzzer WORKS on pin 79, but it doesn't change pitch...

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

@zanthor Yeah, I got that. Have you tried what I suggested?

@zanthor

This comment has been minimized.

Copy link

commented Sep 14, 2016

Yes I had tried 33 with no success.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

@zanthor

This comment has been minimized.

Copy link

commented Sep 23, 2016

I haven't abandoned this, I just got another big print order and haven't had time...

@thinkyhead thinkyhead closed this Oct 31, 2017

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