search

Klipper3d / klipper

Klipper is a 3d-printer firmware
GNU General Public License v3.0
9.33k stars 5.28k forks source link

tmc5160: Increase maximum current error check #6395

Closed KevinOConnor closed 10 months ago

KevinOConnor commented 10 months ago

It's possible to build and configure tmc5160 drivers with external mosfets that support more than 3 amps. The actual maximum for tmc5160 drivers is dependent on how the board is wired and the mosfets used. Increase the error check to 10 amps. This error checking is primarily intended to catch "obvious misconfigurations" (eg, specifying milli-amps instead of amps), and the new value of 10 amps should suffice for this task.

This is an alternative to #6381 (and #6093).

@bigtreetech - FYI.

-Kevin

shaneshort commented 10 months ago

Doesn't this just kick the can down the road though?

I personally much prefer the BTT implementation having the maximum current defined by the sense resistor value, which for all intents and purposes is the most correct way of defining it?

If the main reason is to stop people putting in an absolutely insane value (which I completely understand) will they also have to adjust the sense resistor value in order to make that change meaningful?

If your concern is that the error message isn't clear enough, can we adjust the error message to suit?

I had originally raised this on the discourse but didn't get any feedback-- but another alternative is a configuration setting allowing what we could consider defining some kind of 'high power mode' parameter to relax these kinds of restrictions for people who know what they're doing.

thijstriemstra commented 10 months ago

I personally much prefer the BTT implementation having the maximum current defined by the sense resistor value, which for all intents and purposes is the most correct way of defining it?

If your concern is that the error message isn't clear enough, can we adjust the error message to suit?

Agreed.

KevinOConnor commented 10 months ago

Thanks for the feedback. I decided to merge this. Mainly because I don't think merging this change will hamper any alternative. (That is, the choice of 3 was arbitrary and the new choice of 10 doesn't preclude alternate implementations.)

-Kevin