Various TMC improvements

[leptun]

Summary of changes:

A ticked box means that a separate PR has been made with this change.

• [ ] Moved the initialization of the microstep settings in a place that makes more sense. This was needed because the TSTEP helper relies on the mres value to already be loaded.
• [ ] Added a boat load of fields to the config for the drivers besides tmc5160. For some reason only tmc5160 was very configurable, but now all of the drivers are.
• [x] Added the ability to specify the clock frequency of the tmc driver. This information is only used for calculating fields with the TSTEP helper. This option is useful if the driver has an external clock attached. - #6158
• [x] Configurable vhigh (THIGH) - #6139
• [x] Configurable vcoolthrs (TCOOLTHRS) - #6139

---

Already merged:

• [x] TMC5160 specific: Changed the way the current fields for the driver are calculated when the current is changed at runtime.
• [x] Code reorganization. The tmc2130 code and tmc220x code was a bit messy in the intialization method. tmc5160 init code was much more beautiful, so I made the other tmc code look like the tmc5160 code.
• [x] Fixed the s2vs field formatters. So instead of the misleading LowSideShort_A, it's now ShortToSupply_A (and the same for the B coil). Also implemented this field formatter for TMC5160.
• [x] TSTEP helper: Internal code for computing other fields such as TPWMTHRS, TCOOLTHRS, THIGH, etc...
• [x] Configurable homing_vcoolthrs - can be implemented with macros just like the homing current: https://github.com/Klipper3d/klipper/pull/6104#issuecomment-1464980909

---

Scrapped:

• [x] Added small_hysteresis and multistep_filt as config options for drivers that support it. - This was scrapped since few users will ever need to change these fields. In those cases, a delayed gcode can be used at startup to setup the fields. The patch for tmc2240 and tmc5160 default value was merged with PR #6118.
• [x] Ability to specify a homing current value. ~This current is automatically applied at the beginning of the homing move and the old current is restored at the end of homing. This both improves the StallGuard homing reliability (more tuning possible) and it makes printers safer (less chances of damage during homing if it fails).~. This idea was scrapped in favor of using macros. See https://github.com/Klipper3d/klipper/pull/6104#issuecomment-1464980909

---

For more details, read the commit comment and/or the documentation

All of these features combined should improve the reliability of StallGuard homing and make the tmc drivers more configurable in general. It also makes it possible to use CoolStep with all drivers that support it. The homing related improvements were tested on a Prusa MK3S. I do not have a way of testing anything besides tmc2130 and tmc2240 (todo) at the moment.

[KevinOConnor]

Thanks. As high-level feedback, some of these changes look fine and some I'm unsure of. It might be easier to break this up into a couple of PRs.

I have some comments on the individual commits.

One recurring review comment I have is that it's unclear to me how some of these changes enable users to accomplish real-world tasks, what those tasks are, and who those users are. (There are a few blurbs on this at bit at https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review ). It would help to understand the intent and how the code is being used.

TMC5160 specific: Changed the way the current fields for the driver are calculated when the current is changed at runtime.

In general it looks fine to me. It looks to me like the code has a defect though (see followup message). Also, this alters the behavior of the driver, as the tmc specs state that a change to GLOBALSCALER impacts the drivers auto-calibration. As such, I think this change should be accompanied by an update to docs/Config_Changes.md .

This change doesn't seem related to the other changes, so may make sense to separate out.

Moved the initialization of the microstep settings in a place that makes more sense. This was needed because the TSTEP helper relies on the mres value to already be loaded.

This increases code duplication and it's not clear to me why it is being done.

Added small_hysteresis and multistep_filt as config options for drivers that support it.

It's not clear to me what end users would configure this and what real-world tasks it enables them to accomplish.

Code reorganization. The tmc2130 code and tmc220x code was a bit messy in the intialization method. tmc5160 init code was much more beautiful, so I made the other tmc code look like the tmc5160 code.

Seems fine to me.

Added a boat load of fields to the config for the drivers besides tmc5160. For some reason only tmc5160 was very configurable, but now all of the drivers are.

I see an advantage in being able to configure coolstep, but this commit doesn't seem to enable that (as it lacks setting the coolstep threshold). Otherwise, same comment as earlier - it's not clear to me what end users would configure these fields and what real-world tasks it enables them to accomplish.

