Marlin: [BUG] StealthChop / SpreadCycle inverted with hardwired SPREAD pin

[Loreagle]

I have the problem that from printer display when I enable StealthChop actually is the opposite like you can see in this video: https://youtu.be/XtCDxxT0iKw

My config: https://github.com/Loreagle/Marlin/tree/Sapphire-Pro

Also from terminal the behaviour is inverted, as you can see if I send M569 S0 X Y Z E I have StealthChop enabled instead of disabled...

Here the registers

I'm using an MKS Robin Nano with MKS35 LCD and TMC2209 from Eryone connected in software UART mode.

[thinkyhead]

I've just tested M569 on an EZBoard and there the command produces the expected result.

Is there something special about these Eryone brand stepper drivers that makes them different from the standard?

[teemuatlut]

[Loreagle]

So it could be that I have high level on the SPREAD pin on my Robin Nano?

As @teemuatlut posted and said on Discord, 2209 have opposite logic compared to 2130.

[thinkyhead]

2209 having opposite logic compared to 2130

Possibly, since the board I tested also has TMC2209 in UART mode and it worked correctly.

Ok… so:

• Should we be reading the "SPREAD" pin state before setting or interpreting the meaning of the spreadCycle / stealthChop bit (2) shown in this image and inverting the value sent to M569 S<state> when SPREAD is HIGH?

Or, is everything working "correctly" but we are not making it clear that M569 S<state> can sometimes have the opposite meaning depending on other user-configurable or hardware-configurable settings?

[teemuatlut]

The easiest solution is to have the user not pull the pin high, but that leaves it open for some vendor to be clever and deviate from current convention.
So basically everything is working as designed but the design doesn't taken into account that you can invert the logic with a hardware modification.

If you want to go the way of adding a query for XORing the logic, it might be enough to update just the refresh_stepping_mode and get_stealthChop_status methods. M122 too for extra bonus points.

[Loreagle]

The easiest solution is to have the user _not_ pull the pin high, but that leaves it open for some vendor to be clever and deviate from current convention.
So basically everything is working as designed but the design doesn't taken into account that you can invert the logic with a hardware modification.

If you want to go the way of adding a query for XORing the logic, it might be enough to update just the refresh_stepping_mode and get_stealthChop_status methods. M122 too for extra bonus points.

I didn't pull the pin high, if it's high is a board config.

So at the moment if I enable //#define HYBRID_THRESHOLD I have the opposite situation?

[teemuatlut]

StealthChop will always work in a lower speed range compared to spreadCycle. It's not possible to switch their order.

[Loreagle]

StealthChop will always work in a lower speed range compared to spreadCycle. It's not possible to switch their order.

I was thinking that since I have modes inverted maybe that will invert too...

[thinkyhead]

The main thing here is we want the interface to reflect the real situation of the stealthChop versus spreadCycle state. So, if it's a hardware change that we cannot detect, then it makes sense to add a configuration option to tell Marlin that this modification has been made, and then Marlin can handle setting / displaying the state correctly.

So with the hardware modified, when we send a "1" to change the state, it seems that now it will read back as a "0" and if we send a "0" it will read back as a "1." So, we should only change how we display the value, not how we interpret the S parameter.

So, add this to config…

#define TMC_HARDWIRED_SPREAD

…and make this change to the M569 report…?

template<typename TMC>
void tmc_say_stealth_status(TMC &st) {
  st.printLabel();
  SERIAL_ECHOPGM(" driver mode:\t");
  serialprintPGM(st.get_stealthChop_status() ^ ENABLED(TMC_HARDWIRED_SPREAD) ? PSTR("stealthChop") : PSTR("spreadCycle"));
  SERIAL_EOL();
}

[thinkyhead]

I've posted the PR #19827 in an attempt to address this kind of hardware configuration. Please have a look and see what you think.

[thinkyhead]

After getting some feedback from Teemu et. al., the PR has been modified to check the spread_en() state and return the stealthChop state inverted if the state is HIGH. Please test to see that it produces the correct results on your setup from the LCD and from G-code commands.

[Loreagle]

After getting some feedback from Teemu et. al., the PR has been modified to check the spread_en() state and return the stealthChop state inverted if the state is HIGH. Please test to see that it produces the correct results on your setup from the LCD and from G-code commands.

I can confirm that setting StealthChop from the printer display works well and is fixed, the problem is that now on M569 the value is inverted as you can see here:

[thinkyhead]

I checked the code and saw that M569 wasn't yet accounting for spread_en when _setting_ the state, although it seems correct in _reading_ the state. So, I updated that PR and now possibly M569 is behaving better…?

[Loreagle]

I checked the code and saw that M569 wasn't yet accounting for spread_en when _setting_ the state, although it seems correct in _reading_ the state. So, I updated that PR and now possibly M569 is behaving better…?

I tested and M569 is still working at the opposite as you can see in my screenshot. The registers seems to be settled correctly after commands S1 and S0 but @teemuatlut can confirm better than me.

The display of the printer works well btw, displaying the real status :)

[thinkyhead]

What is the output of M122 with no V parameter? I see that for TMC220x there is a st.stealth() method, and this is the function called by the M122 report for the "stealthChop" reading on those drivers.

I want to contrast the value that comes back from stealth() (GET_REG(TMC2208_n, stealth)) with the reading from get_stealthChop_status. It seems like we should just be calling the stealth() accessor in TMCStepper<TMC2208>, but this is what has evolved…

inline bool get_stealthChop_status() {
  return !(this->en_spreadCycle() ^ this->spread_en());
}

