🐛 Fix, improve PWM on AVR (#23463)

Marlin/src/HAL/AVR/HAL.cpp

@@ -75,6 +75,8 @@ void HAL_init() {
   #if HAS_SERVO_3
     INIT_SERVO(3);
   #endif
+
+  init_pwm_timers();   // Init user timers to default frequency - 1000HZ
 }
 
 void HAL_reboot() {

Marlin/src/HAL/AVR/HAL.h

@@ -207,6 +207,7 @@ inline void HAL_adc_init() {
 #define strtof strtod
 
 #define HAL_CAN_SET_PWM_FREQ   // This HAL supports PWM Frequency adjustment
+#define PWM_FREQUENCY 1000     // Default PWM frequency when set_pwm_duty() is called without set_pwm_frequency()
 
 /**
  *  set_pwm_frequency
@@ -226,3 +227,9 @@ void set_pwm_frequency(const pin_t pin, const uint16_t f_desired);
  *  Optionally allows changing the maximum size of the provided value to enable finer PWM duty control [default = 255]
  */
 void set_pwm_duty(const pin_t pin, const uint16_t v, const uint16_t v_size=255, const bool invert=false);
+
+/*
+ * init_pwm_timers
+ * sets the default frequency for timers 2-5 to 1000HZ
+ */
+void init_pwm_timers();

Marlin/src/HAL/AVR/fast_pwm.cpp

@@ -21,10 +21,7 @@
  */
 #ifdef __AVR__
 
-#include "../../inc/MarlinConfigPre.h"
-#include "HAL.h"
-
-#if NEEDS_HARDWARE_PWM // Specific meta-flag for features that mandate PWM
+#include "../../inc/MarlinConfig.h"
 
 struct Timer {
   volatile uint8_t* TCCRnQ[3];  // max 3 TCCR registers per timer
@@ -32,6 +29,8 @@ struct Timer {
   volatile uint16_t* ICRn;      // max 1 ICR register per timer
   uint8_t n;                    // the timer number [0->5]
   uint8_t q;                    // the timer output [0->2] (A->C)
+  bool isPWM;                   // True if pin is a "hardware timer"
+  bool isProtected;             // True if timer is protected
 };
 
 // Macros for the Timer structure
@@ -53,16 +52,13 @@ struct Timer {
 #define _SET_ICRn(ICRn, V) (*(ICRn) = int(V) & 0xFFFF)
 
 /**
- * get_pwm_timer
- *  Get the timer information and register of the provided pin.
- *  Return a Timer struct containing this information.
- *  Used by set_pwm_frequency, set_pwm_duty
+ * Return a Timer struct describing a pin's timer.
  */
 Timer get_pwm_timer(const pin_t pin) {
+
   uint8_t q = 0;
 
   switch (digitalPinToTimer(pin)) {
-    // Protect reserved timers (TIMER0 & TIMER1)
     #ifdef TCCR0A
       IF_DISABLED(AVR_AT90USB1286_FAMILY, case TIMER0A:)
       case TIMER0B:
@@ -71,212 +67,147 @@ Timer get_pwm_timer(const pin_t pin) {
       case TIMER1A: case TIMER1B:
     #endif
 
-    break;
+    break;    // Protect reserved timers (TIMER0 & TIMER1)
 
     #if HAS_TCCR2
-      case TIMER2: {
-        Timer timer = {
-          { &TCCR2, nullptr, nullptr },
-          { (uint16_t*)&OCR2, nullptr, nullptr },
-            nullptr,
-            2, 0
-        };
-        return timer;
-      }
+      case TIMER2:
+        return Timer({ { &TCCR2, nullptr, nullptr }, { (uint16_t*)&OCR2, nullptr, nullptr }, nullptr, 2, 0, true, false });
+    #elif ENABLED(USE_OCR2A_AS_TOP)
+      case TIMER2A: break; // protect TIMER2A since its OCR is used by TIMER2B
+      case TIMER2B:
+        return Timer({ { &TCCR2A, &TCCR2B, nullptr }, { (uint16_t*)&OCR2A, (uint16_t*)&OCR2B, nullptr }, nullptr, 2, 1, true, false });
     #elif defined(TCCR2A)
-      #if ENABLED(USE_OCR2A_AS_TOP)
-        case TIMER2A:   break; // protect TIMER2A
-        case TIMER2B: {
-          Timer timer = {
-            { &TCCR2A,  &TCCR2B,  nullptr },
-            { (uint16_t*)&OCR2A, (uint16_t*)&OCR2B, nullptr },
-              nullptr,
-              2, 1
-          };
-          return timer;
-        }
-      #else
-        case TIMER2B: ++q;
-        case TIMER2A: {
-          Timer timer = {
-            { &TCCR2A,  &TCCR2B,  nullptr },
-            { (uint16_t*)&OCR2A, (uint16_t*)&OCR2B, nullptr },
-              nullptr,
-              2, q
-          };
-          return timer;
-        }
-      #endif
+      case TIMER2B: ++q; case TIMER2A:
+        return Timer({ { &TCCR2A, &TCCR2B, nullptr }, { (uint16_t*)&OCR2A, (uint16_t*)&OCR2B, nullptr }, nullptr, 2, q, true, false });
     #endif
 
     #ifdef OCR3C
-      case TIMER3C: ++q;
-      case TIMER3B: ++q;
-      case TIMER3A: {
-        Timer timer = {
-          { &TCCR3A,  &TCCR3B,  &TCCR3C },
-          { &OCR3A,   &OCR3B,   &OCR3C },
-            &ICR3,
-            3, q
-        };
-        return timer;
-      }
+      case TIMER3C: ++q; case TIMER3B: ++q; case TIMER3A:
+        return Timer({ { &TCCR3A, &TCCR3B, &TCCR3C }, { &OCR3A, &OCR3B, &OCR3C  }, &ICR3, 3, q, true, false });
     #elif defined(OCR3B)
-      case TIMER3B: ++q;
-      case TIMER3A: {
-        Timer timer = {
-          { &TCCR3A,  &TCCR3B,  nullptr },
-          { &OCR3A,   &OCR3B,  nullptr },
-            &ICR3,
-            3, q
-        };
-        return timer;
-      }
+      case TIMER3B: ++q; case TIMER3A:
+        return Timer({ { &TCCR3A, &TCCR3B, nullptr }, { &OCR3A, &OCR3B, nullptr }, &ICR3, 3, q, true, false });
     #endif
 
     #ifdef TCCR4A
-      case TIMER4C: ++q;
-      case TIMER4B: ++q;
-      case TIMER4A: {
-        Timer timer = {
-          { &TCCR4A,  &TCCR4B,  &TCCR4C },
-          { &OCR4A,   &OCR4B,   &OCR4C },
-            &ICR4,
-            4, q
-        };
-        return timer;
-      }
+      case TIMER4C: ++q; case TIMER4B: ++q; case TIMER4A:
+        return Timer({ { &TCCR4A, &TCCR4B, &TCCR4C }, { &OCR4A, &OCR4B, &OCR4C }, &ICR4, 4, q, true, false });
     #endif
 
     #ifdef TCCR5A
-      case TIMER5C: ++q;
-      case TIMER5B: ++q;
-      case TIMER5A: {
-        Timer timer = {
-          { &TCCR5A,  &TCCR5B,  &TCCR5C },
-          { &OCR5A,   &OCR5B,   &OCR5C },
-            &ICR5,
-            5, q
-        };
-        return timer;
-      }
+      case TIMER5C: ++q; case TIMER5B: ++q; case TIMER5A:
+        return Timer({ { &TCCR5A, &TCCR5B, &TCCR5C }, { &OCR5A, &OCR5B, &OCR5C }, &ICR5, 5, q, true, false });
     #endif
   }
 
-  Timer timer = {
-    { nullptr, nullptr, nullptr },
-    { nullptr, nullptr, nullptr },
-      nullptr,
-      0, 0
-  };
-  return timer;
+  return Timer();
 }
 
 void set_pwm_frequency(const pin_t pin, const uint16_t f_desired) {
   Timer timer = get_pwm_timer(pin);
-  if (timer.n == 0) return; // Don't proceed if protected timer or not recognized
-  uint16_t size;
-  if (timer.n == 2) size = 255; else size = 65535;
+  if (timer.isProtected || !timer.isPWM) return; // Don't proceed if protected timer or not recognized
+
+  const bool is_timer2 = timer.n == 2;
+  const uint16_t maxtop = is_timer2 ? 0xFF : 0xFFFF;
 
-  uint16_t res = 255;   // resolution (TOP value)
-  uint8_t j = 0;        // prescaler index
-  uint8_t wgm = 1;      // waveform generation mode
+  uint16_t res = 0xFF;        // resolution (TOP value)
+  uint8_t j = CS_NONE;        // prescaler index
+  uint8_t wgm = WGM_PWM_PC_8; // waveform generation mode
 
   // Calculating the prescaler and resolution to use to achieve closest frequency
   if (f_desired != 0) {
-    int f = (F_CPU) / (2 * 1024 * size) + 1; // Initialize frequency as lowest (non-zero) achievable
-    uint16_t prescaler[] = { 0, 1, 8, /*TIMER2 ONLY*/32, 64, /*TIMER2 ONLY*/128, 256, 1024 };
-
-    // loop over prescaler values
-    LOOP_S_L_N(i, 1, 8) {
-      uint16_t res_temp_fast = 255, res_temp_phase_correct = 255;
-      if (timer.n == 2) {
-        // No resolution calculation for TIMER2 unless enabled USE_OCR2A_AS_TOP
-        #if ENABLED(USE_OCR2A_AS_TOP)
-          const uint16_t rtf = (F_CPU) / (prescaler[i] * f_desired);
-          res_temp_fast = rtf - 1;
-          res_temp_phase_correct = rtf / 2;
+    constexpr uint16_t prescaler[] = { 1, 8, (32), 64, (128), 256, 1024 };  // (*) are Timer 2 only
+    uint16_t f = (F_CPU) / (2 * 1024 * maxtop) + 1;           // Start with the lowest non-zero frequency achievable (1 or 31)
+
+    LOOP_L_N(i, COUNT(prescaler)) {                           // Loop through all prescaler values
+      const uint16_t p = prescaler[i];
+      uint16_t res_fast_temp, res_pc_temp;
+      if (is_timer2) {
+        #if ENABLED(USE_OCR2A_AS_TOP)                         // No resolution calculation for TIMER2 unless enabled USE_OCR2A_AS_TOP
+          const uint16_t rft = (F_CPU) / (p * f_desired);
+          res_fast_temp = rft - 1;
+          res_pc_temp = rft / 2;
+        #else
+          res_fast_temp = res_pc_temp = maxtop;
         #endif
       }
       else {
-        // Skip TIMER2 specific prescalers when not TIMER2
-        if (i == 3 || i == 5) continue;
-        const uint16_t rtf = (F_CPU) / (prescaler[i] * f_desired);
-        res_temp_fast = rtf - 1;
-        res_temp_phase_correct = rtf / 2;
+        if (p == 32 || p == 128) continue;                    // Skip TIMER2 specific prescalers when not TIMER2
+        const uint16_t rft = (F_CPU) / (p * f_desired);
+        res_fast_temp = rft - 1;
+        res_pc_temp = rft / 2;
       }
 
-      LIMIT(res_temp_fast, 1U, size);
-      LIMIT(res_temp_phase_correct, 1U, size);
+      LIMIT(res_fast_temp, 1U, maxtop);
+      LIMIT(res_pc_temp, 1U, maxtop);
+
       // Calculate frequencies of test prescaler and resolution values
-      const int f_temp_fast = (F_CPU) / (prescaler[i] * (1 + res_temp_fast)),
-                f_temp_phase_correct = (F_CPU) / (2 * prescaler[i] * res_temp_phase_correct),
-                f_diff = ABS(f - f_desired),
-                f_fast_diff = ABS(f_temp_fast - f_desired),
-                f_phase_diff = ABS(f_temp_phase_correct - f_desired);
+      const uint32_t f_diff      = _MAX(f, f_desired) - _MIN(f, f_desired),
+                     f_fast_temp = (F_CPU) / (p * (1 + res_fast_temp)),
+                     f_fast_diff = _MAX(f_fast_temp, f_desired) - _MIN(f_fast_temp, f_desired),
+                     f_pc_temp   = (F_CPU) / (2 * p * res_pc_temp),
+                     f_pc_diff   = _MAX(f_pc_temp, f_desired) - _MIN(f_pc_temp, f_desired);
 
-      // If FAST values are closest to desired f
-      if (f_fast_diff < f_diff && f_fast_diff <= f_phase_diff) {
-        // Remember this combination
-        f = f_temp_fast;
-        res = res_temp_fast;
-        j = i;
+      if (f_fast_diff < f_diff && f_fast_diff <= f_pc_diff) { // FAST values are closest to desired f
         // Set the Wave Generation Mode to FAST PWM
-        if (timer.n == 2)
-          wgm = TERN(USE_OCR2A_AS_TOP, WGM2_FAST_PWM_OCR2A, WGM2_FAST_PWM);
-        else
-          wgm = WGM_FAST_PWM_ICRn;
+        wgm = is_timer2 ? uint8_t(TERN(USE_OCR2A_AS_TOP, WGM2_FAST_PWM_OCR2A, WGM2_FAST_PWM)) : uint8_t(WGM_FAST_PWM_ICRn);
+        // Remember this combination
+        f = f_fast_temp; res = res_fast_temp; j = i + 1;
       }
-      // If PHASE CORRECT values are closes to desired f
-      else if (f_phase_diff < f_diff) {
-        f = f_temp_phase_correct;
-        res = res_temp_phase_correct;
-        j = i;
+      else if (f_pc_diff < f_diff) {                          // PHASE CORRECT values are closes to desired f
         // Set the Wave Generation Mode to PWM PHASE CORRECT
-        if (timer.n == 2)
-          wgm = TERN(USE_OCR2A_AS_TOP, WGM2_PWM_PC_OCR2A, WGM2_FAST_PWM);
-        else
-          wgm = WGM_PWM_PC_ICRn;
+        wgm = is_timer2 ? uint8_t(TERN(USE_OCR2A_AS_TOP, WGM2_PWM_PC_OCR2A, WGM2_PWM_PC)) : uint8_t(WGM_PWM_PC_ICRn);
+        f = f_pc_temp; res = res_pc_temp; j = i + 1;
       }
     }
   }
+
   _SET_WGMnQ(timer.TCCRnQ, wgm);
   _SET_CSn(timer.TCCRnQ, j);
 
-  if (timer.n == 2) {
-    TERN_(USE_OCR2A_AS_TOP, _SET_OCRnQ(timer.OCRnQ, 0, res));  // Set OCR2A value (TOP) = res
+  if (is_timer2) {
+    TERN_(USE_OCR2A_AS_TOP, _SET_OCRnQ(timer.OCRnQ, 0, res)); // Set OCR2A value (TOP) = res
   }
   else
-    _SET_ICRn(timer.ICRn, res);         // Set ICRn value (TOP) = res
+    _SET_ICRn(timer.ICRn, res);                               // Set ICRn value (TOP) = res
 }
 
-#endif // NEEDS_HARDWARE_PWM
-
 void set_pwm_duty(const pin_t pin, const uint16_t v, const uint16_t v_size/*=255*/, const bool invert/*=false*/) {
-  #if NEEDS_HARDWARE_PWM
-
-    // If v is 0 or v_size (max), digitalWrite to LOW or HIGH.
-    // Note that digitalWrite also disables pwm output for us (sets COM bit to 0)
-    if (v == 0)
-      digitalWrite(pin, invert);
-    else if (v == v_size)
-      digitalWrite(pin, !invert);
-    else {
-      Timer timer = get_pwm_timer(pin);
-      if (timer.n == 0) return; // Don't proceed if protected timer or not recognized
-      // Set compare output mode to CLEAR -> SET or SET -> CLEAR (if inverted)
-      _SET_COMnQ(timer.TCCRnQ, timer.q TERN_(HAS_TCCR2, + (timer.q == 2)), COM_CLEAR_SET + invert); // COM20 is on bit 4 of TCCR2, so +1 for q==2
+  // If v is 0 or v_size (max), digitalWrite to LOW or HIGH.
+  // Note that digitalWrite also disables pwm output for us (sets COM bit to 0)
+  if (v == 0)
+    digitalWrite(pin, invert);
+  else if (v == v_size)
+    digitalWrite(pin, !invert);
+  else {
+    Timer timer = get_pwm_timer(pin);
+    if (timer.isProtected) return;                            // Leave protected timer unchanged
+    if (timer.isPWM) {
+      _SET_COMnQ(timer.TCCRnQ, SUM_TERN(HAS_TCCR2, timer.q, timer.q == 2), COM_CLEAR_SET + invert);   // COM20 is on bit 4 of TCCR2, so +1 for q==2
       const uint16_t top = timer.n == 2 ? TERN(USE_OCR2A_AS_TOP, *timer.OCRnQ[0], 255) : *timer.ICRn;
       _SET_OCRnQ(timer.OCRnQ, timer.q, uint16_t(uint32_t(v) * top / v_size)); // Scale 8/16-bit v to top value
     }
+    else
+      digitalWrite(pin, v < 128 ? LOW : HIGH);
+  }
+}
 
-  #else
-
-    analogWrite(pin, v);
-    UNUSED(v_size);
-    UNUSED(invert);
+void init_pwm_timers() {
+  // Init some timer frequencies to a default 1KHz
+  const pin_t pwm_pin[] = {
+    #ifdef __AVR_ATmega2560__
+      10, 5, 6, 46
+    #elif defined(__AVR_ATmega1280__)
+      12, 31
+    #elif defined(__AVR_ATmega644__) || defined(__AVR_ATmega1284__)
+      15, 6
+    #elif defined(__AVR_AT90USB1286__) || defined(__AVR_mega64) || defined(__AVR_mega128)
+      16, 24
+    #endif
+  };
 
-  #endif
+  LOOP_L_N(i, COUNT(pwm_pin))
+    set_pwm_frequency(pwm_pin[i], 1000);
 }
 
 #endif // __AVR__

Marlin/src/HAL/AVR/fastio.cpp

@@ -257,7 +257,7 @@ uint16_t set_pwm_frequency_hz(const_float_t hz, const float dca, const float dcb
     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
+    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

Marlin/src/inc/Conditionals_adv.h

@@ -677,11 +677,6 @@
   #define CUTTER_UNIT_IS(V)    (_CUTTER_POWER(CUTTER_POWER_UNIT) == _CUTTER_POWER(V))
 #endif
 
-// Add features that need hardware PWM here
-#if ANY(FAST_PWM_FAN, SPINDLE_LASER_USE_PWM)
-  #define NEEDS_HARDWARE_PWM 1
-#endif
-
 #if !defined(__AVR__) || !defined(USBCON)
   // Define constants and variables for buffering serial data.
   // Use only 0 or powers of 2 greater than 1

Marlin/src/module/stepper.cpp

@@ -3257,33 +3257,33 @@ void Stepper::report_positions() {
 
       #elif HAS_MOTOR_CURRENT_PWM
 
-        #define _WRITE_CURRENT_PWM(P) set_pwm_duty(pin_t(MOTOR_CURRENT_PWM_## P ##_PIN), 255L * current / (MOTOR_CURRENT_PWM_RANGE))
+        #define _WRITE_CURRENT_PWM_DUTY(P) set_pwm_duty(pin_t(MOTOR_CURRENT_PWM_## P ##_PIN), 255L * current / (MOTOR_CURRENT_PWM_RANGE))
         switch (driver) {
           case 0:
             #if PIN_EXISTS(MOTOR_CURRENT_PWM_X)
-              _WRITE_CURRENT_PWM(X);
+              _WRITE_CURRENT_PWM_DUTY(X);
             #endif
             #if PIN_EXISTS(MOTOR_CURRENT_PWM_Y)
-              _WRITE_CURRENT_PWM(Y);
+              _WRITE_CURRENT_PWM_DUTY(Y);
             #endif
             #if PIN_EXISTS(MOTOR_CURRENT_PWM_XY)
-              _WRITE_CURRENT_PWM(XY);
+              _WRITE_CURRENT_PWM_DUTY(XY);
             #endif
             break;
           case 1:
             #if PIN_EXISTS(MOTOR_CURRENT_PWM_Z)
-              _WRITE_CURRENT_PWM(Z);
+              _WRITE_CURRENT_PWM_DUTY(Z);
             #endif
             break;
           case 2:
             #if PIN_EXISTS(MOTOR_CURRENT_PWM_E)
-              _WRITE_CURRENT_PWM(E);
+              _WRITE_CURRENT_PWM_DUTY(E);
             #endif
             #if PIN_EXISTS(MOTOR_CURRENT_PWM_E0)
-              _WRITE_CURRENT_PWM(E0);
+              _WRITE_CURRENT_PWM_DUTY(E0);
             #endif
             #if PIN_EXISTS(MOTOR_CURRENT_PWM_E1)
-              _WRITE_CURRENT_PWM(E1);
+              _WRITE_CURRENT_PWM_DUTY(E1);
             #endif
             break;
         }
@@ -3302,34 +3302,37 @@ void Stepper::report_positions() {
 
       #elif HAS_MOTOR_CURRENT_PWM
 
+        #ifdef __SAM3X8E__
+          #define _RESET_CURRENT_PWM_FREQ(P) NOOP
+        #else
+          #define _RESET_CURRENT_PWM_FREQ(P) set_pwm_frequency(pin_t(P), MOTOR_CURRENT_PWM_FREQUENCY)
+        #endif
+        #define INIT_CURRENT_PWM(P) do{ SET_PWM(MOTOR_CURRENT_PWM_## P ##_PIN); _RESET_CURRENT_PWM_FREQ(MOTOR_CURRENT_PWM_## P ##_PIN); }while(0)
+
         #if PIN_EXISTS(MOTOR_CURRENT_PWM_X)
-          SET_PWM(MOTOR_CURRENT_PWM_X_PIN);
+          INIT_CURRENT_PWM(X);
         #endif
         #if PIN_EXISTS(MOTOR_CURRENT_PWM_Y)
-          SET_PWM(MOTOR_CURRENT_PWM_Y_PIN);
+          INIT_CURRENT_PWM(Y);
         #endif
         #if PIN_EXISTS(MOTOR_CURRENT_PWM_XY)
-          SET_PWM(MOTOR_CURRENT_PWM_XY_PIN);
+          INIT_CURRENT_PWM(XY);
         #endif
         #if PIN_EXISTS(MOTOR_CURRENT_PWM_Z)
-          SET_PWM(MOTOR_CURRENT_PWM_Z_PIN);
+          INIT_CURRENT_PWM(Z);
         #endif
         #if PIN_EXISTS(MOTOR_CURRENT_PWM_E)
-          SET_PWM(MOTOR_CURRENT_PWM_E_PIN);
+          INIT_CURRENT_PWM(E);
         #endif
         #if PIN_EXISTS(MOTOR_CURRENT_PWM_E0)
-          SET_PWM(MOTOR_CURRENT_PWM_E0_PIN);
+          INIT_CURRENT_PWM(E0);
         #endif
         #if PIN_EXISTS(MOTOR_CURRENT_PWM_E1)
-          SET_PWM(MOTOR_CURRENT_PWM_E1_PIN);
+          INIT_CURRENT_PWM(E1);
         #endif
 
         refresh_motor_power();
 
-        // Set Timer5 to 31khz so the PWM of the motor power is as constant as possible. (removes a buzzing noise)
-        #ifdef __AVR__
-          SET_CS5(PRESCALER_1);
-        #endif
       #endif
     }
 

Marlin/src/module/stepper.h

@@ -317,6 +317,10 @@ class Stepper {
         #ifndef PWM_MOTOR_CURRENT
           #define PWM_MOTOR_CURRENT DEFAULT_PWM_MOTOR_CURRENT
         #endif
+        #ifndef MOTOR_CURRENT_PWM_FREQUENCY
+          #define MOTOR_CURRENT_PWM_FREQUENCY 31400
+        #endif
+
         #define MOTOR_CURRENT_COUNT LINEAR_AXES
       #elif HAS_MOTOR_CURRENT_SPI
         static constexpr uint32_t digipot_count[] = DIGIPOT_MOTOR_CURRENT;