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

https://github.com/MarlinFirmware/Marlin/pull/24797#issuecomment-1288136748

[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.

[tombrazier]

@RV-from-be I now have a Creality 4.2.7 with TMC2225 drivers. Annoyingly I can't get them to freeze up even without this PR.

Could you build with TMC_DEBUG, trigger the error and post the output from M122 please. [Edit: that's assuming your stepper drivers are not all STANDALONE.]

Also do you have a config and gcode file that quickly/easily reproduce the stepper freeze? (e.g. perhaps with MAXIMUM_STEPPER_RATE at the default or without this PR applied).

[RV-from-be]

@tombrazier I did all my tests with your latest PR commits with a Creality v4.2.7 motherboard (TMC 2225). These motherboards can only be used in STANDALONE mode only (TMC2208_STANDALONE declared in Configuration.h). Creality restrained the drivers. UART mode is unavailable. The tests were carried out (after calibration of the IS) with the same Gcodes file printed 5 times:

test1: IS on, LA off, Acceleration fixed (depending on calibration) at 5000mm/s^2, freq = 35.28, Zeta = 0.2, Maximum Stepper Rate at 100000, print speed at 100mm/s (depending on slicer , varying between 80 mm/s and 100mm/s) -> No issues.

test2: IS on, LA on (after K value recalibration), Acceleration set at 5000mm/s^2, freq=35.28, Zeta=0.2, Maximum Stepper Rate at 100000, print speed at 100mm/s ( depending on the slicer, varying between 80 mm/s and 100mm/s) -> an axis stops randomly during the prints systematically.

[tombrazier]

@RV-from-be Yes, I have been discovering the joy of 4.2.7 myself. I guess standalone makes sense in the context of mainboards which can have any of a range of stepper drivers, not all of which have TMCs. But it would have been really handy to be able to interrogate the driver.

Could you post your config and test gcode? Hopefully I can get my 4.2.7 (marked with an H so hopefully with TMC2225s) to replicate the bug.

[Nikolay-Po]

@tombrazier, to reproduce a problem of standalone driver, you need to replicate same driver configuration as set on board. So, to be sure, @RV-from-be need to check PDN_UART, SPREAD, MS1 and MS2 inputs voltages. The state of these pins is depend on driver PCB circuit, control board circuit and jumper settings. Also there is small chance that TMC2225 may be pre-programmed for some different OTP-memory settings.

[RV-from-be]

@Nikolay-Po Sorry but it's not like that at all on a Creality v4.2.2 or v4.2.7 motherboard. I know them very well. There is no UART mode, no jumper whatsoever. The vRefs (driver voltage) can be adjusted, for example, via a small screw.

@tombrazier
As soon as you have your motherboard, let me know and I will send you the pre-compiled firmware with the parameters mentioned in my previous comments. TMC2208_STANDALONE declared in Configuration.h - Calibration cube as file Gcode. test1: IS on, LA off, Acceleration fixed (depending on calibration) at 5000mm/s^2, freq X= 35.28, freq Y=32.8 , Zeta = 0.2 (both) Maximum Stepper Rate at 100000, print speed at 100mm/s (depending on slicer , varying between 80 mm/s and 100mm/s) -> No issues.

test2: IS on, LA on (after K value recalibration), Acceleration set at 5000mm/s^2, freq X= 35.28, freq Y=32.8, Zeta=0.2 (both), Maximum Stepper Rate at 100000, print speed at 100mm/s ( depending on the slicer, varying between 80 mm/s and 100mm/s) -> an axis stops randomly during the prints systematically.

[Nikolay-Po]

@RV-from-be, thank you for clarification. Of course, if you know that driver IC connection is exactly the same on both boards, there should be none difference in driver behavior. Ideally, it should be possible to create the same driver intrinsic setup on UART-aware board using the same mode that configured standalone. But different Marlin software configuration (at least the difference in driver UART interface presence) may change a lot due to different CPU cycles consumption, etc.

[tombrazier]

As soon as you have your motherboard

I have it already. I just haven't been able to get it to freeze yet.

[tombrazier]

Ideally, it should be possible to create the same driver intrinsic setup on UART-aware board using the same mode that configured standalone. But different Marlin software configuration (at least the difference in driver UART interface presence) may change a lot due to different CPU cycles consumption, etc.

I have a 4.2.7 so I am hoping to get able to replicate this. I really hope the OTP data is the same - I hadn't even thought about that!

[Nikolay-Po]

I really hope the OTP data is the same - I hadn't even thought about that!

OTP programming is special production option, which requires a programmer, an adapter for bare IC and a person or a robot to handle the ICs. Also it consumes some time. It cost. I think if Creality will program their driver by OTP, they will write in advertise: "Specially programmed Trinamic drivers!". If not, you may assume the OTP is unprogrammed. Sorry for bothering you with OTP option. Are there testpoints on control board for UART_PDN pin of each driver? If they are, the manufacturer can program already soldered drivers right on board. It is always cheaper to program devices on board, in place. But if there are none testpoints - you may assume that drivers are unprogrammed.

[tombrazier]

There are no testpoints. That's a good observation. I was thinking it would have been really handy if there were testpoints, though. The boards seem to all be very much designed around swapping out drivers and even with the exact same numbering, e.g. 4.2.2 or 4.2.7 could have drivers as different as a TMC or an old A4988.

[tombrazier]

I have heard back from TMC:

I think this means the current setpoint may affect whether the bug is triggered or not. @RV-from-be can you measure the voltage on your trim potentiometers? (Does that make sense to you? From GND to the metal bit where you put the screwdriver to adjust current.)

[RV-from-be]

@tombrazier I see what you mean. My vRefs for X and Y axes are fixed to 0.9v (motherboard v4.2.7 TMC2225 Standalone mode).

[tombrazier]

@thinkyhead @thisiskeithb An update on where we are with this PR: I have been chasing down two issues that are not completely resolved by the PR, #24927 and the TCM2208 problem.

24927: On the STM32G0 processors (and STM32F - but I do not know about other 32 bit processors) adaptive step smoothing massively underestimates the time needed by PULSE_PREP and also Stepper::shaping_isr() uses a lot of processor time. My guess is that this is to do with SRAM bandwidth/latency. Also, it turns out that using a the float ShapeParams::frequency in lieu of a bool is really slow. So @cbagwell and I are working on fixes for these.

TMC2208: According to Maxim support this is actually related to an over-current condition (see https://github.com/MarlinFirmware/Marlin/pull/24951#issuecomment-1307494930) caused by a mis-estimate of back EMF when the step rate changes very suddenly. I suspect this therefore also depends on PSU voltage (I have 12V and have not yet been able to replicate the problem whereas those who have done have 24V), stepper resistance, stepper indictance and, possibly, current set point. Armed with this info I am now hoping I might be able to replicate the problem.

[InsanityAutomation]

@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);

From a vendor or OEM perspective, it may be most desirable to ship with IS compiled in but set to 0 from the factory. Especially on self built machines where belt tension will be basically anywhere. Perhaps the best option is to utilize some sane default in the case of a 0 default frequency for the above calculations, if there is no way to get the constexpr off them to run based on current configured value at least.

[tombrazier]

From a vendor or OEM perspective, it may be most desirable to ship with IS compiled in but set to 0 from the factory.

The buffer size is calculated from the frequency. Zero would mean an infinite buffer size. For a machine with plenty of SRAM, a value lower than any frequency ever expected, say 15Hz, would be best. And along with this using LIMITED_MAX_FR_EDITING and MAX_FEEDRATE_EDIT_VALUES equal to DEFAULT_MAX_FEEDRATE. That would ensure a user could configure any speed and frequency without running into speed limiting.

[Riconec]

sorry for writing quite a different issue here, but I try to compile marlin with IS enabled, after updating config files it compiled, however both latest bugfix, initial PR of IS and this PR all result in broken main screen that leads to reset loop...

it looks like this: part of boot screen isn't redrawn, main screen corrupted.

I am running skr mini e3 v3 (2209), tried to force EEPROM_INIT_NOW thinking it is just EEPROM layout issue, however wasn't the case. what I can do here? can I enable some kind of debug output ?

[tombrazier]

@Riconec This sounds like #24927. Are you using ADAPTIVE_STEP_SMOOTHING? If so, I have some changes coming that should sort it out. Still testing them on my end.

[Riconec]

@tombrazier No, ADAPTIVE_STEP_SMOOTHING isn't enabled. I thought first it might be ram lacking but 144k RAM chip should be enough :D I tried disabling some features like UBL, splashscreen etc however no changes. I guess I need to check stack\heap sizes because if I have enough free ram but it behaves like not enough ram might be it. Interesting why exactly IS triggers it. I will comment what I find later. Maybe it makes sense to check exact chip model regarding STM32G0B0\B1 memory layout

[Riconec]

@tombrazier so I tried increasing stack&heap 2 fold, still the same. Tried disabling IS and it also crashes... So the issue might be somewhere in bugfix branch

[Riconec]

@tombrazier noob question: does EEPROM_VERSION need to be changed for IS?

[tombrazier]

noob question: does EEPROM_VERSION need to be changed for IS?

No.

[tombrazier]

Tried disabling IS and it also crashes...

@Riconec DO you mean disabling with M593 or in Configuration_adv.h? There is some code that gets executed even if disabled with M593 and significantly increases the workload of the stepper ISR with the SKR Mini V3. This is partly what I am fixing.

[tombrazier]

The set of commits above are optimisations and corrections to the ASS logic that should (hopefully!) sort out #24927.

@cbagwell @thespooniest @Riconec I would appreciate testing.

On to TMC2208...

[tombrazier]

@RV-from-be So for the TMC2208 problem, please confirm:

• It only occurs with both input shaping and linear advance.
• The X or Y axis stops responding (i.e. the E axis is fine).
• When an axis stops, the motor is switch off (i.e. you can easily move the bed / print head by hand).

Also, in earlier test, you found the problem occurred when IS is compiled is but disabled with M593 F0. Is this still the case?

Right now the only thing I can think that might affect timing between IS and LA is the performance issues which the latest changes address. Can you test with these changes applied?

Finally, can I have your calibration cube gcode and config please.

[MLiv79]

How could be set correct input shaping if I can't disable it for the first print? If i set in marlin options all frequency and Dumoing to 1 and 0 (minimum values) even homing doesn't work, axis move very very slow. The first tower with variable acceleration speed need to be printed with input shaping disabled right?

[tombrazier]

@MLiv79 Set frequency to zero at runtime or disable IS from config. If you can't set frequency below 1, I suspect you are using older code. Have you applied this PR?

[MLiv79]

@MLiv79 Set frequency to zero at runtime or disable IS from config. If you can't set frequency below 1, I suspect you are using older code. Have you applied this PR?

I've frequency minimum to 1 and dumping to 0. Release is the yesterday nightly. I can't set frequency to zero

[tombrazier]

@MLiv79 Use M593 to set frequency to 0 or disable input shaping in config. Or apply this PR and then the LCD will allow you to disable input shaping.

[MLiv79]

@MLiv79 Use M593 to set frequency to 0 or disable input shaping in config. Or apply this PR and then the LCD will allow you to disable input shaping.

With m593 F0 it returns "frequency must be greater than zero". I'm not able to apply PR. I have to rebuild firmware with m593 disabled...

[tombrazier]

@MLiv79 Ah, yes, I see that now. M593 F0 is also in this PR. Yes, you'll need to change config and recompile.

[Riconec]

@tombrazier I've been told I had incorrect config files for 2.1.x bugfix, working to fix that. When I have printer not crashing on boot will try it. However where are the latest changes? Or I need #24955 also?

[tombrazier]

@Riconec #24955 may improve SCURVE acceleration and deceleration ramps when using ADAPTIVE_STEP_SMOOTHING. But since the ramps take very little time it won't affect the general responsiveness of the printer unless you're printing a lot of short segments (or arcs/beziers which are effectively the same thing).

[cbagwell]

My initial test print went well. I tested this branch in isolation but also with IS+ASS+SCURVE all enabled on STM32G0. Homing work and a calibration cube printed nice and with no watchdog reboots. That's a big behavior improvement even with SCURVE cycle estimates still being under counted by 500 cycles on the STM32G0.

[tombrazier]

@cbagwell Excellent. The SCURVE cycle count will only affect acceleration and deceleration ramps, which do not last long enough to trigger the WDT unless you are printing a long line of short segments which consist entirely of accel/decel ramps. And now that the rest of the cycle count calculations are more accurate, the extra 500 cycles will probably still not push the processor to 100%.

[Riconec]

@tombrazier should I use ADAPTIVE_STEP_SMOOTHING for better IS results?

[tombrazier]

should I use ADAPTIVE_STEP_SMOOTHING for better IS results?

It works independently of IS. I mentioned it because #24955 was caused by IS with ASS both enabled.

[Riconec]

I have 2 printers, ender3 and ender clone ender 3 (up) running 2.1.1 and clone (down) running IS firmware Before I try enable IS on clone if I have no real ringing but see some kind of degradation with increasing acceleration (2500 looks better than 500 somehow) how do I find frequency to compensate :D I see with strong light lines that seems to be belt pitch, not sure they can be dealt in any way

As a side note clone got random glitches during the print, when it finished screen was already complete corrupted trash... so bugfix somehow still gives screen issues

[RV-from-be]

• It only occurs with both input shaping and linear advance.
• The X or Y axis stops responding (i.e. the E axis is fine).
• When an axis stops, the motor is switch off (i.e. you can easily move the bed / print head by hand).

Also, in earlier test, you found the problem occurred when IS is compiled is but disabled with M593 F0. Is this still the case?

Right now the only thing I can think that might affect timing between IS and LA is the performance issues which the latest changes address. Can you test with these changes applied?

@tombrazier Before your latest commits today, when an axis stop (9 times of 10, X axis), the motor is swif off (I can move manually the hotend). And no compilation error.

Ok, I modify my sources with your latest commits and then I do new tests with a Creality v4.2.2 TMC2208 STANDALONE mode and a Creality v4.2.7 TMC 2225 STANDALONE mode: IS alone, IS + LA, IS + LA + SCURVE Experimental with MAXIMUM STEPPER RATE at 100000. Once the tests are finished, I'll come back to you.