search

MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.29k stars 19.24k forks source link

[BUG] Missing steps after Input Shapping merge #25369

Closed dc740 closed 1 year ago

dc740 commented 1 year ago

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

Yes, and the problem still exists.

Bug Description

Input Shaping DOES NOT get disabled even when compiling without it. It's always modifying the max feedrate calculations. This results in A LOT of missing steps when reaching the max feed rate on Tevo Tornado with DRV8825 drivers on fast decay mode.

Tried:

WORKAROUNDS:

  1. revert to 2.1.1
  2. reduce max feed rates to 1/3 of original values from 2.1.1

Input shaping also caused this: https://github.com/MarlinFirmware/Marlin/issues/25350

This MR does NOT fix the issue: https://github.com/MarlinFirmware/Marlin/pull/25335

Bug Timeline

After Input Shaping got merged

Expected behavior

Actual behavior

Steps to Reproduce

  1. Compile without input shaping OR enable input shaping and disable it with M593 F0
  2. Move at max feed rate (for example run: G28 + G0 X10000 F60000)

Version of Marlin Firmware

bugfix-2.1.x

Printer model

Tevo Tornado

Electronics

MKS Gen v1.4

Add-ons

DRV8825 with fast decay mod

Bed Leveling

None

Your Slicer

Prusa Slicer

Host Software

SD Card (headless)

Don't forget to include

Additional information & file uploads

Configuration.zip

thisiskeithb commented 1 year ago

Duplicate of https://github.com/MarlinFirmware/Marlin/issues/25117

tombrazier commented 1 year ago

G0 X10000 F60000 is confusing me: move X at 1000mm/s? At a steps/mm of 160 (from the linked configuration), this means a step rate of 160,000. i.e. 100 CPU cycles per step. A 16MHz AVR cannot get anywhere close to this.

dc740 commented 1 year ago

it was just a sample command I used to make the firmware (or eeprom) limits kick in. It also happens during print.

for example, my start g-code has these that miss steps 100% of the time

G1 X3 Y1 Z15 F9000      ; Move safe Z height to shear strings
G0 X1 Y1 Z0.2 F9000     ; Move in 1mm from edge and up z 0.2mm

the command in the original report is just a sample command outside of specs that force the firmware to use the limits. The problem is that I don't get the 2.1.1 speeds, even disabling input shaping.

these commands work just fine on 2.1.1

tombrazier commented 1 year ago

Ah. I am less confused now.

I think @thisiskeithb is probably correct in marking this as a duplicate of #25117. I think it is related to multi-stepping which exists to enable high speeds for slower hardware.

I have been learning that multi-stepping doesn't generally work above about 4 steps per ISR (although this will vary according to hardware and micro-stepping setup). Input shaping included some corrections to the multi-stepping calculations which now result in higher numbers of steps per ISR. The new calculations are actually more correct in terms of what the AVR can handle during acceleration and deceleration ramps. (Well, correct if #25335 is applied!) But because multi-stepping is intrinsically not how steps should be generated, the higher "correct" number if multi-steps breaks things for some hardware.

I have started working on a much smoother approach to multi-stepping and also implementing some stepper ISR optimisations for AVR.

In the meantime if I am right then reverting to the following #defines in stepper.h will restore 2.1.1 behaviour:

#define ISR_BASE_CYCLES  752UL

#define ISR_LOOP_CYCLES (ISR_LOOP_BASE_CYCLES + _MAX(MIN_STEPPER_PULSE_CYCLES, MIN_ISR_LOOP_CYCLES))

#define ISR_EXECUTION_CYCLES(R) (((ISR_BASE_CYCLES + ISR_S_CURVE_CYCLES + (ISR_LOOP_CYCLES) * (R) + ISR_LA_BASE_CYCLES + ISR_LA_LOOP_CYCLES)) / (R))
dc740 commented 1 year ago

I just updated it twice. Here are the results:

Input shaping DISABLED on compilation time + S Curve enabled + changes from comment above: Lots of missed steps

Input Shaping ENABLED + S Curve disabled + changes from comment above: Works OK.

Question is: What if I want S Curve? Right now as you can see, there is no way to enable it, even when disabling IS. This forces me to go through the IS calibration process.

dc740 commented 1 year ago

I just printed a test calibration cube. Is it possible that Input Shaping is better than SCurve even without calibration?

I didn't go through the calibration process yet. I'm using the default values, and the cube came out with WAY LESS ghosting. It looks much better, but I thought the algorithm was only good after calibrating the frequencies as explained in the documentation.

tombrazier commented 1 year ago

Is it possible that Input Shaping is better than SCurve even without calibration?

