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

Allows user to set (almost) any PWM frequency #12638

Merged
merged 3 commits into from
Mar 8, 2019
Merged

Allows user to set (almost) any PWM frequency #12638

merged 3 commits into from
Mar 8, 2019

Conversation

poshcoe
Copy link
Contributor

@poshcoe poshcoe commented Dec 14, 2018

Requirements

  • 8-bit AVR microcontroller

Description

This feature changes the FAST_PWM_FAN implementation such that (almost) any reasonable PWM frequency can be achieved.

Combinations of Waveform Generation Modes, prescalers and TOP values (resolutions) are calculated internally to produce a frequency as close as possible to the user's desired frequency.

Implementation allows for changing the frequency at run-time, and using different frequencies across timers.

  • Should work on all timers, even TIMER2. (tested on TIMER2 and TIMER3 of AT90USB1286, needs testing on a chip with TCCR2)

Benefits

  • Enables further use cases to PWM control in Marlin
  • Enables setting PWM frequency outside of audible range (consider your pet's audible range also!)

Related Issues

@poshcoe poshcoe changed the title Fast pwm frequency Allows user to set (almost) any PWM frequency Dec 14, 2018
@marcio-ao
Copy link
Contributor

This is an interesting feature, but will it affect people who have their frequencies tuned "just right" but don't actually know what that frequency is? If a change is being made to the config files, it would be nice to have a way to know how to migrate from the old values to the new ones without having to get out an o-scope to figure out what the current frequencies are.

@poshcoe
Copy link
Contributor Author

poshcoe commented Dec 14, 2018

The current FAST_FAN_PWM mode actually leaves the timer in 8-bit phase-correct PWM mode (which is the default mode) - so it's not actually 'Fast PWM' in AVR-speak.
So when you define FAST_PWM_FAN, the current implementation simply sets the prescaler to 1.
In this mode the PWM frequency is calculated as F = F_CPU/(2 * N * TOP) where N (the prescaler) = 1 and TOP = 255.

Thus on 16MHz hardware, the current 'fast pwm' frequency is 31.37 KHz (or 39.217 KHz on 20 MHz).
I confirmed this (with a scope), measuring 31.4KHz on the pwm of my AT90USB1286.

Of course with this equation you could easily calculate your current frequency setting if you've done your own tweaking of the prescaler.

As you say, it would be a good idea to maintain these frequencies - I'll change the default in Conditionals_post.h to match them.

@thinkyhead thinkyhead force-pushed the bugfix-2.0.x branch 2 times, most recently from a32ec92 to a59d3d4 Compare January 3, 2019 23:07
@thinkyhead
Copy link
Member

thinkyhead commented Jan 6, 2019

I like the concept a lot, though I assume it is limited to AVR in the current implementation.

Side note… I decided to try writing a function to do the same, and to also set a duty cycle at the same time. The one here sets the base frequency and the duty cycle for Timer 5 A, B, and C for hardware-based PWM. There are some definite similarities.

/**
 * Set Timer 5 PWM frequency in Hz, from 3.8Hz up to ~16MHz
 * with a minimum resolution of 100 steps.
 *
 * DC values -1.0 to 1.0. Negative duty cycle inverts the pulse.
 */
uint16_t set_pwm_frequency_hz(const float &hz, const float dca, const float dcb, const float dcc) {
  float count = 0;
  if (hz > 0 && (dca || dcb || dcc)) {
    count = float(F_CPU) / hz;            // 1x prescaler, TOP for 16MHz base freq.
    uint16_t prescaler;                   // Range of 30.5Hz (65535) 64.5KHz (>31)

         if (count >= 255. * 256.) { prescaler = 1024; SET_CS(5, PRESCALER_1024); }
    else if (count >= 255. * 64.)  { prescaler = 256;  SET_CS(5,  PRESCALER_256); }
    else if (count >= 255. * 8.)   { prescaler = 64;   SET_CS(5,   PRESCALER_64); }
    else if (count >= 255.)        { prescaler = 8;    SET_CS(5,    PRESCALER_8); }
    else                           { prescaler = 1;    SET_CS(5,    PRESCALER_1); }

    count /= float(prescaler);
    const float pwm_top = round(count);   // Get the rounded count

    ICR5 = (uint16_t)pwm_top - 1;         // Subtract 1 for TOP
    OCR5A = pwm_top * ABS(dca);          // Update and scale DCs
    OCR5B = pwm_top * ABS(dcb);
    OCR5C = pwm_top * ABS(dcc);
    _SET_COM(5, A, dca ? (dca < 0 ? COM_SET_CLEAR : COM_CLEAR_SET) : COM_NORMAL); // Set compare modes
    _SET_COM(5, B, dcb ? (dcb < 0 ? COM_SET_CLEAR : COM_CLEAR_SET) : COM_NORMAL);
    _SET_COM(5, C, dcc ? (dcc < 0 ? COM_SET_CLEAR : COM_CLEAR_SET) : COM_NORMAL);

    SET_WGM(5, FAST_PWM_ICRn);            // Fast PWM with ICR5 as TOP

    //SERIAL_ECHOLNPGM("Timer 5 Settings:");
    //SERIAL_ECHOLNPAIR("  Prescaler=", prescaler);
    //SERIAL_ECHOLNPAIR("        TOP=", ICR5);
    //SERIAL_ECHOLNPAIR("      OCR5A=", OCR5A);
    //SERIAL_ECHOLNPAIR("      OCR5B=", OCR5B);
    //SERIAL_ECHOLNPAIR("      OCR5C=", OCR5C);
  }
  else {
    // Restore the default for Timer 5
    SET_WGM(5, PWM_PC_8);                 // PWM 8-bit (Phase Correct)
    SET_COMS(5, NORMAL, NORMAL, NORMAL);  // Do nothing
    SET_CS(5, PRESCALER_64);              // 16MHz / 64 = 250KHz
    OCR5A = OCR5B = OCR5C = 0;
  }
  return round(count);
}

@poshcoe
Copy link
Contributor Author

poshcoe commented Jan 6, 2019

I'm glad you like it! This is limited to AVR yes. I did see your function while working on this when you posted it in #7606, it is similar but a lot of the stuff in my implementation is to make it work across the AVR timers.

*/
#if ENABLED(FAST_PWM_FAN)
//#define FAST_PWM_FAN_FREQUENCY 31400
//#define USE_OCR2A_AS_TOP
Copy link
Member

Choose a reason for hiding this comment

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

I'm not in favor if this USE_OCR2A_AS_TOP setting. For one thing, it only applies to AVR. This option should be replaced by some kind of automated method to detect whether this would be the best thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it isn't ideal, but think it would be difficult for the software to make this choice for the user. Because TIMER2 is an 8-bit timer, the user will start to see significant drops in PWM control resolution (below the standard 0-255) to achieve their desired frequency. The user also loses timing capabilities on one of the TIMER2 outputs. Since this is a significant trade-off, I made it a user decision. Also, about it only applying to AVR - i'm not sure why you mention that when the majority of this feature would have to change to work with other micros.

@thinkyhead thinkyhead merged commit dbead66 into MarlinFirmware:bugfix-2.0.x Mar 8, 2019
@reloxx13
Copy link
Contributor

reloxx13 commented Mar 8, 2019

heya,
after this merge i get this:

Marlin\src\module\temperature.cpp: In static member function 'static Temperature::Timer Temperature::get_pwm_timer(pin_t)':
Marlin\src\module\temperature.cpp:1628:50: error: 'OCR3C' was not declared in this scope
/*OCRnQ*/   { &OCR3A,   &OCR3B,   &OCR3C},
^
Compiling .pioenvs\melzi\src\src\sd\SdFile.cpp.o
*** [.pioenvs\melzi\src\src\module\temperature.cpp.o] Error 1
 [ERROR] Took 16.51 seconds 

grafik

this is my config:
https://github.com/reloxx13/Marlin/tree/my-marlin/Marlin

after disabling FAST_PWM_FAN it compiles again.
enabling FAST_PWM_FAN & USE_OCR2A_AS_TOP does not fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants