[Review request] Support multiple printer fans

Developers
1

I’m working towards a less painful toolchanging suport in Klipper.
And this is the first self-contained step that seems to make sense.

This adds support for multiple part cooling fans controlled by M106, in a similar way that multiple extruders are currently supported.

Feedback and general thoughts on toolchange support appreciated.

2

Much silence. Friendly ping?

3

Do not take this as a discouragement of your work, but this specific pull request does add some code complexity, yet it claims that making the similar support through macros is quite complex. However, I’d say this should provide the same functionality:

[fan_generic fan_extruder]
pin: ...

[fan_generic fan_extruder1]
pin: ...


[gcode_macro M107]
rename_existing: M107.1
gcode:
  M106 S0
  
[gcode_macro M106]
rename_existing: M106.1
variable_active_fan: 'fan_extruder'
gcode:
  {% if params.S is not defined %}
  action_raise_error('M106 requires S parameter')
  {% endif %}
  SET_FAN_SPEED FAN={printer['gcode_macro M106'].active_fan} SPEED={params.S|float / 255.0}

[gcode_macro ACTIVATE_FAN]
gcode:
  {% if params.FAN is not defined %}
  action_raise_error('FAN parameter must be provided for ACTIVATE_FAN command')
  {% endif %}
  {% if 'fan_generic ' + params.FAN not in printer %}
  action_raise_error({'FAN=' + params.FAN + ' was not defined as fan_generic'})
  {% endif %}
  {% if params.FAN != printer['gcode_macro M106'].active_fan %}
  {% fan_speed = printer['fan_generic ' + printer['gcode_macro M106'].active_fan].speed %}
  M107
  SET_GCODE_VARIABLE MACRO=M106 VARIABLE=active_fan VALUE={params.FAN}
  M106 S{fan_speed * 255.0}
  {% endif %}

I didn’t test this myself, so I may have gotten some details a bit wrong, but you get the idea. And it appears to be simpler than the code changes in question. That’s just my 2 cents though.

2 Likes
4

Thanks for the input. Yes this can def be done by using macros. As can be almost everything short of core kinematics.

However this means overriding core gcodes and in my book should not happen on regular basis and is an indicator that their code should be updated instead.

I’m leaning towards that the core should support a certain set of setups without macros and as more marginal setups need increasingly more macros.

Fan is currently at a significant disadvantage when it comes to any multi extruder setup needing significantly more complex macro than other parts. Except perhaps MMU that is entirely different level.

I needed about 2 years of tinkering with Klipper to be able to write such a macro, so yes I consider that a complex one. I suppose we can add yours to the examples, but that means a user needs to:

  • know it exists, since it’s not documented
  • copy it into their config meaning no updates
  • deal with any conflicts to other macros they might choose to copy.

Adding this to core klipper means better documentation, testing and support for all potential users. I can buy and argument that this is too narrow of a use case, but not that there is no benefit over macros.

If code complexity is an issue, I can simplify it significantly at the expense of not introducing postfixed [fan] configs.

Sorry if I sound fired up about this. I’m looking for a bit more structured feedback:

  • is is type of change overall good fit for Klipper at this time, why?
  • is the ACTIVATE_FAN API reasonable. Should it accept the config name or fan name or both?
  • is the implementation as extras appropriate vs macro example - discussed here.
  • is allowing multiple [fan] configs a reasonable idea. It should we stick to fan_generic? Maybe only allow selecting between [fan] entries?
  • is the python implementation okay, any changes to be made there?