[BUG][TAZ6] PWM fan control not working in RC41+

jjaeggli commented 1 year ago

Did you test the latest `bugfix-2.1.x` code?

I have not yet tested the configuration against the base / non-drunken-octopus branch.

Bug Description

This issue has been tested on Oliveoil_TAZ6/Tilapia_SingleExtruder

In RC41 and above, the part cooling fan only works at full speed. Setting the fan with M106 S255 correctly sets the fan to full speed. M106 S254 stops the fan entirely. When reflashing the board back to RC40, variable fan speed is possible through commands such as M106 S128. This issue has been tested up to RC52. The timing of the issue allows us to correlate this with an upstream change to the Marlin firmware which occurred between RC40 and RC41.

https://github.com/MarlinFirmware/Marlin/pull/23048

And introduced a similar issue in other boards as can be seen by https://github.com/MarlinFirmware/Marlin/issues/23094

A subsequent change https://github.com/MarlinFirmware/Marlin/pull/23102

While it may not be the correct or ideal fix, enabling FAN_SOFT_PWM in the Oliveoil_TAZ6/Tilapia_SingleExtruder config resolves the issue and allows variable fan speeds in RC52.

I've not yet verified this issue with the stock marlin 2.1.x branch

Bug Timeline

RC41+

Expected behavior

Fan control should be variable throughout the duty cycle range - M106 S128 should set the fan to %50. M106 S254 should set the fan to %99.6 of full speed.

Actual behavior

Any value below 255 will cause the fan to stop.

Steps to Reproduce

Flash printer firmware to RC41 or above.
Execute M106 S255 => fan turns on at full speed
Execute M106 S254 => fan stops

Version of Marlin Firmware

https://github.com/drunken-octopus/drunken-octopus-marlin/releases/tag/v2.1.x-rc52

Printer model

LULZBOT TAZ6

Electronics

Stock RAMBo (1.3?)

Add-ons

No response

Bed Leveling

ABL Bilinear mesh

Your Slicer

Prusa Slicer

Host Software

Pronterface

Don't forget to include

[X] A ZIP file containing your Configuration.h and Configuration_adv.h.

Additional information & file uploads

...

marciot commented 1 year ago

I have determined that the following commit introduces the problem:

https://github.com/drunken-octopus/drunken-octopus-marlin/commit/da830e6ced7f7c7e509e748104245064d1c1b265

I will further note that the configuration files used by LulzBot printers will enable FAST_PWM_FAN and set FAST_PWM_FAN_FREQUENCY to 122, as explained here:

    # For the Pelonis C4010L24BPLB1b-7 fan, we need a relative low
    # PWM frequency of about 122Hz in order for the fan to turn.

    if USE_BTT_002 or USE_ARCHIM2:
        # On the Archim, it is necessary to use soft PWM to get the
        # frequency down in the kilohertz
        MARLIN["FAN_SOFT_PWM"]                           = True
    else:
        # By default, FAST_PWM_FAN_FREQUENCY sets PWM to ~31kHz,
        # but we want to lower this to 122 Hz.
        MARLIN["FAST_PWM_FAN"]                           = True
        MARLIN["FAST_PWM_FAN_FREQUENCY"]                 = 122

So, while enabling FAN_SOFT_PWM might resolve the problem, it could potentially cause issues with the particular fan mentioned.

marciot commented 1 year ago

In commit da830e6 in Marlin\src\HAL\AVR\fast_pwm.cpp, the following line was changed from:

_SET_OCRnQ(timer.OCRnQ, timer.q, v * float(top) / float(v_size)); // Scale 8/16-bit v to top value

To:

_SET_OCRnQ(timer.OCRnQ, timer.q, (v * top + v_size / 2) / v_size); // Scale 8/16-bit v to top value

I assume this was meant as a refactoring step, but algebraically, it is incorrect.

UPDATE: A fix to this was provided by additional work in commit 125310d

marciot commented 1 year ago

It looks like additional work may have been done in commit 125310d, which fixed the bug I had posted about above. I now tested a lot of additional intermediate commits, as such:

