Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] 🔧 Revamp AUTO_FAN configuration #27214

Draft
wants to merge 19 commits into
base: bugfix-2.1.x
Choose a base branch
from

Conversation

thisiskeithb
Copy link
Member

@thisiskeithb thisiskeithb commented Jun 27, 2024

Description

Many pins files have *_AUTO_FAN_PIN(s) defined directly or with an #ifndef, but by setting *_AUTO_FAN_PIN(s) to -1 as default in a config, this will keep the pin unset or cause a wall of redefined warnings.

By disabling/commenting out these in the config by default, it'll allow board defaults to be used and is more in line with how pin overrides are done in the rest of the config.

Some other fixes/improvements:

  • Ensure that *_AUTO_FAN_PIN(s) & CONTROLLER_FAN_PIN(s) are wrapped in #ifndefs so they can be overridden.
  • Add E4_AUTO_FAN_PIN to Azteeg X3 Pro
  • Fix E0_AUTO_FAN_PIN on BOARD_CREALITY_F401RE (Ender-5 S1) since both the hotend & hotend cooling fan pin cannot be PA1. Official Creality Klipper configs list PC0 as the correct pin, but I cannot test this.
  • Fix / add Malyan M300 E0_AUTO_FAN_PIN since it was just "AUTO_FAN_PIN". Needs 🔧 Define Malyan M300 E0_AUTO_FAN_PIN directly Configurations#1074 for M300 test to pass.

Benefits

Allows board OEMs to specify *_AUTO_FAN_PIN(s) in pins files with easy overrides via configs.

@thisiskeithb thisiskeithb marked this pull request as draft June 27, 2024 01:06
@thisiskeithb thisiskeithb added this to the After 2.1.3 milestone Jun 27, 2024
@thisiskeithb thisiskeithb changed the title 🔧 Disable/comment out AUTO_FANs by default, allow more fan overrides [WIP] 🔧 Disable/comment out AUTO_FANs by default, allow more fan overrides Jun 27, 2024
@thisiskeithb thisiskeithb added the Needs: Work More work is needed label Jun 27, 2024
@thisiskeithb
Copy link
Member Author

thisiskeithb commented Jun 27, 2024

GTR test fails due to the following:

Marlin/src/module/temperature.cpp: In static member function 'static void Temperature::update_autofans()':
Marlin/src/module/temperature.cpp:1315:56: error: 'E3_AUTO_FAN_PIN' was not declared in this scope; did you mean 'E0_AUTO_FAN_PIN'?
1315 |   #define _EFANOVERLAP(I,N) ((I != N) && _FANOVERLAP(I,E##N))
     |                                                        ^
Marlin/src/module/../inc/../core/macros.h:643:24: note: in definition of macro 'EVAL1'
 643 | #define EVAL1(V...)    V
     |                        ^

edit: After testing this on an SKR V3 build with two extruders/hotends (to simplify what needs be enabled to test), you're forced to define each additionalE*_AUTO_FAN_PIN if you've enabled the first one.

@thisiskeithb
Copy link
Member Author

The "*_AUTO_FAN_PIN hack" commit is just that. I'm sure there's a better place or better way to unset all these pins, but it's basically what we're doing now in the config.

@thinkyhead
Copy link
Member

thinkyhead commented Jun 27, 2024

Keep in mind that this is one of the more "unusual" settings, and it hasn't been changed because it would require a bit of overhaul.

  • The extruder auto fans are always defined for you if the board has them (and you have extruders).
  • They should only be defined in the config if you want to DISABLE (-1) or REDEFINE (>= 0) an auto fan pin.
  • We set them as all disabled by default because it is an optional feature requiring an extra PWM fan connector.

We don't need to alter this configuration strategy right now, but consider how auto fans ought to be defined to be consistent with how other settings work. e.g., If you enable filament runout, the board pins file may provide you a default filament detect pin. If you have one or more extruders, the board may define En auto fan pins for you. There is no difference here in substance, except that we can disable these optional pins. At most you might add an option to enable/disable extruder auto fans as a whole, but this is covered now by selectively defining the auto fans you don't want as -1.

@thisiskeithb
Copy link
Member Author

thisiskeithb commented Jun 27, 2024

The extruder auto fans are always defined for you if the board has them (and you have extruders).

Not really. Using FAN1_PIN for E0_AUTO_FAN_PIN, FAN2_PIN for E1_AUTO_FAN_PIN, etc. is a common configuration, but this is missing from many pins files.

They should only be defined in the config if you want to DISABLE (-1) or REDEFINE (>= 0) an auto fan pin.

Right, but the main point of this PR is to make it more like other config-based pin overrides and have the options commented out instead of set to -1 by default since being set to -1 by default means that any auto fan set in the pins file is not used.

