search

MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.26k stars 19.23k forks source link

[BUG] LASER_FEATURE inhibits CONTROLLERFAN_SPEED_ACTIVE setting #22291

Open I3megaS opened 3 years ago

I3megaS commented 3 years ago

Did you test the latest bugfix-2.0.x code?

Yes, and the problem still exists.

Bug Description

I was using CONTROLLERFAN_SPEED_ACTIVE at 190 and worked fine until I activate LASER FEATURE. I noticed that controller fan was not working as it should, and I realized that CONTROLLERFAN_SPEED_ACTIVE needs to be at 255 to work . If I use any other value controller fan stops turning. I am using different pins to control controller fan and laser. To control the laser I am using the servo pin D6 (PWM 5V) on Trigorilla board.

Readings

I realized that enabling LASER FEATURE PWM frequency changes. Maybe a scale error?

Bug Timeline

Issue was closed before being fixed on December 2020

Expected behavior

With or without LASER FEATURE function activated, controller fan should turn to the specified CONTROLLERFAN_SPEED_ACTIVE value.

Actual behavior

When activating LASER FEATURE controller fan only works if the CONTROLLERFAN_SPEED_ACTIVE value is 255.

Steps to Reproduce

  1. With LASER FEATURE disabled, change CONTROLLERFAN_SPEED_ACTIVE to 190.
  2. Test controller fan by moving any axis. It should work well.
  3. Now, activate LASER FEATURE and upload the changes to the printer.
  4. Test controller fan again. Fan doesn't turn.
  5. Change CONTROLLERFAN_SPEED_ACTIVE to 255 value. The controller fan starts turning.

Version of Marlin Firmware

Tested on FW 2.0.6.1, 2.0.7.1, 2.0.9, 2.0.9.1

Printer model

Anycubic I3 Mega S

Electronics

Stock motherboard

Add-ons

No response

Your Slicer

Cura

Host Software

No response

Additional information & file uploads

No response

ellensp commented 3 years ago

Provided config is broken "Configuration.h:1417:0: error: unterminated #if" missing #endif from line 1419 and line 924 is missing the start of a comment block /** and Configuration_adv.h has invalid line "#define EXTRUDER_AUTO_FAN_TEMPERATURE 60 /" either an additional / is needed or removal of this trailing /

There is no way you actually tested this config.

I3megaS commented 3 years ago

Sorry, I sent by mistake the files I was modifying now. The following one works.

WorkingConfigFiles(2.0.9.1).zip

ellensp commented 3 years ago

On your controller: pin 7 CONTROLLER_FAN_PIN, pin 6 SPINDLE_LASER_PWM_PIN With LASER_FEATURE disabled pin 7 PWM's at 490Hz With LASER_FEATURE enabled pin 7 PWM's at 10kHz

This is the same freq as SPINDLE_LASER_FREQUENCY is set to. If you comment out SPINDLE_LASER_FREQUENCY it works as expected with LASER_FEATURE enabled.

Pins 6,7 and 8 all use timer 4 (part of the hardware, unchangeable). This seem to be the cause,

I don't know if you can set different prescallers (freq ranges) for pin 6 and pin 7.. Also once this is set, setting the PWM value the normal way will no longer works as it should on the other pins.

ellensp commented 3 years ago

From further reading the prescaler value used to allow SPINDLE_LASER_FREQUENCY to do its thing is applied to pins 6,7 and 8. there is no way to avoid it. Hardware limitation.

ellensp commented 3 years ago

What I think this needs is when SPINDLE_LASER_FREQUENCY is enabled, the pin group that CONTROLLER_FAN_PIN belongs to, the other effected pins need to be switched from HARDWARE PWM to software PWM.

timer 0 (controls pin 13, 4); timer 1 (controls pin 12, 11); timer 2 (controls pin 10, 9); timer 3 (controls pin 5, 3, 2); timer 4 (controls pin 8, 7, 6); timer 5 (controls pin 46, 45, 44);

So they can stay at close to default rate as possible..

But this is beyond my capabilities. And may not even be possible.

I3megaS commented 3 years ago

@ellensp, thank you very much for your answer

