Input Shaping improvements

[tombrazier]

Description

What the PR does address:

• Disable input shaping by setting frequency to 0. Works per axis.
• ADAPTIVE_STEP_SMOOTHING now takes the extra processing required for IS into account (erring on the side of caution).
• There was a synchronisation error that caused the echo dividends to change one step too soon.
• Removed the restriction on using laser with input shaping.
• TMC2208 fix for direction changes.
• TMC2208 fixes for very occasional double steps caused by tracking the rounding error.
• Fixed a step counting problem caused by sync blocks. (Which are generated by G92 E0 in the middle of a print for example.)
• Handles multi-stepping correctly.

Finally there is also a simplification in the echo logic that is possible which will hopefully reduce SRAM usage, code and CPU usage. But this needs completing and testing so is perhaps for another PR.

Requirements

No.

Benefits

Input shaping is now better.

Related Issues

#24797
#24927
#24928
#24797 (comment)

[RV-from-be]

@tombrazier
I pushed this PR and did some testing. I'm sorry but the TMC2208 and 2225 bug is still there. On a calibration cube, after 10 min 1 time out of 2, the X axis stops. On other prints, after 35 min, see less than 1 h, the X axis stops. Worse still, by setting the X and Y frequencies to 0, the X axis also stops. I had done tests with your first patch and setting the MAXIMUM_STEPPER_RATE to 100000 and all my impressions went through. Note that in this case, I performed my tests with the same Gcodes.
Tests with Ender 3 Pro (v4.2.7 and TMC 2225 Standalone) and Ender 3 v2 (v4.2.2 and TMC 2208 Standalone) without Linear Advance.

[RV-from-be]

