[BUG] CURRENT_STEP_DOWN wrapping around to 65536 on multiple driver errors

[sargonphin]

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

Yes, and the problem still exists.

Bug Description

Upon encountering an error, the feature CURRENT_STEP_DOWN reduces current a certain amount in the hope to fix the error condition

If the motor current _CURRENT is not a multiple of CURRENT_STEP_DOWN, the condition that triggers a stop if current goes below 50 mA will never trigger due to the int16 current value wrapping around to 65,536 and less.

For instance, X_CURRENT == 1900 and CURRENT_STEP_DOWN == 200
Then decreasing on multiple errors would go 1700, 1500, 1300, [...], 300, 100, 65,000 (wrap around), and the condition will never be true (on top of failing the print, putting the driver in a dangerous power state and overloading the stepper motor)

Bug Timeline

New bug

Expected behavior

When the driver current reaches critical levels, stop the printer without wrapping the int16

Actual behavior

The stepper driver current wraps around to over 65,000 mA

Steps to Reproduce

For this you require a faulty condition that is not easy to reproduce.

Version of Marlin Firmware

Bugfix Feb. 2024

Printer model

--

Electronics

--

LCD/Controller

--

Other add-ons

--

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

Running Marlin Bugfix from February 2024

[thisiskeithb]

Don't forget to include

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

You forgot to do this. Please attach your configs.

[ellensp]

Issue is in

        const uint16_t I_rms = st.getMilliamps() - (CURRENT_STEP_DOWN);
        if (I_rms > 50) {

eg
st.getMilliamps() returns 100
CURRENT_STEP_DOWN = 200
100 - 200 = 65436 which passes the > 50 check

[classicrocker883]

why not use something like:
const uint16_t I_rms = NOMORE(st.getMilliamps() - CURRENT_STEP_DOWN, 2000);

[ellensp]

@classicrocker883

say you had set the current set to 850 (a common value)
with your code it wraps around and set the current to 2000, which can still damage the stepper motor coils.

This doesn't solve the issue

either of these is better

    const int16_t I_rms = st.getMilliamps() - (CURRENT_STEP_DOWN);
    if (I_rms > 50) {

eg
st.getMilliamps() returns 100
CURRENT_STEP_DOWN = 200
so 100 - 200 = -100 and fails the test and doesn't decrease any further

or this

    const uint16_t I_rms = st.getMilliamps() - (CURRENT_STEP_DOWN);
    if (I_rms > CURRENT_STEP_DOWN) {

eg
st.getMilliamps() returns 300
CURRENT_STEP_DOWN = 200
so 300 - 200 = 100, which fails the test and doesn't decrease any further

[sargonphin]

Would this fix work? I don't know how to test it, cause I would have to trigger fake TMC errors and I don't know how. And I would like to have some devs eyes tell me if that wouldn't work :)

void step_current_down(TMC &st) {
      if (st.isEnabled()) {
        if (st.getMilliamps() < (st.getMilliamps() - (CURRENT_STEP_DOWN))) {   // Adding this check to make sure this is possible
          const int16_t I_rms = st.getMilliamps() - (CURRENT_STEP_DOWN);
          if (I_rms > 50) {    // This can then be removed? 
            st.rms_current(I_rms);
            #if ENABLED(REPORT_CURRENT_CHANGE)
              st.printLabel();
              SERIAL_ECHOLNPGM(" current decreased to ", I_rms);
            #endif
          }
        }
      }
    }

I am not sure what is also supposed to happen once the value I_rms cannot go down anymore, other than not triggering the current change reporting.

[classicrocker883]

well then this should fix the issue. but is it the correct course of action? here the current will always be set to a minimum of 50.

template<typename TMC>
void step_current_down(TMC &st) {
  if (st.isEnabled()) {
    const int16_t I_rms = ((st.getMilliamps() - CURRENT_STEP_DOWN) >= 50) ? (st.getMilliamps() - CURRENT_STEP_DOWN) : 50;
    st.rms_current(I_rms);
    #if ENABLED(REPORT_CURRENT_CHANGE)
      st.printLabel();
      SERIAL_ECHOLNPGM(" current decreased to ", I_rms);
    #endif
  }
}