Normalising Fan PWM power

[kubaracek]

What this does

It makes it possible to set 1% for a fan such as CPAP with a large off_below value and the fan will run at its lowest power at this setting

Why

Fans with quite high value for off_below (CPAP for example) are currently in klipper bit tricky to operate and people use several tricks either in slicer/gcode/macros to scale the values as requesting a 10% fan for cpap that might start at 16% or 30% gets ignored by klipper and fan stays off. When fan is properly configured in klipper and I request a fan to run at 1% of its speed I expect it to actually move, which is not the case currently. So the slicer/gcode needs to have a 'low level' knowledge of my hardware (when my fan actually will kick-in).

The value is actually currently partially scaled but between 0 and max_power. It's a bit odd.

How

This normalises the value between off_below and max_power.
When off_below would be set to 0.3 and max_power to 1.0 a 10% increment would have a value of 0.7 so requesting 10% of a fan will result in a value of off_below + 0.7 = 0.37 and not 0.1 as it does currently which effectively has no effect.

Normalizing the value according to fan setting such that the CPAP and similar fans will kick in even at 1% so the whole range can be used in same fashion as with any other fan where off_below=0

Note

These two notes bellow were addressed by renaming the parameter.

When testing this. Mainsail has a logic in place where they use off_below as a minimal setting for the fan using the fan slider, so setting the fan to say 2% will automatically set it to 30%
I will create a separate PR in Mainsail to remove this logic as it will no longer be needed.

After such update, users with off_below > 0 who will not change their M106 to lower the value will have their fans a bit louder, heh. They will generaly expect that their cpap might kick-in at around 16-30% so that's what they will have their lowest value set to in slicer and such. After the update it will be an actual 16-30% fan speed.

[JamesH1978]

Thank you for submitting a PR, please could you refer to point 3 in the "what to expect" section in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md and add a signed-off-by line.

Also you need to fix your whitespaces/length of line to pass build checks

Thanks
James

[kubaracek]

@JamesH1978 It should be fine now. Let me just test everything once more and I think it's ready unless you have some objections.

[Sineos]

You might want to cross check with:
#5360
#4720
#3937
#3208
#2165

[kubaracek]

@Sineos Thanks. I looked into the implementations and they modify a lot the 'input' value parameter which then stores scaled value as last_value. This would cause troubles for say Mainsail and other UIs as if you set a slider to say 10% it would get translated to 50% if that makes sense. So the slider would be just jumping around. I'm only changing what actually goes to self.mcu_fan.set_pwm function and leave the input value unchanged. Which also keeps the logic of kickstarting.

Renaming the value if we decide not to keep it to min_power would make sense though

I'm also inviting @dewi-ni-je for hopefully final discussion! As he seems to be the most vocal about the change :))
Ah I cannot invite him, maybe he's inactive?

[kubaracek]

I went though the discussions again. It doesn't seem like anybody has a problem with this per-se more so the way it was implemented that there were too many variables.

So do we agree to rename the off_power to min_power and scale the value between min_power and max_power while keeping the logic as I did -> no change to the value itself that is returned back but only scale the value that actually goes to the self.mcu_fan.set_pwm fn

Renaming the value has that added benefit that the user has to review the setting if it's set. Plus it deals with the off_bellow handling that mainsail is doing. I will create a separate PR to mainsail also to remove the dead code

[kdb424]

I would very much like to see a min/max power, and scale between them. Currently "50%" doesn't mean anything related to "50%" relevant to the fan, but relevant to pwm speed. For bare pins, that makes sense, but in the context of fans, I'd like to see 50% mean 50% of the available fan curve. Consider me onboard with this change in concept. Will actively be checking back to code review and do testing on my printers.

[Sineos]

There is only one person you need to convince and this is @KevinOConnor 😉

[kubaracek]

I've renamed the off_below to min_power and updated any references to it.

IMG_1371_MOV.mp4

[dewi-ny-je]

Isn't this something that can be implemented as macro? why a native way?

[kubaracek]

Isn't this something that can be implemented as macro? why a native way?

I wrote in description of the PR. I can't see any reason why in the context of Fan values below certain treshold should be ignored while M106 S255 being scaled to max_power such that the command will result in pwm of 0.7. What I'm configuring in klipper is a safe duty cycle range for my fan.
The client that communicates with klipper shouldn't have such a 'low level' - hardware knowledge of what's my safe fan range. Client requests min fan power, server delivers min fan power.

So the question to ask really is what use do you have for this not to be scaled.
So it's either not to scale any value at all or scale it fully. Now we have something in between.

So now, either we handle values above max_power the same way as we do with off_bellow == ignore them (or clip) OR we normalise the safe duty cycle range such that this PR suggest.

I'm "happy" to implemented a scale_pwm: true/false flag to the fan setting but frankly then we should remove the scaling that happens on the upper bond already.
But I can't really see a point in doing that. As what benefit would it have?

[Sineos]

IMO this is a good change:

• From a developer's perspective, it is clear that a PWM range is between 0 and 255, regardless of any capping at the upper or lower limits, and it is also clear to the developer that any capping will shift the resulting duty cycle.
• From a user's perspective, this is not very intuitive as the user sees it from the fan's perspective and expects 50% to be 50% fan speed and not something in between.

[freakydude]

... and in addition,

• it removes (or reduces) the slicer setting dependencies to the installed fans or their configuration

[kubaracek]

Indeed. Also searching for M106 the internet says Set Fan Speed not Set fan PWM. So PWM in this context should be abstracted away from the client

[github-actions]

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
   If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[kdb424]

Looks good to me. Workin as intended.

[github-actions]

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[trofen]

What if to leave both "off_below" and "min_power"? This will ensure backward compatibility and both parameters are still different in meaning and may be useful.

[surikatec]

Is this implemented in newest klipper version? (Trying that but can't find it.)

[JamesH1978]

@surikatec no this is an open PR, it is not implemented or merged

[github-actions]

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[github-actions]

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[github-actions]

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[github-actions]

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[freakydude]

What tasks are open here?

[KoenVanduffel]

It looks like this thing has stalled. I was looking for exactly this and manually copied the fan.py into my RatOs setup. Works like a charm.
A quick check gave me this result: with min_power: 0.2 and max power: 1 my 4 pin 4028 reports following rpm: 1% fan it runs 1200 rpm, 25% = 5600 rpm, 50% = 11000 rpm, 75% = 16100 rpm and 100% 24900 rpm. Great that is plenty linear (and probably the linearity of the fan itself).

[kubaracek]

Thanks for testing, it stalled on merging. There isn't anybody assigned to this with the rights to merge this unfortunately.
I don't think there is anything more to discuss or code for this. There's some other klipper clone project that merged this and from the community of cpap in Voron and RatRig there're people merging this themself to their self-built klipper.

[stablestud]

people merging this themself to their self-built klipper

And yes I'm unfortunately doing it aswell, shame that this simple feature is not in klipper...

@KoenVanduffel comment regarding developer and user perspective is very good and its confusing for most people. Marlin does that quite well with FAN_MIN_PWM and FAN_MAX_PWM

@KevinOConnor can you please merge this or #4720?
Looking at the amount of issues and opened PRs - this feature is quite in demand

[willpuckett]

Chiming in to support this PR. It seems this would align the way fans work with most users expected behhavior, and would simplify/reduce the need to explicitly configure slicer settings for low fan speeds.

In the event that some users wish to keep the current fan setup, perhaps a "scaled_fan" class could be added to wrap the regular fan and add the scaled fan functionality?