I have been seeking information about ATmega 2560 and, if I am not wrong, it has different registers (OCRnX) in each timer (see following screenshot). So it could have different configurations in each output. But I don't know if this information is valid for all the architectures that Marlin can handle. I can't help you much more with this matter...

Timer1345

slowbro commented 3 years ago

Looking at the docs, it does indeed appear that the 16-bit timers (specifically in this case, Timer 4) have 3 output comparison units A, B, and C. These are connected individual output pins via OC4A/B/C registers and are all compared to the same timer, TCNT4.

Each out comparator can be in a different output mode, as controlled by COM4x[1:0] (where x is A/B/C). In Fast PWM mode, the "normal" operation will disconnect OC4B and OC4C and only output PWM on OC4A (i.e. pin 6). However, Fast PWM can be operated in "non-inverting" or "inverting" mode instead, by setting COM4x[1:0] to 1,0 or 1,1 respectively.

2021-07-05-140813_709x292_scrot

So, by selecting one of those modes, and allowing the timer to simply increment until overflowing to 0 and starting over, one could conceivably get three separate PWM duty cycles out of the same timer, but at the same PWM frequency. The frequency of the PWM would be controlled by the prescaler.

2021-07-05-141153_684x103_scrot

As an example, lets say we set up timer 4 as such:

// Set Port E timer pins as output.
DDRH |= ((1 << PH3) | (1 << PH4) | (1 << PH5)); // pins 6, 7, and 8

// TCCR4A – Timer/Counter 4 Control Register A
// Bit 7:6 – COM4A1:0: Compare Output Mode for Channel A: non-inverting
// Bit 5:4 – COM4B1:0: Compare Output Mode for Channel B: non-inverting
// Bit 3:2 – COM4C1:0: Compare Output Mode for Channel C: non-inverting
// Bit 1:0 – WGM4[1:0]: Waveform Generation Mode; see below
TCCR4A = 0b10101001;

// TCCR4B – Timer/Counter 4 Control Register B
// Bit 7 – ICNC4: Input Capture Noise Canceler: off
// Bit 6 – ICES4: Input Capture Edge Select: off
// Bit 5 – Reserved Bit
// Bit 4:3 – WGM4[3:2]: Waveform Generation Mode:
//   Combined with WGM4[0:1], we get 0101, which is
//    Fast PWM, 8 bit, OCR4x update at BOTTOM, TOV4 flag set at TOP
// Bit 2:0 – CSn2:0: Clock Select: 001, no prescaling
TCCR4B = 0b00001001;

// OCR4x and OCR4x – Output Compare Register 4 x [ABC]
// The Output Compare Registers contain a 16-bit value that is continuously compared
// with the counter value (TCNT4). A match can be used to generate an Output Compare
// interrupt, or to generate a waveform output on the OC4x pin.
//
// Perform an atomic 16-bit write to the registers. The compiler will handle 16-bit register
// translation.
unsigned char sreg;
sreg = SREG; // Save global interrupt flag
cli(); //disable interrupts

OCR4A = 128U; // 50% duty
OCR4B =  64U; // 25% duty
OCR4C = 192U; // 75% duty

SREG = sreg; // restore previous interrupt state

Seemingly, that would generate something like this:

2021-07-05-152934_863x645_scrot

With a TOP of 255 (8-bit) and CPU frequency of 16MHz, the PWM frequency would be 62.5kHz. It could be lowered by modifying the resolution of the PWM mode - 31.25kHz (9-bit) or 15.625 kHz (10-bit) - or, by adjusting the prescaler.

That's my understanding, at least. I believe I have an ATMega 1280 to test on, but not a 2560. Just kidding, I have a 2560. I'll see if I can validate this.

slowbro commented 3 years ago

Yep, seems to work as expected! Using this code:

#include <avr/io.h>
#include <avr/interrupt.h>

#define F_CPU 16000000UL

int main( void ) {
    // Set Port H timer pins as output.
    DDRH  |= ((1 << PH3) | (1 << PH4) | (1 << PH5)); // physical pins 24, 25, 26, arduino pins 6, 7, 8
    TCCR4A = 0b10101001;
    TCCR4B = 0b00001001;

    cli(); //disable interrupts

    OCR4A = 128U; // 50% duty
    OCR4B =  64U; // 25% duty
    OCR4C = 192U; // 75% duty                                                                                                                                                                                                                  

    sei();

    do{ } while(1);
}