Added the ability to specify the clock frequency of the tmc driver. This information is only used for calculating fields with the TSTEP helper. This option is useful if the driver has an external clock attached.

It's not clear to me how many users would have hardware that needs this. I fear it adds complexity and increases the chance of a misconfiguration.

Ability to specify a homing current value. This current is automatically applied at the beginning of the homing move and the old current is restored at the end of homing. This both improves the StallGuard homing reliability (more tuning possible) and it makes printers safer (less chances of damage during homing if it fails).

I don't think this is a good direction for Klipper to take. In my experience homing is a delicate, step-by-step process that the hardware must make to ensure good positional accuracy.

I think printer designers and power users would be better served by specifying these detailed steps in one place - a homing macro.

This commit adds a config option that alters the homing process. However, I fear this increases the challenges for both end-users and printer designers as it effectively increases the number of places in the config that alters homing behavior. To wit, we don't want to require users to study the entire Config_Reference.md document to understand what occurs during homing.

TSTEP helper: Internal code for computing other fields such as TPWMTHRS, TCOOLTHRS, THIGH, etc...

Adding a helper seems fine to me. It seems odd that the helper function can take a velocity of None or a velocity less than zero. It would seem to me that the caller should check for these things (and it seems that they do).

Configurable vhigh (THIGH)

It's not clear to me what end users would configure this and what real-world tasks it enables them to accomplish.

Configurable vcoolthrs (TCOOLTHRS)

Adding support for coolstep seems useful to me. FWIW, it would probably be easier to review in its own PR.

Configurable homing_vcoolthrs

As above, I don't agree with this change.

Fixed the s2vs field formatters. So instead of the misleading LowSideShort_A, it's now ShortToSupply_A (and the same for the B coil). Also implemented this field formatter for TMC5160.

Seems fine to me. Though if we change the descriptions, we may need to also update the TMC_Drivers.md document.

Thanks again for working on this. -Kevin

[leptun]

Sorry for the delay. I'm quite busy at the moment, so I'll have to postpone dealing with this PR for a few days. I'll try my best to answer all questions when I return to this and I'll also make PRs for the individual features which can be isolated.

[leptun]

@KevinOConnor I've created a few PRs based on this PR. You can see them linked above. I've also addressed the issues reported in the respective PRs. I will not backport the changes to this branch. As for the other questions, I will address those as soon as I have time. I am also still waiting for permission to talk about one of the changes/questions (can't because of NDA).

[leptun]

tmc: homing current and tmc: homing_vcoolthrs are now being handled by PR #6104 and macros

[andrewmcgr]

Configurable vhigh (THIGH)

It's not clear to me what end users would configure this and what real-world tasks it enables them to accomplish.

I have a use case: experience with the Bigtreetech SB2240, which so far as I am aware is the first readily available TMC 2240 device, shows that this board does not work reliably without setting THIGH to a large value (it fails with a charge pump undervoltage error). Now, it's a toolhead board, and thus never moves particularly fast, but this shows that at least some configurations need it to work correctly at high torque/low RPM.

[leptun]

@andrewmcgr Setting THIGH to a large value means that the driver switches to high velocity mode at a really low velocity. As such, StealthChop2 also gets disabled above a really low velocity. Have you tried disabling StealthChop on the driver via the stealthcop_threshold setting and running exclusively in SpreadCycle mode?

[andrewmcgr]

So it does, and I realise now my configuration is not doing at all what I thought.

Do you happen to have any insight into that charge pump undervoltage error?

[leptun]

I don't have a SB2240 board and I've never encountered the error on tmc2240 eval boards. The only time I got uv_cp error was because of a design error which resulted in undervoltage (~4.0V) on the VCC pin of a tmc2130 since the charge pump is powered from that pin on that driver. However, this power input doesn't exist on the tmc2240, so something else is not working correctly.

[KevinOConnor]

What is the status of this PR? Is it just tracking other PRs or is there code to be reviewed here?

Cheers, -Kevin

[leptun]

@KevinOConnor Just for tracking the other PRs. You may close it if you want. I'll keep the branch intact anyway just in case I need something from here in the future.

[github-actions[bot]]

It looks like this GitHub Pull Request has become inactive. If there are any further updates, you can add a comment here or open a new ticket.

Best regards, ~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.