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

[BUG] Enabling both MPCTEMP and ADAPTIVE_FAN_SLOWING breaks build #25115

Closed
1 task done
LMBernardo opened this issue Dec 19, 2022 · 8 comments
Closed
1 task done

[BUG] Enabling both MPCTEMP and ADAPTIVE_FAN_SLOWING breaks build #25115

LMBernardo opened this issue Dec 19, 2022 · 8 comments

Comments

@LMBernardo
Copy link

LMBernardo commented Dec 19, 2022

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

Yes, and the problem still exists.

Bug Description

Building fails when enabling both MPCTEMP and ADAPTIVE_FAN_SLOWING. This combination is not mentioned anywhere in the MPCTEMP documentation, nor is it mentioned anywhere in the config files, and it seems like these features should be compatible.

I believe that this has gone unnoticed for so long because MPCTEMP is still noted as experimental and unreliable, and I don't think ADAPTIVE_FAN_SLOWING is a particularly common option to enable either. Most likely related to #24687.

Possible Solution

Starting at line 1168 in src/module/temperature.h, there is:

    #if ENABLED(MPCTEMP)
      void MPC_autotune();
    #endif

Marlin builds successfully when this is replaced with the following:

    #if ENABLED(MPCTEMP)
      void MPC_autotune();
      #if ENABLED(ADAPTIVE_FAN_SLOWING)
        static constexpr bool adaptive_fan_slowing = true;
      #endif
    #endif

If these features truly are incompatible, then this will cause other problems. But I don't see why they would be.

Bug Timeline

Old - this has probably existed ever since both features have been available.

Expected behavior

Marlin should either build successfully, or fail with a message stating that these two options cannot be used together. If the latter, this should also be documented somewhere.

Actual behavior

Build fails with the error:

Marlin\src\module\temperature.cpp: In member function 'void Temperature::tr_state_machine_t::run(const_celsius_float_t, const_celsius_float_t, heater_id_t, uint16_t, celsius_t)':
Marlin\src\module\temperature.cpp:2872:15: error: 'adaptive_fan_slowing' was not declared in this scope
           if (adaptive_fan_slowing && heater_id >= 0) {
               ^~~~~~~~~~~~~~~~~~~~
*** [.pio\build\mega2560\src\src\module\temperature.cpp.o] Error 1

Steps to Reproduce

  1. Checkout most the recent Marlin bugfix-2.1.x code @ fb5a595

  2. Disable PIDTEMP and enable MPCTEMP in Configuration.h

  3. Enable ADAPTIVE_FAN_SLOWING in Configuration_adv.h

  4. Try to build

Version of Marlin Firmware

bugfix-2.1.x

Printer model

N/A

Electronics

N/A

Add-ons

N/A

Bed Leveling

None

Your Slicer

None

Host Software

None

Don't forget to include

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

Additional information & file uploads

Config.zip

@GMagician
Copy link
Contributor

AFAIK MPC should take care of fan and then adaptive fan shouldn't be necessary.
But let's wait for someone who has knowledge about it

@thisiskeithb
Copy link
Member

AFAIK MPC should take care of fan and then adaptive fan shouldn't be necessary.

With MPC_INCLUDE_FAN enabled, ADAPTIVE_FAN_SLOWING shouldn't be necessary, but it's not specifically sanity checked.

@tombrazier
Copy link
Contributor

I have always had ADAPTIVE_FAN_SLOWING enabled along with MPC and no compile error.

@thisiskeithb
Copy link
Member

I have always had ADAPTIVE_FAN_SLOWING enabled along with MPC and no compile error.

I would guess something broke it recently.

Using a fresh bugfix-2.1.x pull & disabling PIDTEMP, enabling MPCTEMP & ADAPTIVE_FAN_SLOWING produces the error:

Indexing .pio/build/mega2560/libFrameworkArduino.a
Marlin/src/module/temperature.cpp: In member function 'void Temperature::tr_state_machine_t::run(const_celsius_float_t, const_celsius_float_t, heater_id_t, uint16_t, celsius_t)':
Marlin/src/module/temperature.cpp:2893:15: error: 'adaptive_fan_slowing' was not declared in this scope
           if (adaptive_fan_slowing && heater_id >= 0) {
               ^~~~~~~~~~~~~~~~~~~~
*** [.pio/build/mega2560/src/src/module/temperature.cpp.o] Error 1

@tombrazier
Copy link
Contributor

I do not get the compile error because I have PIDTEMPBED defined. The problem is not with enabling MPCTEMP, it is with disabling PIDTEMP. The same happens if you disable PIDTEMP for bang bang control.

The following code incorrectly couples ADAPTIVE_FAN_SLOWING with PID.

#if ENABLED(NO_FAN_SLOWING_IN_PID_TUNING)
static bool adaptive_fan_slowing;
#elif ENABLED(ADAPTIVE_FAN_SLOWING)
static constexpr bool adaptive_fan_slowing = true;
#endif

@tombrazier
Copy link
Contributor

tombrazier commented Jan 28, 2023

I think https://github.com/tombrazier/Marlin/tree/25115 should fix it. Haven't tried it though.

@thisiskeithb
Copy link
Member

Closing since there's now a PR fix this:

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants