DangerKlippers / danger-klipper

Klipper but... Dangerous
GNU General Public License v3.0
193 stars 71 forks source link

Do not assume default values for safety critical config fields #325

Closed calithameridi closed 1 week ago

calithameridi commented 2 months ago

In certain configuration parameters, Klipper assumes default values if the field is omitted by the user or not explicitly specified. This has led to some ... concerning user error scenarios in practice. Take the following example:

  1. User is setting up hardware (e.g. Corevus, Mellow HV 5160 Pro, BTT 5160 Plus, Kraken) that have Trinamic TMC2160/5160 drivers with non-0.075Ω current sense resistors, but omits the 'sense_resistor' in the configuration file.
  2. Klipper assumes 0.075 Ω (as described in an ever so small note in the documentation that the user did not read), and configures the TMC driver accordingly.
  3. Machine powers up and sends far in excess of the rated motor current through the stepper motors.
  4. User has an electrical fire and is out $100 in hardware replacement costs

Affected fields include 'sense_resistor' in [tmc] and 'pullup_resistor' in [temperature_sensor]. There may be other examples of this behavior that I haven't thought of at this moment.

This change may cause headaches for end users who must fill in the formerly optional values but safety comes before inconvenience.

rogerlz commented 2 months ago

@lraithel15133 is working on it

rogerlz commented 2 months ago

Thanks for the suggestion.

There is ongoing work in the background to add a list of common stepsticks and their sense resistors, so users can choose the exact hardware they have.

I've added a warning for now, as I do not want to break existing configurations.

https://github.com/DangerKlippers/danger-klipper/pull/332

rogerlz commented 1 week ago

sense_resistor now has a warning, and soon, we will have step stick lookup tables, so I am closing this. I don't feel pullup_resistor is as dangerous as sense_resistor, so we won't change that now and might revisit this in the future. Thanks