I see this (ch1 - PH3, ch2 - PH4, ch3 - PH5):

NewFile1

slowbro commented 3 years ago

@I3megaS, can you try this patch? I think the issue here is that SPINDLE is using the timers as multi-channel for PWM, whereas CONTROLLERFAN is just doing the arduino-y analogWrite(pin, pwm).

diff --git a/Marlin/src/feature/controllerfan.cpp b/Marlin/src/feature/controllerfan.cpp
index 0206467752..1528ce113f 100644
--- a/Marlin/src/feature/controllerfan.cpp
+++ b/Marlin/src/feature/controllerfan.cpp
@@ -88,9 +88,14 @@ void ControllerFan::update() {
       ? settings.active_speed : settings.idle_speed
     );

-    // Allow digital or PWM fan output (see M42 handling)
-    WRITE(CONTROLLER_FAN_PIN, speed);
-    analogWrite(pin_t(CONTROLLER_FAN_PIN), speed);
+    #if NEEDS_HARDWARE_PWM
+      // use hardware PWM if it has been required by something else
+      set_pwm_duty(pin_t(CONTROLLER_FAN_PIN), speed);
+    #else
+      // Allow digital or PWM fan output (see M42 handling)
+      WRITE(CONTROLLER_FAN_PIN, speed);
+      analogWrite(pin_t(CONTROLLER_FAN_PIN), speed);
+    #endif
   }
 }
I3megaS commented 3 years ago

@slowbro, I have tested your patch and it works. I have tested with a laser frequency of 10kHz and now also the controller fan works at 10kHz. But the scaling is correctly executed on both outputs. The pity now is that controller fan buzzes at 10kHz. Maybe I can use another PWM output for the Laser control.

slowbro commented 3 years ago

Ah, that's too bad. What would be a normal/good frequency for fan PWM?

I'll take a more in-depth look into the changes above and submit a PR soon.

I3megaS commented 3 years ago

Yes is bad because the mosfet transistors will work at increased temperature. And in this pins (6, 7, 8) we have laser control, controller fan and heater bed respectively. The standard fan frequency before this change was 490Hz. Very reasonable for the mosfet in my opinion. In the file pins_Ramps.h (what applies for my board) the laser pins are defined as follows:

define SPINDLE_LASER_ENA_PIN 4 // Pullup or pulldown!

define SPINDLE_LASER_PWM_PIN 6 // Hardware PWM

define SPINDLE_DIR_PIN 5

If we interchange laser_pwm with spindle_dir_pin we will have the laser with timer 3 which I think it is not being used in another side. (at least in my printer)

define SPINDLE_LASER_ENA_PIN 4 // Pullup or pulldown!

define SPINDLE_LASER_PWM_PIN 5 // Hardware PWM

define SPINDLE_DIR_PIN 6

So in my opinion, this laser pins config should be at Configuration_adv.h with the recommendation of pin 5 to use a timer that is not being used.

I3megaS commented 3 years ago

I have tested the configuration mentioned above. I have commented the laser configuration pins in file pins_RAMPS.h

//#define SPINDLE_LASER_ENA_PIN 4 // Pullup or pulldown! //#define SPINDLE_LASER_PWM_PIN 6 // Hardware PWM //#define SPINDLE_DIR_PIN 5

And added to Configuration_adv.h the following pins configuration:

define SPINDLE_LASER_ENA_PIN 4 // Pullup or pulldown!

define SPINDLE_LASER_PWM_PIN 5 // Hardware PWM

define SPINDLE_DIR_PIN 6

This way, laser output PWM (pin 5) works well but controller fan (pin 7) only works when setting M710 to 255.

slowbro commented 3 years ago

Try reverting the patch I gave you from above; that will make it use software pwm again. Likely timer 3 is not initialized since there would be no call to set_pwm_frequency.

I3megaS commented 3 years ago

