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] Unconditionnal definition of SUICIDE_PIN for BOARD_RAMPS_CREALITY, leading to issue with PSU_CONTROL #27142

Closed
1 task done
TheRaf974 opened this issue Jun 1, 2024 · 3 comments · Fixed by #27143
Closed
1 task done

Comments

@TheRaf974
Copy link
Contributor

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

Yes, and the problem still exists.

Bug Description

I have a CR-10 S5 with a Creality mainboard v2.2. It runs on Marlin 2.1.2.3 with BOARD_RAMPS_CREALITY

I was setting up PSU_CONTROL to turn off my printer on M81 using a static relay located before my power supply unit.

To use PSU_CONTROL, a PS_ON_PIN is required. The default pin 40, in pins_RAMPS CREALITY.h isn't accessible in my board. The CR2020 must have a specific board with pin 40 exposed, which is not my case.

#ifndef PS_ON_PIN
  #define PS_ON_PIN                           40  // Used by CR2020 Industrial series
#endif

So, I added #define PS_ON_PIN EXP4_PIN in my Configuration.h, based on pins_RAMPS CREALITY.h. Comments suggest that PS_ON_PIN could be use on pin 12, which seems much more logical than pin 40.

#define EXP1_PIN                              65  // A11 - Used by CR2020 Industrial series for case
#define EXP2_PIN                              66  // A12
#define EXP3_PIN                              11  // SERVO0_PIN
#define EXP4_PIN                              12  // PS_ON_PIN

At this time, PSU_CONTROL comportement was inversed (M80 turned off and M81 turned on) no matter what was the value of PSU_ACTIVE_STATE. After digging a bit, I found that in M80_M81.cpp for M81() (turn off), the usage of suicide() is prioritary to powerManager.power_off_soon():

  #if HAS_SUICIDE
    suicide();
  #elif ENABLED(PSU_CONTROL)
    powerManager.power_off_soon();
  #endif

For M80() (turn on), this time after powerManager.power_on(), it turn low the SUICIDE_PIN:

powerManager.power_on();

    /**
     * If you have a switch on suicide pin, this is useful
     * if you want to start another print with suicide feature after
     * a print without suicide...
     */
    #if HAS_SUICIDE
      OUT_WRITE(SUICIDE_PIN, !SUICIDE_PIN_STATE);
    #endif

PSU_CONTROL comportement was inversed because :

  1. SUICIDE_PIN is unconditionnally defined to pin 12 (the one I use)
  2. the logic of suicide is the opposite of the one I was searching for
  3. suicide was prioritary to PSU_CONTROL

This unconfitionnal definition sounds problematic to me, as it seems to be specific to one printer among the tens of creality's printer.

#define SUICIDE_PIN                           12  // Used by CR2020 Industrial series
#ifndef SUICIDE_PIN_STATE
  #define SUICIDE_PIN_STATE                 HIGH
#endif

I solved my problem by replacing this part with this

#ifndef SUICIDE_PIN
  #define SUICIDE_PIN                           12  // Used by CR2020 Industrial series
  #ifndef SUICIDE_PIN_STATE
    #define SUICIDE_PIN_STATE                 HIGH
  #endif
#endif

and #define SUICIDE_PIN -1 in my Configuration.h (I'll make a PR for this, looks like a good fix to me)

Nevertheless, I'm not convinced that one specific printer using a modified version of a more standard board should drive the default definition of the standard one.

At least, I think that PS_ON_PIN and SUICIDE_PIN should be modified. Pin 40 (PS_ON_PIN) isn't exposed on the standard creality mainboard, so this is irrelevant. Pin 12 (SUICIDE_PIN) is a part of a group of 4 pins planned as extensions pins.

If a default PS_ON_PIN is really required, then its sounds more logic to me to use pin 12, as comments suggested.

PS: After some research, the board used in the CR2020 seems to be the Creality mainboard v3.1, which seems to be discontinued.
Maybe, this board should have its own pins.h file.

Bug Timeline

No response

Expected behavior

No response

Actual behavior

No response

Steps to Reproduce

No response

Version of Marlin Firmware

2.1.2.3

Printer model

Creality CR-10 S5

Electronics

Power Supply Control with a static relay

LCD/Controller

No response

Other add-ons

No response

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

Configuration.zip

@TheRaf974
Copy link
Contributor Author

I also have an interrogation about the suicide feature, what is it exactly ? I can't find any documentation or discussion about it. It seems to be a legacy of Marlin 1. Should it be mapped to a pin connected to the reset pin of a the microcontroller to force reset in case of a critical failure ?

@TheRaf974 TheRaf974 changed the title [BUG] Unconditionnal definition of SUICIDE_PIN for RAMPS_CREALITY, leading to issue with PSU_CONTROL [BUG] Unconditionnal definition of SUICIDE_PIN for BOARD_RAMPS_CREALITY, leading to issue with PSU_CONTROL Jun 1, 2024
@thinkyhead
Copy link
Member

thinkyhead commented Jun 8, 2024

Some boards (ZM3) and add-ons (MKS_PWC) provide a "suicide pin" feature, similar to a "dead-man's switch." The pin must remain in a particular state, or else the machine does a rapid shutdown. The "CR2020 Industrial series" must have this also. These pins definitions came directly from Creality, and often vendor code changes are not tailored to consider use in our more generic codebase. I am adding a wrapper so that configs will need to specify that they are actually a "CR2020" and only then will these power / suicide pins be applied.

Copy link

github-actions bot commented Aug 8, 2024

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 Aug 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants