tmc5160.py does not update CS values

[HonestBrothers]

This uses the tuning spreadsheet to calculate CS values versus globalscaler.

pg. 74 of the datasheet shows that Irms is calculated by the tmc5160:

Irms = (globalscaler/256) ((CS+1)/32) (Vfs/Rsense) * (1/sqrt(2))

This leaves us with one equation and two unknowns, which results in an unsolvable equation. The current code in Klipper then ignores the CS value and solves the above equation for global scaler:

globalscaler = int((current 256. math.sqrt(2.) * self.sense_resistor / VREF) + .5)

Effective what this does, since multiplying by 1 results in the same number that we started with, is sets ((CS+1)/32) = 1. This means that CS is 31.

Irms = (globalscaler/256) ((31+1)/32) (Vfs/Rsense) * (1/sqrt(2))

Irms = (globalscaler/256) 1 (Vfs/Rsense) * (1/sqrt(2))

Klipper then back calculates the CS value. The problem with this is the way the driver implements this is it uses BOTH CS and Globalscaler in the same equation. So we can't back calculate because those maths don't math. One equation, two unknown functions are unsolvable, we would need two equations for this.

[richfelker]

Can you clarify what's functionally wrong right now? Is the actual current significantly different from the requested current because of this bug? Are there other symptoms?

[HonestBrothers]

The PR I raised on the main branch outlines the issue. Essentially the current is slightly different (not enough to make a difference), but the big issue is the chopper hysteresis can't be properly set in many circumstances without access to change CS.

Conversation with Dmitry basically boiled down to the fact that we may just need to expose the CS value as a parameter we can set, instead of auto calculating it. Auto calculating will screw up every tmc5160 driver chopper setting on every instance this is pushed to.

But, the way it's set right now, the settings are likely already screwed up.

https://github.com/Klipper3d/klipper/pull/6644

[dalegaard]

Hi,

So there seems to be some misunderstandings about why this change is needed(especially in the mainline discussion). Basically it boils down to chopper hysteresis settings only being applied to the CS term only. In cases where extreme hysteresis is needed in relation to the CS value, reducing CS can get the hysteresis values back to within limits. The lost current can then be made up by setting globalscaler higher to match.

Only very few motors should require this, but I think this is overall a good change. Having CS=31 be the default, so CS never becomes 30 anymore, seems fine to me.

Best regards, Lasse

[HonestBrothers]

Hi,

So there seems to be some misunderstandings about why this change is needed(especially in the mainline discussion). Basically it boils down to chopper hysteresis settings only being applied to the CS term only. In cases where extreme hysteresis is needed in relation to the CS value, reducing CS can get the hysteresis values back to within limits. The lost current can then be made up by setting globalscaler higher to match.

Only very few motors should require this, but I think this is overall a good change. Having CS=31 be the default, so CS never becomes 30 anymore, seems fine to me.

Best regards, Lasse

Feel free to comment on the mainline branch, if you want. I end up going on too many tangents.

I don't think the change will be needed often, but those that want to run high end motors will benefit. Defaulting to 31, in my opinion is fine, because under the current implementation you'd be waaaay outside the boundaries of the driver when CS switches to 30. Like scarecrow keeps saying, you really need to change RSense or switch drivers at that point.

I don't believe the mainline code would behave in the way Trinamic intended it to, even if the code didn't switch to CS=30 at Globalscaler <= 32. That's one of the tangents I keep going on, but have probably poorly explained.

[CarVac]

Matt the Printing Nerd has found that not lowering CS causes noticeably lower performance on 5160 drivers.

Here's what he had to say:

Combine that with a fast motor... and a board that has a low Rsens and you end up spending a week on debugging why your 4 motors perform way worse than your two motors on your T100

He made his own fork of Klipper which auto-calculates CS by lowering it first, then adjusting globalscaler.

https://github.com/MSzturc/klipper/blob/master/klippy/extras/tmc5160.py

[MSzturc]

I don't think the change will be needed often, but those that want to run high end motors will benefit. Defaulting to 31, in my opinion is fine, because under the current implementation you'd be waaaay outside the boundaries of the driver when CS switches to 30. Like scarecrow keeps saying, you really need to change RSense or switch drivers at that point.

I don't believe the mainline code would behave in the way Trinamic intended it to, even if the code didn't switch to CS=30 at Globalscaler <= 32. That's one of the tangents I keep going on, but have probably poorly explained.

That's 100% true for TMC2209 drivers, but in my point of view it does not fit for TMC5160

Most TMC5160 drivers come with low Rsens resistors, like 0.050 or 0.022 ohm and since people use them to drive more premium motors like 2504er, 2804er, CM06 or CM08, all these motors have a coil resistance/inductance that requires a cs lower than 31.

The current TMC5160 implementation makes it impossible to use TMC5160 with klipper properly. I did over the last few weeks multiple tests and the results where shocking. On my current printer configuration (4x2804 on a BTT Kraken) I was not able to reach speeds over 1200mm/s at 130k acceleration with the standard klipper behaviour (cs=31), Compared to a configuration with cs=9 that setup tapped out at 1600mm/s at 280k acceleration which is more than double.

