Marlin: [BUG] TMC driver initialization after ``M80`` doesn't initialize completely.

[ManuelMcLure]

Description

If you use PSU_CONTROL to control your power supply with PS_DEFAULT_OFF so the power supply is off when the board initializes, and then you use M80 to switch on the power supply, the TMC drivers are not correctly initialized.

Steps to Reproduce

1. Use TMC2209 drivers on a board with PSU_CONTROL and PS_DEFAULT_OFF
2. Power board from USB
3. Use M80 to power up the supply
4. Use M122 to see TMC driver status.

Expected behavior: [What you expect to happen]
TMC is completely initialized.
Actual behavior: [What actually happens]
TMC drivers are missing initialization for microsteps, CS actual and PMW scale.

Additional Information

The code for M80() calls restore_stepper_drivers() which attempts to reinitialize the TMC drivers. This calls TMCStepper.push() which reinitializes the drivers with stored values, but the three values listed above are not stored in TMCStepper.
Running M501 reinitializes the steppers correctly.
I believe calling reset_stepper_drivers() instead of restore_stepper_drivers() in M80() would also result in correct reinitialization, but might lose setting changes that were done since the drivers were first initialized. However, I think this may be an acceptable tradeoff.

This issue is more to discuss whether the change to make M80() call reset_stepper_drivers() would be a good option, or whether something more complex (maybe in TMCStepper instead of Marlin) would be needed. If the change in M80() is sufficient I'll make a PR.
Configuration.zip

[ManuelMcLure]

In my tests using reset_stepper_drivers() fixes the initialization of microsteps, but it looks like "CS actual" and "PWM scale" are still not correctly initialized. So this will probably need something more complicated. @teemuatlut , any input on this?

[ManuelMcLure]

Here's the result of M122 with different sequences of initialization:

Power supply on when USB power is applied to controller board:

CS actual   12/31   12/31
PWM scale   14  14
...
msteps      16  16

Power supply off when USB power is applied, M80 used to turn on PS:

CS actual   25/31   24/31
PWM scale   29  28
...
msteps      8   32