I'm going to make a couple more tweaks to the PR, such as adding spread_en to the M122 report, and then from this last round of debug hopefully we'll sort out the most proper way to access and interpret this status.

[thinkyhead]

I've inverted the method so it's better for EEPROM storage. The internal storage of "stealth" will now always be 1 when "stealth" is the wanted effect. But the state will be inverted when setting or reading the driver bit directly if spread_en is high.

[Loreagle]

What is the output of M122 with no V parameter? I see that for TMC220x there is a st.stealth() method, and this is the function called by the M122 report for the "stealthChop" reading on those drivers.

I want to contrast the value that comes back from stealth() (GET_REG(TMC2208_n, stealth)) with the reading from get_stealthChop_status. It seems like we should just be calling the stealth() accessor in TMCStepper<TMC2208>, but this is what has evolved…

inline bool get_stealthChop_status() {
  return !(this->en_spreadCycle() ^ this->spread_en());
}

I'm going to make a couple more tweaks to the PR, such as adding spread_en to the M122 report, and then from this last round of debug hopefully we'll sort out the most proper way to access and interpret this status.

I'm sorry but I'm not really good into coding so I can't fully follow you :(

Btw this is the M122 output with the same firmware and settings as my latest screenshots:

Now I updated with your last commit and M569 is still broken... it shows the opposite status.

[teemuatlut]

.stealth() is for reading the current status of the chopper method and so is affected by hybrid threshold. You cannot use this method for this purpose.

[thinkyhead]

Thanks! I figured that was the distinction.

[thinkyhead]

Now I updated with your last commit and M569 is still broken... it shows the opposite status.

I added a spread_en element to the M122 report but it only shows up without the V parameter. On your setup it should always show up as true.

So it's interesting that you're seeing the opposite status now. The code was according to my best understanding of the situation yesterday. So, maybe the inversion of the bit is only when it is written…?

Since there are only a couple of possibilities… and the current one is wrong…

inline bool get_stealthChop()        { return !(this->en_spreadCycle() ^ this->spread_en()); }  // If SPREAD_EN is HIGH the mode is reversed
inline bool get_stored_stealthChop() { return this->stored.stealthChop_enabled; }
inline void refresh_stepping_mode()  { this->en_spreadCycle((!this->stored.stealthChop_enabled) ^ this->spread_en()); }

…then the only other reasonable possibility must be right… right?

If the spread_en status only inverts the _outcome_ on write but not the _meaning_ on read, the code should be this instead:

inline bool get_stealthChop()        { return !this->en_spreadCycle(); }
inline bool get_stored_stealthChop() { return this->stored.stealthChop_enabled; }
inline void refresh_stepping_mode()  { this->en_spreadCycle((!this->stored.stealthChop_enabled) ^ this->spread_en()); } // If SPREAD_EN is HIGH the outcome is reversed

I suspect that is the case, but I will actually check the datasheet if I must! Anyway, I'll push an update with the latter block of code and hopefully both the G-code and the LCD will then behave properly! If the M122 (no V) report shows all the spread_en as true that will be good.

[teemuatlut]

After writing to a register, a read from the same should result in the same exact value.

Btw, this PR may need to also address the other use cases of en_spreadCycle in tmc_util.cpp: tmc_enable_stallguard and trinamic.cpp. I think my initial suggestion of overloading the method might've inadvertently taken this into account.

[thinkyhead]

After writing to a register, a read from the same should result in the same exact value.

You would think so. But the initial report based on our old code is clearly writing a "1" expecting this to enable spreadCycle mode, but instead it is having the opposite effect.

Hopefully someone familiar with the technical details can jump in with the exact code and simply post the correction, as I am of course chasing several issues today and don't have time for too much back-and-forth to work it out in the abstract.

[Loreagle]

I'm not very good at using github and I made a lot of confusion, I don't understand how can I revert back to before all this changes, can you please point me in the right direction? I just want to go back and sync again with bugfix.

I'm using Github desktop and as you can see here I made a lot of confusion :( https://github.com/Loreagle/Marlin/commits/Sapphire-Pro

[thinkyhead]

I'm not very good at using github…

To get your working copy and fork's branch bugfix-2.0.x into sync use the Git console with these commands:

git fetch upstream
git fetch origin
git checkout bugfix-2.0.x
git reset --hard upstream/bugfix-2.0.x
git push -f origin

And before making changes that you might want to submit, always create a new branch as a copy of upstream/bugfix-2.0.x so you don't trounce your bugfix-2.0.x branch:

git fetch upstream
git checkout upstream/bugfix-2.0.x -b my_work_branch

[thinkyhead]

Another good idea to keep in proper sync is to set the the upstream repo of your bugfix-2.0.x branch to "upstream":

git checkout bugfix-2.0.x
git branch --unset-upstream
git branch --set-upstream-to upstream/bugfix-2.0.x

Then GitHub Desktop will automatically offer you the latest changes when you select the branch.

[Loreagle]

I'm more confused than before because I don't know the commands meaning, btw thank you a lot for the explanation. I'm using GithHub app and I work using the app features, never with command prompt...
The problem is that I have my "Sapphire Pro" branch (where I made all the changes in configs etc.) that is messed up now as you can see in this photo:

The question is if I can go back to a specific date (before some changes) with a specific command.

I suppose the best thing to do is to create a new branch of bugfix 2.0.x and copy my config files, easier for me 😅

[thinkyhead]

Git has lots of powers, but this isn't the appropriate place to request a Git tutorial. There are zillions of videos and articles here on GitHub and elsewhere that can help you to master Git within a few hours.