bc9479e - Oct 15th - PWM Okay cd0ee8c - Oct 17th - PWM Okay 67b075f - Oct 29th - PWM Okay 18a924d - Oct 29th - PWM Okay be412e3 - Oct 30th - PWM Okay 767a15d - Nov 1st - PWM Okay da830e6 - Nov 1st - Bug introduced in fast_pwm dfdffca - Nov 6th, - Broken Nov 9th - Drunken Octopus R41 - Broken 4a840e1 - Nov 12th - Broken 125310d - Nov 14th - fast_pwm bug fixed 89fe5f6 - Nov 16th - PWM Okay 36173fb - Nov 16th - PWM Okay 430a4ae - Nov 22nd - PWM Okay b6a8cc0 - Nov 23rd - PWM Okay 09ee63c - Nov 27th - PWM Okay 87dd245 - Nov 27th - PWM Okay 63d73bf - Dec 6th - PWM Okay 7ab9628 - Dec 7th - PWM Okay 82d569f - Dec 12th - PWM Okay c80ef71 - Dec 22nd - PWM Okay bdb0716 - Dec 25th - Won't upload 7d46291 - Dec 26th - PWM Okay 761b1b9 - Dec 27th - PWM Okay 6d1eaa0 - Dec 28th - Won't compile due to dwin_lcd error 4de8403 - Dec 31st - Won't compile due to dwin_lcd error 08aacb8 - Jan 1st - Won't compile due to dwin_lcd error 15936d3 - Jan 5th - Won't compile due to dwin_lcd error 3eb794d - Jan 6th - Won't compile due to dwin_lcd error 43ebd23 - Jan 7th - Won't compile due to dwin_lcd error 9564d68 - Jan 7th - Won't compile due to dwin_lcd error 0dfc17c - Jan 9th - Won't compile due to dwin_lcd error 476028d - Jan 10th - Won't compile due to dwin_lcd error 89f1391 - Jan 11th - Won't compile due to dwin_lcd error a058a8a - Jan 12th - Compiles; PWM Broken 9086082 - Jan 12th - PWM Broken Jan 18th - Drunken Octopus R42 - Broken

So right now, the latest commit with PWM verified working is 761b1b9. From that point on, something gets broken in the dwin lcd code which doesn't allow me compile until commit 9086082, at which point the PWM is broken again.

So it looks like the PWM was broken somewhere between December 27th and January 12th, but I had to fix the dwin_lcd stuff before I can narrow it down any further

marciot commented 1 year ago

Okay, I've conducted a few more tests:

6d1eaa0 - Dec 28th - Won't compile; cherry pick a058a8a; PWM Okay 4de8403 - Dec 31st - Won't compile; cherry pick a058a8a; PWM Okay 08aacb8 - Jan 1st - Won't compile; cherry pick a058a8a; PWM Okay 15936d3 - Jan 5th - Won't compile; cherry pick a058a8a; PWM Okay 3eb794d - Jan 6th - Won't compile; cherry pick a058a8a; PWM Okay 43ebd23 - Jan 7th - Won't compile 9564d68 - Jan 7th - Won't compile 0dfc17c - Jan 9th - Won't compile; cherry pick a058a8a; PWM Okay 476028d - Jan 10th - Won't compile; cherry pick a058a8a; PWM Okay 02b29c0 - Jan 10th - Won't compile; cherry pick a058a8a; PWM Okay 07bffdf - Jan 10th - Won't compile; cherry pick a058a8a; PWM broken 89f1391 - Jan 11th - Won't compile; cherry pick a058a8a; PWM broken

At this point, it appears as if commit 07bffdf caused yet another regression in PWM.

marciot commented 1 year ago

Okay, interesting. With commit 07bffdf, the PWM frequency became totally out of whack, but with the alleged fix in commit 2cfde39, the PWM stopped working altogether.

marciot commented 1 year ago

Okay, I have managed to narrow down an error in commit 07bffdf. The prototype of the following function was changed from:

void set_pwm_frequency(const pin_t pin, int f_desired)

To:

void set_pwm_frequency(const pin_t pin, const uint16_t f_desired)

This causes the following expressions later in the function to perform a subtraction of unsigned values, which gives incorrect values:

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

After changing the function prototype, it would also be necessary to change the expressions to:

                f_diff = ABS(f - int(f_desired)),
                f_fast_diff = ABS(f_temp_fast - int(f_desired)),
                f_phase_diff = ABS(f_temp_phase_correct - int(f_desired));

I now need to see how this code was subsequently modified in commit 2cfde39 to see whether the fix is still applicable.

marciot commented 1 year ago

So I've gone through commit 2cfde39 line-by-line and it looks like the breaking change is indeed a sign error of some sort. Here is the non-working code:

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

This is code that works, based on previous commits:

      const int f_diff = ABS(f - int(f_desired)),
                f_fast_temp = (F_CPU) / (p * (1 + res_fast_temp)),
                f_fast_diff = ABS(f_fast_temp - int(f_desired)),
                f_pc_temp = (F_CPU) / (2 * p * res_pc_temp),
                f_pc_diff = ABS(f_pc_temp - int(f_desired));

I still haven't gotten my head around why this makes a difference.

marciot commented 1 year ago

I have created a PR for a fix in upstream https://github.com/MarlinFirmware/Marlin/pull/25005

drunken-octopus / drunken-octopus-marlin