(msteps are different for the two drivers because I'm using MS1/MS2 to set the driver address so I can control them with a single pin).

Power supply off when USB power is applied, M80 used to turn on PS, reset_stepper_drivers() called from M80():

CS actual   19/31   19/31
PWM scale   22  22
...
msteps      16  16

[Haschtl]

I can confirm this bug. I'm using the Watterott TMC5160 on a BigtreeTech SKR Pro 1.1.

I also have a hardware button, which cuts the power for the motors (a safety switch). If i toggle it, the motors will not move.
There should be some GCODE to reinitialize the drivers.

Related issue: https://github.com/bigtreetech/BIGTREETECH-SKR-PRO-V1.1/issues/39

[ManuelMcLure]

Try to see if M501 initializes the drivers. It does for me.

[wap32]

Not sure if this might be the same issue as #13092 .

The issue there was for PWM mode in particular, but if the TMC status was read via the TMCStepper library while power was off, the update function would try to read from SPI (which would return all 0 since power is off) and then this function would update the internal config register with these bogus values. I believe the read part was happening at start-up when Marlin dumps the config via serial.
Then at power-on, these bogus values stored in the register would be restored to the steppers.
Calling M501 after power-on restored the correct values again.

[teemuatlut]

cs actual and pwm scale are read only registers.
mres is part of chopconf which should get pushed to the driver with M80.
It might be that when pushing the failed writes again to the driver, the first command needs to be to the GCONF registers which enables UART control bit pdn_disable.

reset and restore drivers do two different things.

[ManuelMcLure]

reset and restore drivers do two different things.

Yeah, that was my concern which is why I didn't just create a PR to change this in Marlin. Later on today I'll see if I can figure out how to build my own version of TMCStepper with GCONF first in push() and see if that fixes the problem.

[ManuelMcLure]

I made the change suggested (move the GCONF line to be the first one in TMCStepper2209::push()) but it did not seem to cause any change in behavior. I edited the code directly in .pio/libdeps/LPC1768/TMCStepper_ID5513/src/source/TMC2209Stepper.cpp and saw that file being rebuilt by PIO when I recompiled, so I'm pretty sure the change was uploaded.

[ManuelMcLure]

If I put a call to begin() at the start of TMC2209Stepper::push() it appears to fix the issue, so it looks like this may be a problem with initializing the software serial library? I made no other changes to push().

I don't know what other side effects this may have.

[ManuelMcLure]

Further digging shows that instead of begin() at the start of push(), it seems to be sufficient to just call mtep_reg_select(true). This sets the microsteps correctly. The only difference in the M122 output from the "12V enabled before USB power is applied" case are the "CS actual" and "PWM scale" values, so if that's not going to be a problem I'll do a PR for this.

[teemuatlut]

mstep_reg_select is already set be tmc_init from stepper_reset.
You need to find out why this bit wouldn't be set before, or if all you're doing is pushing the same bits again which then helps the situation. And that's the purpose of push().
Don't create a PR if you don't yet understand the underlying problem.

You can use M122 X V to read the raw register values.

[ManuelMcLure]

I can confirm that the change works. If I remove that line the problem comes back, but if I put it back in the problem is fixed. Maybe GCONF needs to be written twice in this case? I’ll do an experiment tomorrow to see if that works. I’ll also get the output from M122 X V.

[ManuelMcLure]

So I did a whole bunch of testing, adding extra calls to GCONF() in push() without success. I got the following results from M122 X V:

PSU control disabled, VMOT available when board was plugged into USB (this is the "control" result):

GCONF       0x00:00:00:C0
IHOLD_IRUN  0x00:0A:19:0C
GSTAT       0x00:00:00:00
IOIN        0x21:00:02:41
TPOWERDOWN  0x00:00:00:80
TSTEP       0x00:0F:FF:FF
TPWMTHRS    0x00:00:00:00
TCOOLTHRS
THIGH
CHOPCONF    0x14:02:81:03
COOLCONF
PWMCONF 0xC8:0D:0E:24
PWM_SCALE   0x00:00:00:0E
DRV_STATUS  0xC0:0C:00:00
Testing X connection... OK

All other tests were performed with PSU control enabled. In each case the following steps were taken:

• Upload new firmware to the board.
• Reset the board.
• Run M502 followed by M500
• Reset the board again.
• Connect and do M80 to turn on the PSU.
• Run M122 X V

I tested the original code, moving the GCONF() call to the start of push(), adding a call to GCONF() to the start of push but leaving the original call as well, and having two consecutive calls to GCONF() at the start of push(). All produced identical M122 X V output:

GCONF       0x00:00:00:00
IHOLD_IRUN  0x00:0A:19:0C
GSTAT       0x00:00:00:01
IOIN        0x21:00:02:41
TPOWERDOWN  0x00:00:00:80
TSTEP       0x00:0F:FF:FF
TPWMTHRS    0x00:00:00:00
TCOOLTHRS
THIGH
CHOPCONF    0x15:02:81:03
COOLCONF
PWMCONF 0xC8:0D:0E:24
PWM_SCALE   0x00:00:00:1D
DRV_STATUS  0xC0:19:00:00
Testing X connection... OK

I also tested adding a call to mstep_reg_select() at the start of push() with the following results:

GCONF       0x00:00:00:80
IHOLD_IRUN  0x00:0A:19:0C
GSTAT       0x00:00:00:01
IOIN        0x21:00:02:41
TPOWERDOWN  0x00:00:00:80
TSTEP       0x00:0F:FF:FF
TPWMTHRS    0x00:00:00:00
TCOOLTHRS
THIGH
CHOPCONF    0x14:02:81:03
COOLCONF
PWMCONF 0xC8:0D:0E:24
PWM_SCALE   0x00:00:00:1D
DRV_STATUS  0xC0:19:00:00
Testing X connection... OK

So it definitely seems like the GCONF register (and possibly others) are not being initialized correctly. I've been looking through the Marlin code to see if there's any reason why the TMC driver init calls are not being executed but as of now I'm stumped. reset_stepper_drivers() should be called from the code that loads the settings from EEPROM, and that is definitely happening when the board powers up. And an M501 properly starts up the drivers as well. Very strange.

[ManuelMcLure]

I'm going to add some debug output to reset_stepper_drivers() and see if I can catch it being called on board startup. It will require some fast timing to connect PronterFace to the board after a reset, but hopefully I can see if it's actually being called or not.

[ManuelMcLure]

I added code to both tmc_init() and restore_stepper_drivers(). I made GCONF_register member of the TMC2208Stepper class public so I could display it from Marlin code. I see the following when Marlin starts up:

tmc_init called on 0x10:00:3C:EC GCONF is 0x00:00:00:C0 GCONF_register is 0x00:00:00:C0

and the following when I execute M80:

push() called on 0x10:00:3C:EC GCONF_register is 0x00:00:00:00

The code looks like this in tmc_init():

    st.GCONF(gconf.sr);
    SERIAL_ECHOPGM("tmc_init called on ");
    print_hex_long((uint32_t)&st, ':');
    SERIAL_ECHOPGM(" GCONF is ");
    print_hex_long(gconf.sr, ':');
    SERIAL_ECHOPGM(" GCONF_register is ");
    print_hex_long(st.GCONF_register.sr, ':');
    SERIAL_CHAR('\n');

and as follows in reset_stepper_drivers():

#if AXIS_IS_TMC(X)
    SERIAL_ECHOPGM("push() called on ");
    print_hex_long((uint32_t)&stepperX, ':');
    SERIAL_ECHOPGM(" GCONF_register is ");
    print_hex_long(stepperX.GCONF_register.sr, ':');
    SERIAL_CHAR('\n');  stepperX.push();
  #endif

so somehow the value of GCONF_register in the stepper object is being lost. The first hex number on each line is the address of the TMC2209Stepper object and they are both the same, so something is clobbering the GCONF_register value between the two calls.

I think what might be happening is that we are calling GCONF() to read some value back when we can't communicate with the driver, and that is causing GCONF_register to be clobbered. See the following code:

uint32_t TMC2208Stepper::GCONF() {
    if (write_only) return GCONF_register.sr;
    GCONF_register.sr = read(GCONF_register.address);
    return GCONF_register.sr;
}

This unconditionally clobbers the GCONF_register variable. I think this is caused by the call to get_stealthChop_status() to display theM569 line in the report of data loaded from EEPROM. This calls TMC2208Stepper::en_spreadCycle which in turn calls GCONF() and clobbers the register.

I don't know what the best solution to this might be. It obviously makes sense to try to update the register if possible, but it leads to this clobber in case we can't connect to the driver. Maybe only update GCONF_register if the read didn't return all 0s?

[ManuelMcLure]

I tried a quick-and-dirty fix:

uint32_t TMC2208Stepper::GCONF() {
    GCONF_t gc;
    if (write_only) return GCONF_register.sr;
    gc.sr = read(GCONF_register.address);
    if (!CRCerror)
        GCONF_register.sr = gc.sr;
    return GCONF_register.sr;
}

which seems to fix the problem, at least on my end.

[Haschtl]

I assume this will Not work for the 5160, but only for the 2208?

[ManuelMcLure]

Correct - there will need to be more changes for the 5160. Might be very similar, though.

[ManuelMcLure]

It's possible that you can do something similar with status_response in TMC2130Stepper::read() to fix the problem on the 5160, but I'm not really familiar with SPI so someone else will have to confirm.

[ManuelMcLure]

Does TMC2208Stepper::FACTORY_CONF() also need a similar change?
I noticed that only FACTORY_CONF() and GCONF() seem to have the potential clobbering behavior. The other functions either return the register without reading it or read into a temporary variable and return it without overwriting the register.

[teemuatlut]

It depends on the type of register. If the reg is write only, the we will always just return the cached value. If the register is read-only then we don't really need to cache the register but we can make a read & parse operation.
Similar with RW register, of which there aren't many, that after reading should we update the cached value against which future bit changes are made. The read back value is of course the true state of the register but there may be times when it doesn't match with our cached values.
Your change makes sense so that we favor our intended register status (cache) instead of the true status in the driver (read value).

[delwinbest]

Correct - there will need to be more changes for the 5160. Might be very similar, though.

Hi Manuel. I'm a little lost when it comes to the relation between MC2208Stepper::GCONF() and the TMC2209. Will this fix apply to the TMC2209 devices too? Are would it need to be replicated for these? Thanks for the work here. Took me a while to figure out why stealthchop was not enabled until I saw this.,

[wap32]

This unconditionally clobbers the GCONF_register variable. I think this is caused by the call to get_stealthChop_status() to display theM569 line in the report of data loaded from EEPROM. This calls TMC2208Stepper::en_spreadCycle which in turn calls GCONF() and clobbers the register.

I don't know what the best solution to this might be. It obviously makes sense to try to update the register if possible, but it leads to this clobber in case we can't connect to the driver. Maybe only update GCONF_register if the read didn't return all 0s?

Okay, so this was the same as #13092 after all, except in that case it was with TCM2130.

The way I solved it on the Marlin side was to simply to check if the PSU is on before any calls the offending function, like (powersupply_on ? st.en_pwm_mode() : false).
Since we know we will not get any meaningful values in the first place since there's no power, it does not make sense to call the function, and this should work (I guess) for all the affected TMC stepper models.

[ManuelMcLure]

Will this fix apply to the TMC2209 devices too?

Yes. TMCStepper2209 is a subclass of TMCStepper2208 and doesn’t define its own GCONF() method so it inherits its parent’s method.

[AnHardt]

There is still a loophole. Power may not be stable enough until several ms after the PS was switched on.
Maybe it needs a power_good flag rissen POWER_GOOD_DELAY_MS after M80 and reset immediately with M81.
Or M80 must block for long enough. But i hate all busy waiting.

[teemuatlut]

Maybe I'll add a offline flag or something into the library so that everything works in the command chain until it comes time to write something. And read commands can return the shadow register if there is one available or 0 if not.
Or maybe read commands shouldn't modify the shadow registers anymore.

[ManuelMcLure]

The M80 code already has a 100ms delay before calling restore_stepper_drivers().

[AnHardt]

1/10 second = 10 AC half waves (50Hz) -> could need more in some cases.

[ManuelMcLure]

It might make sense to have the ability to test a POWER_OK pin - most power supplies that can be controlled via a PS_ON pin also have a pin that allows them to say that the power is stable. The PSU_ON function could wait until that pin becomes active before returning.

[enricomilan]

I have the same problem but with TMC2130 it started when i setup the hardware for use the PSON function so I have checked my hardware several times believing it was a problem of poor power or electrical noise of some kind but the tools say that everything is in order I feel relieved to read this issue and I kindly ask you to upload a mini guide on how you solved the problem ... unfortunately I'm not very good with the software

[ManuelMcLure]

There's a new version of the TMCStepper library that fixes this issue. Try deleting the ".pio" folder from your Marlin checkout and rebuilding so it grabs the new version of TMCStepper.

[ManuelMcLure]

Since the original problem is fixed by the new version of TMCStepper provided by @teemuatlut I will close this issue. I think it still makes sense to look into the feasibility of using a POWER_OK pin (if available) to ensure that power is stable before doing anything that requires full power, but that can be discussed in a different issue or in a pull request.

[sebastian-be]

Hey. I still have the issue of TMC (2209's) misbehaving after 24V power-off / power-on cycle (board "SKR Mini E3 v1.2" & Marlin 2.0.4.4 remain active, powered by separate usb power source), and that with the current TMCStepper drivers v0.6.2. Is that a regression or some other issue I'm suffering from? Thx

M501 DOES fix it as mentioned before...

edit: added some details

[ManuelMcLure]

How are you cycling the power? Unless you're using the PS_ON pin and controlling it with M80/M81 or the LCD "Switch Power On/Off" option Marlin doesn't know that the power is back and can't reinitialize the TMC drivers.

If you switch the power on with the PS_ON pin Marlin will be smart and reinitialize the drivers automatically when you do M80 or "Switch Power On".

[sebastian-be]

Thx for feedback. In my case I'm triggering power-status change from octoprint. I'll try that.

[ManuelMcLure]

OctoPrint can either control power via sending M80 to Marlin, or it can use a GPIO pin on the Raspberry Pi to control power. If it is doing the latter then Marlin doesn't know about the change in power status.

[sebastian-be]

I was indeed using gpio's, and will retry with "through Marlin". Thx for info

[tureka]

I still suffer the same issue. After M80 with USB connected PSU is starting but steppers are inactive. After antoher M80 movement is possible. M81 is turning power off as expected. I've checked using Pronterface, Repetier-host (auto M80 when connected) and Octoprint. My steppers are TMC5160s and I run latest bugfix with TMCStepper 0.6.2. I temporary resolved this issue by double M80 using macro in Pronterface:M80 !time.sleep(3) M80.
I didn't check stable Marlin yet. M80 S shows PS:1 when powerd on and PS:0 when powered off. M122 does not return any errors after first M80 or after following one.

[ManuelMcLure]

Instead of modifying the code you can just change PSU_POWERUP_DELAY in Configuration.h

[tureka]

Instead of modifying the code you can just change PSU_POWERUP_DELAY in Configuration.h

Tried that too. I even made a delay 1000ms. It is not helping. In my macro i give 3s before second M80.

[thinkyhead]

Tried that too. I even made a delay 1000ms. It is not helping. In my macro i give 3s before second M80.

So… it sounds like a value of 3000 might work, then?

[ManuelMcLure]

@thinkyhead With a 3000ms delay we're getting pretty close to the watchdog timeout. Do you think we should switch to using safe_delay() instead of delay() in the power-on code to avoid problems?

[github-actions[bot]]

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.