By reverting the patch it works well. Controller fan at 490Hz (pin 7) and laser control at 10kHz (pin 5) with PWM control. The best solution should be leaving all the PWM outputs done by hardware, shouldn't it? (all de cpu load we can save the better it will work)

slowbro commented 3 years ago

I would think so, but it may have been designed that way for a reason. At the very least, there should be some sanity checks that would prevent hardware pwm, software pwm, and analogWrite-style pwm from all stepping on eachother.

I3megaS commented 3 years ago

Ok, that's right. If you need any other test tell me. Thank you very much.

I3megaS commented 3 years ago

@slowbro, it seems that it was not so simple... I noticed that after powering on bed and hotend laser control pin changed PWM frequency to 8Hz, and I don't know how to restore 10kHz...

slowbro commented 3 years ago

Does it go back to 10kHz after power cycle, before turning on the hotend/bed? Likely the same problem is happening (overriding pins w/ hardware pwm)..

slowbro commented 3 years ago

timer 0 (controls pin 13, 4); timer 1 (controls pin 12, 11); timer 2 (controls pin 10, 9); timer 3 (controls pin 5, 3, 2); timer 4 (controls pin 8, 7, 6); timer 5 (controls pin 46, 45, 44);

this means...

So the heater bed pin is likely stepping on the spinder laser pwm pin (by default). You mentioned that you used pin 5 for the pwm pin... which should be on timer3 by itself. Do you have something different than above plugged into pin 2 or 3?

I3megaS commented 3 years ago

I don't know exactly when it has gone to 10Hz... I have checked pin configuration with M43 

**PIN:   2**   Port: E4        X_MAX_PIN                              protected
**PIN:   3**   Port: E5        X_MIN_PIN                              protected
.                          X_STOP_PIN                             protected
PIN:   4   Port: G5        SERVO3_PIN                             Output = 1    TIMER0B   PWM:   128    WGM: 3    COM0B: 3    CS: 3    TCCR0A: 3    TCCR0B: 3    TIMSK0: 5   compare interrupt enabled   overflow interrupt enabled
.                          SPINDLE_LASER_ENA_PIN                  Output = 1    TIMER0B   PWM:   128    WGM: 3    COM0B: 3    CS: 3    TCCR0A: 3    TCCR0B: 3    TIMSK0: 5   compare interrupt enabled   overflow interrupt enabled
**PIN:   5**   Port: E3        SERVO2_PIN                             Output = 0    TIMER3A   PWM:     0    WGM: 14    COM3A: 2    CS: 1    TCCR3A: 2    TCCR3B: 25    TIMSK3: 0
.                          SPINDLE_LASER_PWM_PIN                  Output = 0    TIMER3A   PWM:     0    WGM: 14    COM3A: 2    CS: 1    TCCR3A: 2    TCCR3B: 25    TIMSK3: 0

All seems to be well...

I3megaS commented 3 years ago

It doesn't go back to 10kHz. It still remains at 10Hz.

thinkyhead commented 3 years ago

Are you able to use PINS_DEBUGGING and review the output of M43 to ensure that there is no pin or timer conflict?

EDIT: Nevermind, after re-formatting I can read the output above.

thinkyhead commented 3 years ago

@slowbro — Yeah, it would be good to get a handle on timers and pins to really end the potential for conflicts. Sanity checks may help, but it is not always possible to check what external libraries are doing. Anyway, improvements in this area will be very valuable going forward.

descipher commented 3 years ago

Based on the board you have, did you try using FAN2 (pin44) as the controller fan? Seems like that would avoid the timer overlaps and give you the result you were looking for.

I3megaS commented 3 years ago

But in this board fan2 (pin44) is being used by hotend fan if I am not wrong...

descipher commented 3 years ago

Hmmm, could be but does it have to?

I3megaS commented 3 years ago

I think that pin45 is unused. Maybe I could swap functions of pin44 and pin45 and then use pin44 to work with controller fan. If it works, it wouldn't solve the firmware issue, only my specific problem. Maybe I could test the following week.

thisiskeithb commented 2 years ago

With https://github.com/MarlinFirmware/Marlin/pull/23125 merged, please download bugfix-2.0.x to test with the latest code and let us know if you're still having this issue.