@tombrazier
I also think there is a problem here when the frequencies are at 0 in stepper.cpp:

  TERN_(HAS_SHAPING_X, count_position.x += x_shaping_delta);
  TERN_(HAS_SHAPING_X, shaping_x.last_block_end_pos = spos.x);
  TERN_(HAS_SHAPING_Y, count_position.y += y_shaping_delta);
  TERN_(HAS_SHAPING_Y, shaping_y.last_block_end_pos = spos.y

and:

      #if HAS_SHAPING_X
        int32_t echo_x;
      #endif
      #if HAS_SHAPING_Y
        int32_t echo_y;
     #endif

TERN_(INPUT_SHAPING, shaping_dividend_queue.enqueue(TERN0(HAS_SHAPING_X, echo_x), TERN0(HAS_SHAPING_Y, echo_y)));

fixed by:

  #if ENABLED(INPUT_SHAPING)
    if (TERN0(HAS_SHAPING_X, shaping_x.frequency)) {
      count_position.x += x_shaping_delta;
      shaping_x.last_block_end_pos = spos.x;
    }
    if (TERN0(HAS_SHAPING_Y, shaping_y.frequency)) {
      count_position.y += y_shaping_delta;
      shaping_y.last_block_end_pos = spos.y;
    }
  #endif

and:

      #if HAS_SHAPING_X
        int32_t echo_x = 0;
      #endif
      #if HAS_SHAPING_Y
        int32_t echo_y = 0;
     #endif

if ((TERN0(HAS_SHAPING_X, shaping_x.frequency)) || (TERN0(HAS_SHAPING_Y, shaping_y.frequency)))
        shaping_dividend_queue.enqueue(TERN0(HAS_SHAPING_X, echo_x), TERN0(HAS_SHAPING_Y, echo_y));

And in stepper.h, if HAS_SHAPPING_X or Y not defined:

const bool result = !shaping_queue.empty_x() || !shaping_queue.empty_y();

fixed by :

const bool result = TERN0(HAS_SHAPING_X, !shaping_queue.empty_x()) || TERN0(HAS_SHAPING_Y, !shaping_queue.empty_y());

[tombrazier]

@RV-from-be I have made the suggested changes, plus some others. Haven't had a chance to test them. I would appreciate a code review and also if you have the time to test that would be very helpful.

[tombrazier]

Oh hang on, there are some more compile errors with shaping just one axis.

[tombrazier]

I have also committed the changes I think need making for multi-stepping. I haven't been able to test extensively but if others can confirm the changes are good, I think this PR is ready to merge for 2.1.2.

[RV-from-be]

@tombrazier Nice, I will prepare some tests. I defined MAXIMUM_STEPPER_RATE with 100000.

[Nikolay-Po]

There are some problems about disabling input shaping.
First. There is UI problem: it is impossible to set Input shaping frequency in advanced configuration menu to zero. Lowest possible value is 1.0Hz, so I can't disable IS from printer panel.
Second. If set SHAPING_FREQ to zero in configuration_adv.h:

In file included from Marlin/src/MarlinCore.cpp:47:
Marlin/src/module/stepper.h:337:55: warning: division by zero [-Wdiv-by-zero]
  337 |   constexpr uint16_t shaping_dividends = max_steprate / _MIN(0x7FFFFFFFL OPTARG(HAS_SHAPING_X, SHAPING_FREQ_X) OPTARG(HAS_SHAPING_Y, SHAPING_FREQ_Y)) / 2 + 3;

The only option to disable IS remaining is to put a command M593 F0.

[RV-from-be]

@tombrazier
I did a series of tests (Input Shaping activated, Linear Advance deactivated, MAXIMUM_STEPPER_RATE at 100000, TMC2208/2225 Standalone) and for the moment it works correctly. I will start them again by activating the Linear Advance in order to be certain that these issues with the TMC2208 are a thing of the past.

[RV-from-be]

@Nikolay-Po
I don't think it's a very good idea to set SHAPING_FREQ to 0 in configuration_adv.h...That's not the spirit. It is better to set the frequencies to 0 in the user interface.
For that you have to modify in menu_advanced.cpp:

void menu_advanced_input_shaping() {
      constexpr float min_frequency = TERN(__AVR__, float(STEPPER_TIMER_RATE) / 2 / 0x10000, 1.0f);

by:

void menu_advanced_input_shaping() {
      constexpr float min_frequency = TERN(__AVR__, float(STEPPER_TIMER_RATE) / 2 / 0x10000, 0.0f);

[tombrazier]

@Nikolay-Po Thanks for noticing both of those things.

The UI needs changing. I think we actually need an on/off option for each axis because there really is a min frequency below which integer overflows occur. M593 allows zero or a value >= the minimum.

In Configuration_adv.h you can just comment out X or Y config to disable. This is non-obvious now that you point it out. I'll change the comment.

[RV-from-be]

@tombrazier
I go back to my last tests this time with Linear Advance activated and Input shaping activated. Well it's a failure. After 30 to 40 min, an axis stops! (still with the latest additions to the PR, TMC2208 and 2225 Standalone, MAXIMUM STEPPER RATE at 100000). A higher (including default) or lower value leads to a failure with a stop of one of the axes.

[Nikolay-Po]

Thanks for UI update. It is convenient and intuitive. Trying to tune IS. Hope mine TMC2209 will be OK with IS+LA.

[ansonl]

Suggested changes for ISR_LOOP_CYCLES

[synman]

@tombrazier apologies for practicing necromancy, but do we have a way of zeroing out the freq in Configuration_adv.h? It looks like we introduce division by zero if we try to zero out X/Y frequency there.

In file included from Marlin/src/MarlinCore.cpp:47:
Marlin/src/module/stepper.h:359:53: warning: division by zero [-Wdiv-by-zero]
  359 |                      shaping_echoes = max_step_rate / shaping_min_freq / 2 + 3;

More than happy to file this as a new issue, or even submit a PR. Just need to know where to start (administratively).

[tombrazier]

@synman Yes. Define SHAPING_MIN_FREQ and that will override the zeroed frequencies when calculating the buffer size. Then at runtime, IS will be available but disabled by default.

[synman]

@synman Yes. Define SHAPING_MIN_FREQ and that will override the zeroed frequencies when calculating the buffer size. Then at runtime, IS will be available but disabled by default.

Thank you and apologies again for hitting up a closed PR.