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

Fix Ultimainboard hotend fan #13296

Closed
wants to merge 1 commit into
base: bugfix-2.0.x
from

Conversation

Projects
None yet
4 participants
@svpcom
Copy link

svpcom commented Mar 3, 2019

Fix Ultimainboard hotend fan control (fan is on PJ6). Solution with PIN 77 doesn't work.

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Mar 3, 2019

It should work according to fastio1280.h

#define DIO77_PIN   PINJ6
#define DIO77_RPORT PINJ
#define DIO77_WPORT PORTJ
#define DIO77_DDR   DDRJ
#define DIO77_PWM   NULL
@svpcom

This comment has been minimized.

Copy link
Author

svpcom commented Mar 3, 2019

I've tried it on real hardware now

@boelle

This comment has been minimized.

Copy link
Contributor

boelle commented Mar 3, 2019

@svpcom what was the result of your test?

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Mar 3, 2019

Great. But this is not an acceptable solution. If pin 77 isn't mapping properly, that is what needs solving.

@svpcom

This comment has been minimized.

Copy link
Author

svpcom commented Mar 3, 2019

@boell Fan did't rotate. But all works with direct port access solution

@boelle

This comment has been minimized.

Copy link
Contributor

boelle commented Mar 3, 2019

@svpcom read what @thinkyhead just wrote

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Mar 3, 2019

The pin mapping appears to be in good working order. To get the full functionality you're attempting, you should revert your changes to temperature.cpp, then update your config and comment out these lines:

//#define E0_AUTO_FAN_PIN -1
//#define E1_AUTO_FAN_PIN -1
//#define E2_AUTO_FAN_PIN -1

…then update the pins file with:

#define ORIG_E0_AUTO_FAN_PIN 77
#define ORIG_E1_AUTO_FAN_PIN 77
#define ORIG_E2_AUTO_FAN_PIN 77
@svpcom

This comment has been minimized.

Copy link
Author

svpcom commented Mar 3, 2019

@thinkyhead I've tried it with //#define E0_AUTO_FAN_PIN -1 commented out in Configuration_adv.h

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Mar 3, 2019

Indeed, I have been following this closely.

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Mar 3, 2019

Trust me on this. Start out by changing SBI(DDRJ, 6) to SET_OUTPUT(77) and you will see it does the same thing. Then replace SBI(PORTJ, 6); else CBI(PORTJ, 6); with WRITE(77, HIGH); else WRITE(77, LOW); and you'll see that works too. So, if auto fans aren't coming on for you, then something else is going on.

@svpcom

This comment has been minimized.

Copy link
Author

svpcom commented Mar 3, 2019

@thinkyhead It seems that this code doesn't work properly with non-pwm output:

    uint8_t fanDone = 0;
    for (uint8_t f = 0; f < COUNT(fanPin); f++) {
      const pin_t pin =
        #ifdef ARDUINO
          pgm_read_byte(&fanPin[f])
        #else
          fanPin[f]
        #endif
      ;
      const uint8_t bit = pgm_read_byte(&fanBit[f]);
      if (pin >= 0 && !TEST(fanDone, bit)) {
        uint8_t newFanSpeed = TEST(fanState, bit) ? EXTRUDER_AUTO_FAN_SPEED : 0;
        #if ENABLED(AUTO_POWER_E_FANS)
          autofan_speed[f] = newFanSpeed;
        #endif
        // this idiom allows both digital and PWM fan outputs (see M42 handling).
        digitalWrite(pin, newFanSpeed);
        analogWrite(pin, newFanSpeed);
        SBI(fanDone, bit);
      }
    }
@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Mar 3, 2019

Try this:

        digitalWrite(pin, newFanSpeed);
-       analogWrite(pin, newFanSpeed);
+       if (PWM_PIN(pin)) analogWrite(pin, newFanSpeed);
@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Mar 3, 2019

See if #13298 allows the fans connected to digital pins to work properly.

@thinkyhead thinkyhead closed this Mar 3, 2019

@AnHardt

This comment has been minimized.

Copy link
Contributor

AnHardt commented Mar 3, 2019

        digitalWrite(pin, newFanSpeed);
-       analogWrite(pin, newFanSpeed);
+       if (PWM_PIN(pin)) analogWrite(pin, newFanSpeed);

It's the digitalWrite(pin, newFanSpeed); what can't work with Pin 77. That's based on the Arduino mapping - not on the FastIOs.

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Mar 5, 2019

Indeed. I misunderstood the issue initially to be some quirk of analogWrite messing with the write of the digital pin. (e.g., LOW<=127 and HIGH>=128 or LOW<255 HIGH==255). Anyway, the PWM_PIN test can't do any harm… can it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.