//#define CONTROLLER_FAN_PIN 148 // FAN2
//#define E0_AUTO_FAN_PIN 148 // FAN2
#ifndef E0_AUTO_FAN_PIN
#define E0_AUTO_FAN_PIN FAN1_PIN // FAN2
Copy link
Member

@thinkyhead thinkyhead Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been conscientiously defining more pins using the pin number rather than references to other defined pins, especially those that have an optional function w/r/t Marlin. This is in consideration of the possibility of some configuration variant that includes #undef FAN1_PIN (because why define a pin that's not needed?) or which redefines FAN1_PIN — either of which would then break the definition of E0_AUTO_FAN_PIN. Although in most cases fan pins do correspond well to the pins marked for fans on the board, that is not strictly the way fan pins must be defined.

Copy link
Member Author

@thisiskeithb thisiskeithb Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using FAN1_PIN for E0_AUTO_FAN_PIN, FAN2_PIN for E1_AUTO_FAN_PIN, etc. is a common configuration and has been around since as long as I have been using Marlin (since at least 2017), so it would be really tough to see that go away.

Copy link
Member

@thinkyhead thinkyhead Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defining pins in reference to other pins is fine when they are known to be unaltered and works best in the case of a configuration file. But pins files are only meant to define the best defaults for any board, while also leaving open the potential for overrides and reordering. Tying the default of one pin to the value of another, which might be modified, is bound to cause confusion, so I am inclined to avoid this inside of pins files going forward. A common internal default used only within a pins file is another matter, as individual pins files are free to define "private" pin names according to their AUX number, silkscreen name, etc.

@thisiskeithb
Copy link
Member Author

thisiskeithb commented Jun 27, 2024

I resubmitted the E0_AUTO_FAN_PIN change for the M300 config again because it is now unsafe due the M300 pins file defining an AUTO_FAN_PIN (which does nothing) instead of E0_AUTO_FAN_PIN: MarlinFirmware/Configurations#1075

It's fixed in this (#27214) PR, but this PR is in the "After 2.1.3" milestone due to it being low priority & needing work.

We don't want to leave the M300 config broken & unsafe since the hotend cooling fan no longer works.

EDIT:

I broke out the M300 E0_AUTO_FAN_PIN change to #27216 so we don't leave the hosted config in an unsafe state.

@thinkyhead
Copy link
Member

Thanks! The import-2.1.x branch is the best place to apply changes for a near-future version of the bugfix-2.1.x (or dev) branch, containing all proposed changes from ongoing PRs (so that active PRs can pass CI). So it's ok to submit the Malyan M300 changes early, and with the pins file updated this PR can continue to move forward.

@thisiskeithb
Copy link
Member Author

thisiskeithb commented Jun 28, 2024

thinkyhead modified the milestones: After 2.1.3, Version 2.1.3 Jun 27, 2024

This PR really was low priority and I'd still like see it wait until after 2.1.3 so we have as few bugs as possible.

@thinkyhead
Copy link
Member

The rest of the changes in this PR are good, so these pins can be overridden / disabled without compiler warnings. However, we need to leave these pin defines enabled and set to -1 in the default configuration because by default we don't want any auto fans except those enabled in the config explicitly (by the unusual convention of commenting them out).

If the aim is to use a more standard convention, we would do something more like this…

// Which extruders have auto fans?
//#define USE_E0_AUTO_FAN
//#define USE_E1_AUTO_FAN
//#define USE_E2_AUTO_FAN
//#define USE_E3_AUTO_FAN
//#define USE_E4_AUTO_FAN
//#define USE_E5_AUTO_FAN
//#define USE_E6_AUTO_FAN
//#define USE_E7_AUTO_FAN

// Provide overrides if needed. Leave undefined to use board defaults.
//#define E0_AUTO_FAN_PIN -1
//#define E1_AUTO_FAN_PIN -1
//#define E2_AUTO_FAN_PIN -1
//#define E3_AUTO_FAN_PIN -1
//#define E4_AUTO_FAN_PIN -1
//#define E5_AUTO_FAN_PIN -1
//#define E6_AUTO_FAN_PIN -1
//#define E7_AUTO_FAN_PIN -1

@thisiskeithb
Copy link
Member Author

Related to this, I think it'd be good to extend & rework/apply similar config options to FOURWIRES_FANS/*_FAN_TACHO_PINs. Simply defining *_FAN_TACHO_PINs in pins files enables the tachometer feature, which isn't ideal.

@thisiskeithb thisiskeithb changed the title [WIP] 🔧 Disable/comment out AUTO_FANs by default, allow more fan overrides [WIP] 🔧 Revamp AUTO_FAN configuration Jul 23, 2024
@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 3 times, most recently from 37d77d6 to aa44542 Compare September 28, 2024 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants