[BUG] Restart after MINIMUM_STEPPER_PULSE to MINIMUM_STEPPER_PULSE_NS #27113 PR

vovodroid commented 5 months ago

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

Yes, and the problem still exists.

Bug Description

Latest commit d96c6f8 fails (Marlin restart) on the first extraction move. So I went to 0169cde (one before MINIMUM_STEPPER_PULSE to MINIMUM_STEPPER_PULSE_NS #27113 merge, and it work properly.

Does this PR contain something suspicious?

Bug Timeline

No response

Expected behavior

No response

Actual behavior

No response

Steps to Reproduce

No response

Version of Marlin Firmware

bugfix-2.1.x

Printer model

No response

Electronics

SKR-3, TMC2209

LCD/Controller

TFT SPI 2

Other add-ons

No response

Bed Leveling

ABL Bilinear mesh

Your Slicer

None

Host Software

None

Don't forget to include

[X] A ZIP file containing your Configuration.h and Configuration_adv.h.

Additional information & file uploads

Config.zip

dbuezas commented 4 months ago

Same here and also a very sluggish UI while steppers move. Also bisected commits to find the problem in the same commit as this issue reports. I also have an SKR-3, with TMC2209 drivers.

I have a 20MHz oscilloscope at hand in case measuring pins is of use.

dbuezas commented 4 months ago

I should mention that the printer also often stops mid print for user confirmation, which I think means the planner is starved. Maybe a small bug where the steppers are driven at an order of magnitude faster rate than intended?

thisiskeithb commented 4 months ago

@mh-dm: Mind giving this a review?

mh-dm commented 4 months ago

First, sorry for the issues. I've tried a few things but I can't reproduce the reported issues (on my LPC1678).

Looking at #27113 there are basically 2 parts. First the actual MINIMUM_STEPPER_PULSE_NS change then there's a refactor to switch quite a few things to constexpr. It would help to figure out which part of #27113 is causing the issue.

So could one of you please:

make sure you are synced right before 27113
then download the first commit of #27113 as a patch: wget https://github.com/MarlinFirmware/Marlin/pull/27113/commits/ddb8189da75f971e4081b0dfafde85ec5068b334.patch
then apply the patch locally: git am ddb8189da75f971e4081b0dfafde85ec5068b334.patch

and report back whether it's showing the same symptoms or not.

vovodroid commented 4 months ago

I've tried first commit only. Actually it's the same, just crashes a bit later.

mh-dm commented 4 months ago

Thank you. This was quite weird but I think I figured out what's going on.

First: I've tried to build with the configuration files attached and I'm getting #error "NONLINEAR_EXTRUSION doesn't currently support multi-extruder setups.. I changed a few things to get it to build. But it was a good error to get since NONLINEAR_EXTRUSION turns out to be relevant.

So here's what I think is happening.

You're using SKR 3, which runs at a blistering 480Mhz
You're using TMC drivers, which can top out at 5 million steps/s and has a minimum pulse duration of 100ns.
You have ADAPTIVE_STEP_SMOOTHING enabled which results in oversampling. This oversampling is calculated based on an estimate of the number of cycles the stepper isr will take to run
You're using NONLINEAR_EXTRUSION which happens to blindly increase oversampling without care for what the hardware can actually do
PR 27113 fixes MIN_STEPPER_PULSE_CYCLES to correctly compute the number of cycles that a pulse of MINIMUM_STEPPER_PULSE_NS will take: constexpr uint32_t min_stepper_pulse_cycles = _min_pulse_high_ns * CYCLES_PER_MICROSECOND / 1000;

The result of all of this is with PR 27113 on SKR3 w/ TMC the MIN_STEPPER_PULSE_CYCLES goes from 960 (wrong) to 48 (correct as that number of cycles at 480Mhz takes 100ns) which means MIN_STEP_ISR_FREQUENCY goes from 75519UL to 136518UL. To give some context, assuming 80steps/mm a move at 100mm/s normally runs at 8000steps/s. Due to ADAPTIVE_STEP_SMOOTHING it will then be oversampled at 16x (136518/8000 is ~17) times 2x oversampling (due to NONLINEAR_EXTRUSION). That's a lot of oversampling.

I'm not seeing this issue on LPC176x w/ TMC because that's max 120Mhz and the absolute change in MIN_STEPPER_PULSE_CYCLES is much less so it doesn't affect MIN_STEP_ISR_FREQUENCY much.

To confirm whether my analysis is correct and to figure out a fix could you please change this section in stepper.cpp from:

        // Nonlinear Extrusion needs at least 2x oversampling to permit increase of E step rate
        // Otherwise assume no axis smoothing (via oversampling)
        oversampling_factor = TERN0(NONLINEAR_EXTRUSION, 1);

to just

        oversampling_factor = 0;

and report back whether this fixes your issues.

vovodroid commented 4 months ago

You're using SKR 3, which runs at a blistering 480Mhz

Even worse, I have 550Mhz version))

error "NONLINEAR_EXTRUSION doesn't currently support multi-extruder setups.

Yes, I just commented out this check in Sanity.h. I use second extruder for support interface, so I don't mind to use values from main extruder for both of them.

change this section in stepper.cpp

I can change config instead, if you want. Assuming you are right and change in stepper.cpp helps. Can I use it for printing or it's good just for test only?

mh-dm commented 4 months ago

If it helps it should be good enough for actual printing. I'm preparing that change as (edit) PR #27171.

vovodroid commented 4 months ago

Assuming oversampling_factor = 0; helps, should we do something like

        // Decide if axis smoothing is possible
        if (stepper.adaptive_step_smoothing_enabled) {
          uint32_t max_rate = current_block->nominal_rate;  // Get the step event rate
          while (max_rate < MIN_STEP_ISR_FREQUENCY) {       // As long as more ISRs are possible...
            max_rate <<= 1;                                 // Try to double the rate
            if (max_rate < MIN_STEP_ISR_FREQUENCY)          // Don't exceed the estimated ISR limit
              ++oversampling_factor;                        // Increase the oversampling (used for left-shift)
          }

         #if ENABLED(NONLINEAR_EXTRUSION)
            oversampling_factor = max(oversampling_factor , 1);
         #endif
        }

to ensure that we have at least 2x oversampling for NONLINEAR_EXTRUSION ? Or it anyway will be oversampled as high as possible in while loop?

dbuezas commented 4 months ago

oversampling_factor = 0; solved the issue in my machine

vovodroid commented 4 months ago

Could you try to add

         #if ENABLED(NONLINEAR_EXTRUSION)
            oversampling_factor = max(oversampling_factor , 1);
         #endif

as I suggested?

mh-dm commented 4 months ago

Or it anyway will be oversampled as high as possible in while loop?

Yes, it will be oversampled as high as possible. There's no need to try to use something like max(oversampling_factor , 1);

dbuezas commented 4 months ago

I'm printing samples to confirm non-linear extrusion still works as before (yes remembered to invert A and B)

dbuezas commented 4 months ago

yes, still works fine

vovodroid commented 4 months ago

Great! I hope that full PR with this fix works as well.

github-actions[bot] commented 2 months 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.

MarlinFirmware / Marlin