Yes, absolutely. SCurve does not address the worst cause of ghosting, which is the sudden direction change on corners. IS does address corners (and also the accel/decel ramps which SCurve addresses). IS still works if the configured frequency is incorrect, but it less effective. Most printers seem to have dominant resonant frequencies not too far from the default of 40Hz. So I would expect IS to show improvements even before calibration.

tombrazier commented 1 year ago

Input shaping DISABLED on compilation time + S Curve enabled + changes from comment above: Lots of missed steps

Did you have SCurve enabled on 2.1.1? If IS is disabled at compilation and the multi-stepping changes are reversed then 2.1.2 is essentially the same code as 2.1.1. (There are some other non-IS related changes but they're primarily just cleaning up the code and don't change functionality.)

So I would expect this case to behave the same between 2.1.1 and 2.1.2.

dc740 commented 1 year ago

Did you have SCurve enabled on 2.1.1?

Yes, this is what I've always used. But seeing the results of IS, I have to admit I will be using IS in the future. NICE JOB! Really.

I only tested disabling SCurve because it was mentioned here: https://github.com/MarlinFirmware/Marlin/issues/25117

I think there should be an error or a warning if both features are enabled, since (from your comment) it doesn't make sense to have both.

Regarding this ticket: I'm still worried about not being able to enable SCurve after the IS changes, even though I won´t be using them anymore, the step skipping was terrible. I guess for my slow avr board (MKS Gen v1.4) I'll have to apply the patch you suggested if I want to keep using newer (post IS) versions, or wait for more fixes in the future related to slow boards. Correct?

tombrazier commented 1 year ago

NICE JOB! Really.

Thank you.

I think there should be an error or a warning if both features are enabled, since (from your comment) it doesn't make sense to have both.

A warning would make sense. I have the impression there are still users who would prefer to enable both. And if their hardware can handle it there is no convincing argument for them not to.

I'm still worried about not being able to enable SCurve after the IS changes

Yeah, me too. Slightly out of ideas and available time right now, though. Do you happen to have the skills to track down the actual commit that made the difference?

I guess for my slow avr board I'll have to apply the patch you suggested if I want to keep using newer (post IS) versions, or wait for more fixes in the future related to slow boards. Correct?

I have an MKS GEN L so we're in the same boat and I am pretty keen on keeping Marlin available on the AVR boards.

Yes, you'll need the patch. But if you're interested in testing smooth multi-stepping when it is ready I would appreciate that. I am already breaking through the previous speed limits on my printer with it.

dc740 commented 1 year ago

Sure! I'll do a git bisect. For the record, I need to find the commit that makes the machine start skipping with these two: IS disabled, SCurve enabled.

Correct?

About testing future features. No problem! I was going to test them anyway on new releases. I've been using Marlin for my home printer for years, offering support and helping when needed is the least I can do.

Last question: Would it make sense to add a condition to use the values you sent when the platform is AVR? I mean, it wont work otherwise if the feed rates are too high and the micro stepping is too low (1/32 in my case). The code could warn the user, or apply those values to prevent issues like mine

tombrazier commented 1 year ago

For the record, I need to find the commit that makes the machine start skipping with these two: IS disabled, SCurve enabled.

Yes. [Edit: Oh hang on, not quite. Also apply the above changes to ISR_BASE_CYCLES, ISR_LOOP_CYCLES and ISR_EXECUTION_CYCLES for any commit where these are changed from 2.1.1.]

Would it make sense to add a condition to use the values you sent when the platform is AVR?

I think I'd like to just address the real underlying problem with multi-stepping if possible. Mind you, that depends on the results of your git bisect. Maybe the underlying problem is something else.

dc740 commented 1 year ago

With and without the changes to stepper.h, the commit that always breaks the stepping on my machine is this one: https://github.com/MarlinFirmware/Marlin/commit/89334caa526f2d300eee834d34d06d8f837a57d5 It starts failing from the moment you apply that one.

First I tried without the changes, it failed exactly on that commit. Then I applied the changes you suggested and tested it again. It failed again on that commit. This previous commit works OK: https://github.com/MarlinFirmware/Marlin/commit/d4d1112ae8bb65d8b7c9f74d1a8586b893e57b1d

tombrazier commented 1 year ago

I have created branch https://github.com/tombrazier/Marlin/tree/dc740 which is 89334caa526f2d300eee834d34d06d8f837a57d5 but reverting all the changes to stepper and planner which are not guarded by a compile time test for input shaping. That includes the changes above and also some others which I don't think could make any difference. But who knows, maybe they do and I am just not seeing it.

Can you test this branch with input shaping disabled in config please. i.e. INPUT_SHAPING_[XY] not defined in Configuration_adv.h.

dc740 commented 1 year ago

