[BUG] Recent LPC "toggle" change breaks multi-driver axis configs

[Sanguium]

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

Yes, and the problem still exists.

Bug Description

When activating a second Z driver and EDGE_STEPPING, the first Z motor grinds and loses steps, while the second works fine.
All other axis work fine, problem goes away when disabling the Z2 driver OR disabling EDGE_STEPPING.

This is the M122 report after moving Z 10m up with and without EDGE_STEPPING enabled:
EDGE_STEPPING OFF:

EDGE_STEPPING ON:

Using SKR v1.3 with TMC2009 drivers

Bug Timeline

Tested wrong in 2.1.2.4 and current bugfix-2.1.x

Expected behavior

Dual Z drivers work fine with the recomended EDGE_STEPPING for trinamic drivers

Actual behavior

First Z motor rattles and loses steps

Steps to Reproduce

1. Set Z_DRIVER_TYPE TMC2209 and Z2_DRIVER_TYPE TMC2209
2. Set EDGE_STEPPING
3. Move Z axis

Version of Marlin Firmware

Commit 0ec1a54

Printer model

Custom cartesian

Electronics

SKR v1.3 board with TMC2209 drivers

LCD/Controller

BIGTREE_TFT24_V1_1

Other add-ons

No response

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

Config.zip

[anomalchik]

Same issue with mks sgen l v1
I use tmc2226 and they also use the driver type TMC2209

[anomalchik]

If you disconnect the first motor but do not remove the driver, the second motor remains in the brake mode or something like that. Also, the second motor works properly if you remove the Z1 driver.

[tombrazier]

#27489 is a duplicate of this issue.

I, too, have dual Z axes and TMC2209 but do not have the problem. The primary difference that I can is is that I am running on 8 bit AVR and all those reporting the problem have LPC1768.

Looking at the code, EDGE_STEPPING for the first Z stepper comes down to calling TOGGLE(Z_STEP_PIN) on line 156 of module/stepper/trinamic.h. The TOGGLE macro is MCU dependent.

[ellensp]

@tombrazier TOGGLE was recently changed on the LPC176x #27149 ...

[tombrazier]

Okay, @moriamoria, @anomalchik and @Sanguium here is something to try:

Change the file Marlin/src/HAL/LPC1768/fastio.h as follows:

Remove #define _TOGGLE(IO) LPC176x::gpio_toggle(IO)

and replace it with #define _TOGGLE(IO) _WRITE(IO, !READ(IO))

Does this resolve the problem?

I have a bunch of mainboards but no LPC1768 to test on.

[moriamoria]

Hello,

I have kill my SKR 1.3 motherboard, the X- Y- pullup end switch seems dead !

I have order a new SKR 1.4 turbo, and i will do the test when I receive it.

Thanks for your support

Regards

[Sanguium]

Okay, @moriamoria, @anomalchik and @Sanguium here is something to try:

Change the file Marlin/src/HAL/LPC1768/fastio.h as follows:

Remove #define _TOGGLE(IO) LPC176x::gpio_toggle(IO)

and replace it with #define _TOGGLE(IO) _WRITE(IO, !READ(IO))

Does this resolve the problem?

I have a bunch of mainboards but no LPC1768 to test on.

Tested with latest bugfix, this change does fix the problem.
Can I keep this until the fix is merged or does it have some other caveats?

Thanks.

[tombrazier]

@mh-dm please see above and also #27489. It looks like #27149 has caused a problem with dual Z steppers. My guess is that two calls to LPC176x::gpio_toggle() in quick succession can cause some kind of race condition. Any thoughts?

[tombrazier]

I think I have found the race condition. Function pin_type::gpio_toggle(IO) is defined in @p3p's PlatformIO framework for the LPC1768 in file master/system/lpc176x/pin_control.h.

The function is

  [[gnu::always_inline]] inline void toggle() {
    gpio_reg().reg_control ^= gpio_mask();
  }

Reading chapter 9 of the LPC1768 user manual (a copy can be found here) this function reads FIOPIN, modifies it and sets it to the new value. However a read from FIOPIN returns the current pin state, not the current port output register. To get the latter you have to read FIOSET.

As a consequence, when two pin toggles occur in quick succession the second read of the pin state can happen before the pin has actually changed after the first toggle operation (at least this is my working theory).

I think the function should be modified to:

  [[gnu::always_inline]] inline void toggle() {
    gpio_reg().reg_control = gpio_reg().reg_set ^ gpio_mask();
  }

If there is anyone who is able to find their platformio directory (on Linux it is ~/.platformio and on Windows I think it will be C:\Users\<username>\.platformio) and change the file packages/framework-arduino-lpc176x/system/lpc176x/pin_control.h as I suggest I would be interested to know the result.

Also any comment by @p3p would be appreciated.

[tombrazier]

Can I keep this until the fix is merged or does it have some other caveats?

@Sanguium Yes you can keep it until a fix is merged. The difference is slightly less clean step timing, but that's way better than steps not actually happening.

PS If you're able to test the change to LPC176x::gpio_toggle() I would appreciate it.

[Sanguium]

PS If you're able to test the change to LPC176x::gpio_toggle() I would appreciate it.

This also works.

[tombrazier]

This also works.

Excellent. So the solution is to submit a PR to @p3p's LPC1768 framework. I would appreciate pretty wide testing, I wouldn't want to break a the framework accidentally.

[tombrazier]

p3p/pio-framework-arduino-lpc176x#55

[mh-dm]

Sorry for #27149 opening up a can of issues.

@tombrazier Great investigation!

As a consequence, when two pin toggles occur in quick succession the second read of the pin state can happen before the pin has actually changed after the first toggle operation (at least this is my working theory).

That's also my read of the code and the reported issue. Z step pin gets toggled with gpio_reg().reg_control ^= gpio_mask(); (note that means gpio_reg().reg_control necessarily gets read as it's volatile). Then Z2 immediately also gets toggled with another gpio_reg().reg_control ^= gpio_mask(); which should also mean another gpio_reg().reg_control read. 1) This read happens so quickly the pin state didn't change yet. 2) toggle() in particular is problematic because it's writing to gpio_reg().reg_control which can in theory affect the state of other pins (whereas get() followed by a set(inverse) which then calls either set() or clear() can't).

I think the proposed solution:

[[gnu::always_inline]] inline void toggle() {
    gpio_reg().reg_control = gpio_reg().reg_set ^ gpio_mask();
  }

is correct to fix toggle() for one pin affecting another.

[tombrazier]

@mh-dm Do you have an LPC mainboard you could do some testing on? I'd like to see this tested fairly widely considering it affects the underlying framework.

[anomalchik]

I'll try to check it today.

[anomalchik]

  [[gnu::always_inline]] inline void toggle() {
    gpio_reg().reg_control = gpio_reg().reg_set ^ gpio_mask();
  }

@tombrazier
works fine for me. Thanks. My board is MKS_SGEN_L LPC1768.

[tombrazier]

@anomalchik Thanks.

[mh-dm]

@mh-dm Do you have an LPC mainboard you could do some testing on? I'd like to see this tested fairly widely considering it affects the underlying framework.

Sure. Tested in the same scenario as in #27149 and can confirm that the proposed toggle() version works including maintaining the slightly more consistent timings/speed benefit over the original _WRITE(IO, !READ(IO)).

[tombrazier]

Thanks Mihai.