Also I would prefer to implement an auto calculation of cs since honest every stepper on a tmc5160 will profit from that optimisation I did a lot of calculation and even the cheapest motors for $4-5 combined with the worst tmc5160 driver (1st gen with 0.075 rsens) will need lower cs values in every high performance and every silent szenario

[HonestBrothers]

@MSzturc that's some really good data.

The problem with auto-calculating is it screws up all the existing implementations of the 5160 code deployed. Suddenly everyone will need to recalibrate their drivers.

[Andor]

@HonestBrothers is it possible to add option "auto" for this parameter in the config? Or maybe "magic" value like -1 which will mean auto if adding a string value instead of integer one is difficult.

[HonestBrothers]

@HonestBrothers is it possible to add option "auto" for this parameter in the config? Or maybe "magic" value like -1 which will mean auto if adding a string value instead of integer one is difficult.

I like that idea. Maybe we can set tmc_cs: -1, as auto or even 0 (I need to read the datasheet, I think 0 would be fine). Default value will be 31 to not screw anything up. And then if people put auto, it'll auto calculate. Otherwise they can manually select a value.

I'll see if I can find time to implement that. Haven't had time to work on the printer lately.

[MSzturc]

@MSzturc that's some really good data.

The problem with auto-calculating is it screws up all the existing implementations of the 5160 code deployed. Suddenly everyone will need to recalibrate their drivers.

I think this point is debatable. With the current implementation the people had to choose settings to compenbsate the flaws in the implementation. Those settings increase the power dissipation for the driver. To use a metaphor you drive a car with a gas pedal that only knows 0 and max position and since that is an issue they choose to limit the fuel that gets injected to the engine (globalscaler). So basically in the current setup people roasting their drivers, shorten their lifespan, increase artefacts on their prints.

I personally would classify this bug as critical and therefore I would suggest implementing a non backward compatible implementation. But as I said it's debatable and I also understand the standpoint of the klipper guys maneuver around the shitstorm that change would cause. Therefore suggested an auto version that defaults to 31 in the codereview

[HonestBrothers]

@MSzturc I agree that the power dissipates in the driver, but immediate harm might come to the motors/drivers if we switch to auto-calculating all the sudden.

I've blown one of these drivers by switching settings and making no hardware changes.

[MSzturc]

@MSzturc I agree that the power dissipates in the driver, but immediate harm might come to the motors/drivers if we switch to auto-calculating all the sudden.

I've blown one of these drivers by switching settings and making no hardware changes.

I'm pretty sure that changing the CS would not damage your drivers since the global scaler limits the potential damage, but on the other hand I don't know where all the chinese board manufacturers are cutting corners on their low budget printer boards, so defaulting to 31 might be a good idea for klipper standard,

[CarVac]

Damage can occur from ringing and from too-short break-before-make time (which exists to prevent the driver from shorting the mosfets out).

[HonestBrothers]

I updated everything.

1. tmc_cs: 0 Sets the system to auto-calculate
2. tmc_cs: 1-30 Lets the user choose the tmc_cs value
3. tmc_cs: not defined or 31 Default condition.

CS value is updated for ihold if defined.

Code should be ready to merge.

[MSzturc]

I updated everything.

1. tmc_cs: 0 Sets the system to auto-calculate
2. tmc_cs: 1-30 Lets the user choose the tmc_cs value
3. tmc_cs: not defined or 31 Default condition.

CS value is updated for ihold if defined.

Code should be ready to merge.

It might be a bit pedantic but it would be easier for people to find that setting when it's part of the klipper naming convention, starting with driver_

Also even it's not a realistic szenario technically cs = 0 is a valid value regarding to the documentation of TMC

[HonestBrothers]

@MSzturc I will update to fix the naming convention.

Not sure how I feel about CS=0. I suppose I should code in accordance to the datasheet. I will change this to -1.

[MSzturc]

@MSzturc I will update to fix the naming convention.

Not sure how I feel about CS=0. I suppose I should code in accordance to the datasheet. I will change this to -1.

Yeah I know, it's unrealistic that cs=0 will be ever needed but im sure that in the future there will be a person looking into the code and being confused, so at least add a comment

[HonestBrothers]

Changed.

driver_cs: -1 Sets the system to auto-calculate driver_cs: 0-30 Lets the user choose the tmc_cs value driver_cs: not defined or 31 Default condition.

[CarVac]

Reminder to remove the attribution per @bwnance 's requested change.

[HonestBrothers]

@CarVac didn't realize it was still there. I reverted to an old version before I uploaded.

[Zeanon]

@HonestBrothers Do you know how to apply the same thing to tmc2240 Since they suffer from the same issue

[HonestBrothers]

@Zeanon

@HonestBrothers Do you know how to apply the same thing to tmc2240 Since they suffer from the same issue

I will look at the code in a couple weeks and give it a go. Should be fairly trivial. I don't own these drivers though, so I may need your support in testing.

I'm in the middle of moving houses and my entire system is down for the near term. Got to get my old house fixed and on the market, and then set up my office and I'll have some time to work on it.

[rogerlz]

Hi,

I am closing this one in favour of a new implementation here: https://github.com/DangerKlippers/danger-klipper/pull/374