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

[jjaeggli]

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.

MarlinFirmware#23048

And introduced a similar issue in other boards as can be seen by MarlinFirmware#23094

A subsequent change MarlinFirmware#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

1. Flash printer firmware to RC41 or above.
2. Execute M106 S255 => fan turns on at full speed
3. 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

• A ZIP file containing your Configuration.h and Configuration_adv.h.

Additional information & file uploads

...

[marciot]

I have determined that the following commit introduces the problem:

da830e6

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]

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]

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]

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]

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]

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]

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]

I have created a PR for a fix in upstream MarlinFirmware#25005