I disabled input shaping and enabled S-Curve. The problem with missing steps persists. This is the configuration I used (just to double check) Configuration.zip

dc740 commented 1 year ago

Same config as last comment, Same branch. I reverted the stepper changes to before the "IS improvements" commit (and only the stepper changes, not the planner)

git checkout d4d1112ae8bb65d8b7c9f74d1a8586b893e57b1d Marlin/src/module/stepper.cpp
git checkout d4d1112ae8bb65d8b7c9f74d1a8586b893e57b1d Marlin/src/module/stepper.h

Now the branch works.

dc740 commented 1 year ago

Same test as above, same config (IS disabled, S-Curve enabled) but this time on the Marlin official bugfix-2.1.x branch, updated to 5 minutes ago, but reverting the stepper files to the commit before the "IS improvements":

git checkout d4d1112ae8bb65d8b7c9f74d1a8586b893e57b1d Marlin/src/module/stepper.cpp
git checkout d4d1112ae8bb65d8b7c9f74d1a8586b893e57b1d Marlin/src/module/stepper.h

Now the code works. Looks like it's something in the stepper changes indeed. But the repository you sent me didn't work.

dc740 commented 1 year ago

Last test. Same config (IS disabled, S-Curve enabled) on the official repository bugfix-2.1.x branch. Reverting ONLY stepper.h file:

git checkout d4d1112ae8bb65d8b7c9f74d1a8586b893e57b1d Marlin/src/module/stepper.h

Result: it works OK

tombrazier commented 1 year ago

I disabled input shaping and enabled S-Curve. The problem with missing steps persists. This is the configuration I used (just to double check) Configuration.zip

That config results in a compile error for me on the branch I created.

In file included from buildroot/share/PlatformIO/scripts/../../../../Marlin/src/inc/MarlinConfigPre.h:39:0,
                 from buildroot/share/PlatformIO/scripts/../../../../Marlin/src/inc/MarlinConfig.h:28,
                 from buildroot/share/PlatformIO/scripts/common-dependencies.h:29:
buildroot/share/PlatformIO/scripts/../../../../Marlin/src/inc/../../Configuration.h:3050:15: error: missing binary operator before token "("
 #if DGUS_UI_IS(MKS)
               ^
Error: Failed to parse Marlin features. See previous error messages.

It's a 2.1.3 config file by the looks of it.

tombrazier commented 1 year ago

@dc740 I have compiled my branch with the example Tevo Tornado V2 config (which has S Curve and not IS) and then reverted stepper.cpp and stepper.h with the git checkout commands you give and compiled again. The binary files generated are identical.

I have also modified the default config to match (as best I can) the config you posted above. Again, before and after reverting the stepper files, the same binary is generated.

tombrazier commented 1 year ago

Oh I think I might just have spotted the problem: I did not specify which definition of ISR_BASE_CYCLES to change. It is found on around line 79 and also around line 113. The one that applies to AVRs is the second one. Did you change the first one instead perhaps?

dc740 commented 1 year ago

Comment entirely edited to reflect the latest results:

first things first, the files are different changing the .h file:

fb9c7e85e633c72a3aeeff90f577fa73  latest_stepper.hex
c1716f6a59030cd446a8d8ad8e308c3d  reverted_stepper.hex

these are the two binaries (+configuration) that came from the bugfix-2.1.x branch at commit 4c136c3dad1284d7c1ece2238c8413f8f97f8392

binaries_for_diff_stepper_files.zip

I compile the project like this: platformio run -e mega2560

regarding ISR_BASE_CYCLES: I didn't know there were 2 definitions!!! when i DID have to edit it manually (first comments on this thread), I edited the wrong one! so we can discard all tests that involved touching that line manually. I guess I have to retest those.

Regarding configuration I found I was using the wrong branch on the repository you sent me. My bad, I apologize.

dc740 commented 1 year ago

EDIT: Changing the right line in stepper.h makes it work! Finally some reasonable results. Iḿ terribly sorry about the other results. I didn't see there were more definitions.

I'm retesting your repository now. I just discovered I was in the wrong branch (facepalm).

EDITED AGAIN: I can confirm your repository+branch works OK. The patch you suggested on your first comments is enough to make SCurve work (in your branch and in the main repository too). I apologize again for the confusion. I was editing the wrong line, and later used the wrong branch.

tombrazier commented 1 year ago

I was editing the wrong line, and later used the wrong branch.

I'll let you claim using the wrong branch, but I think not telling you there are two definitions of ISR_BASE_CYCLES is on me.

Anyway, this means this issue is indeed a duplicate of #25117. I'll crack on with smooth multi-stepping when I get the chance.

dc740 commented 1 year ago

Thank you! I'll keep an eye to test it.

github-actions[bot] commented 1 year ago

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.