Skip to content

Commit

Permalink
Revert "Coolstep for TMC2130, 2209, 5130, 5160"
Browse files Browse the repository at this point in the history 
Reverting #16790 as not ready for primetime.
  • Loading branch information
@thinkyhead
thinkyhead committed Feb 10, 2020
1 parent 28b48fc commit 18a7276
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 352 deletions.
160 changes: 0 additions & 160 deletions Marlin/Configuration_adv.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2275,166 +2275,6 @@
#define E6_HYBRID_THRESHOLD 30
#define E7_HYBRID_THRESHOLD 30

/**
* CoolStep. Currently supported for TMC2130, TMC2209, TMC5130 and TMC5160 only.
* This mode allows for cooler steppers and energy savings.
* The driver will switch to coolStep when stepper speed is over COOLSTEP_THRESHOLD mm/s.
*
* If SG_RESULT goes below COOLSTEP_LOWER_LOAD_THRESHOLD * 32 stepper current will be increased.
* Set to 0 to disable CoolStep.
*
* If SG_RESULT goes above (COOLSTEP_LOWER_LOAD_THRESHOLD + COOLSTEP_UPPER_LOAD_THRESHOLD + 1) * 32
* stepper current will be decreased.
*
* SEUP sets the increase step width. Value range is 0..3 and computed as 2^SEUP.
* SEDN sets the decrease delay. Value range is 0..3, 0 being the slowest.
* SEIMIN sets the lower current limit. 0: 1/2 of IRUN, 1:1/4 of IRUN
*/

#if AXIS_HAS_COOLSTEP(X)
#define X_COOLSTEP_SPEED_THRESHOLD 5
#define X_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define X_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define X_COOLSTEP_SEUP 2
#define X_COOLSTEP_SEDN 0
#define X_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(X2)
#define X2_COOLSTEP_SPEED_THRESHOLD 5
#define X2_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define X2_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define X2_COOLSTEP_SEUP 2
#define X2_COOLSTEP_SEDN 0
#define X2_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(Y)
#define Y_COOLSTEP_SPEED_THRESHOLD 5
#define Y_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define Y_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define Y_COOLSTEP_SEUP 2
#define Y_COOLSTEP_SEDN 0
#define Y_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(Y2)
#define Y2_COOLSTEP_SPEED_THRESHOLD 5
#define Y2_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define Y2_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define Y2_COOLSTEP_SEUP 2
#define Y2_COOLSTEP_SEDN 0
#define Y2_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(Z)
#define Z_COOLSTEP_SPEED_THRESHOLD 5
#define Z_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define Z_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define Z_COOLSTEP_SEUP 2
#define Z_COOLSTEP_SEDN 0
#define Z_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(Z2)
#define Z2_COOLSTEP_SPEED_THRESHOLD 5
#define Z2_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define Z2_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define Z2_COOLSTEP_SEUP 2
#define Z2_COOLSTEP_SEDN 0
#define Z2_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(Z3)
#define Z3_COOLSTEP_SPEED_THRESHOLD 5
#define Z3_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define Z3_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define Z3_COOLSTEP_SEUP 2
#define Z3_COOLSTEP_SEDN 0
#define Z3_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(Z4)
#define Z4_COOLSTEP_SPEED_THRESHOLD 5
#define Z4_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define Z4_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define Z4_COOLSTEP_SEUP 2
#define Z4_COOLSTEP_SEDN 0
#define Z4_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(E0)
#define E0_COOLSTEP_SPEED_THRESHOLD 5
#define E0_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define E0_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define E0_COOLSTEP_SEUP 2
#define E0_COOLSTEP_SEDN 0
#define E0_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(E1)
#define E1_COOLSTEP_SPEED_THRESHOLD 5
#define E1_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define E1_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define E1_COOLSTEP_SEUP 2
#define E1_COOLSTEP_SEDN 0
#define E1_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(E2)
#define E2_COOLSTEP_SPEED_THRESHOLD 5
#define E2_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define E2_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define E2_COOLSTEP_SEUP 2
#define E2_COOLSTEP_SEDN 0
#define E2_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(E3)
#define E3_COOLSTEP_SPEED_THRESHOLD 5
#define E3_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define E3_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define E3_COOLSTEP_SEUP 2
#define E3_COOLSTEP_SEDN 0
#define E3_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(E4)
#define E4_COOLSTEP_SPEED_THRESHOLD 5
#define E4_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define E4_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define E4_COOLSTEP_SEUP 2
#define E4_COOLSTEP_SEDN 0
#define E4_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(E5)
#define E5_COOLSTEP_SPEED_THRESHOLD 5
#define E5_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define E5_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define E5_COOLSTEP_SEUP 2
#define E5_COOLSTEP_SEDN 0
#define E5_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(E6)
#define E6_COOLSTEP_SPEED_THRESHOLD 5
#define E6_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define E6_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define E6_COOLSTEP_SEUP 2
#define E6_COOLSTEP_SEDN 0
#define E6_COOLSTEP_SEIMIN 1
#endif

#if AXIS_HAS_COOLSTEP(E7)
#define E7_COOLSTEP_SPEED_THRESHOLD 5
#define E7_COOLSTEP_LOWER_LOAD_THRESHOLD 7
#define E7_COOLSTEP_UPPER_LOAD_THRESHOLD 0
#define E7_COOLSTEP_SEUP 2
#define E7_COOLSTEP_SEDN 0
#define E7_COOLSTEP_SEIMIN 1
#endif

/**
* Use StallGuard2 to home / probe X, Y, Z.
*
Expand Down
16 changes: 1 addition & 15 deletions Marlin/src/feature/tmc_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,13 +137,6 @@ class TMCMarlin : public TMC, public TMCStorage<AXIS_LETTER, DRIVER_ID> {
#endif
#endif

uint32_t get_cool_thrs() {
return _tmc_thrs(this->microsteps(), this->TCOOLTHRS(), planner.settings.axis_steps_per_mm[AXIS_ID]);
}
void set_cool_thrs(const uint32_t thrs) {
TMC::TCOOLTHRS(_tmc_thrs(this->microsteps(), thrs, planner.settings.axis_steps_per_mm[AXIS_ID]));
}

#if HAS_LCD_MENU
inline void refresh_stepper_current() { rms_current(this->val_mA); }

Expand Down Expand Up @@ -240,14 +233,6 @@ class TMCMarlin<TMC2209Stepper, AXIS_LETTER, DRIVER_ID, AXIS_ID> : public TMC220
#endif
}
#endif

uint32_t get_cool_thrs() {
return _tmc_thrs(this->microsteps(), this->TCOOLTHRS(), planner.settings.axis_steps_per_mm[AXIS_ID]);
}
void set_cool_thrs(const uint32_t thrs) {
TMC2209Stepper::TCOOLTHRS(_tmc_thrs(this->microsteps(), thrs, planner.settings.axis_steps_per_mm[AXIS_ID]));
}

#if USE_SENSORLESS
inline int16_t homing_threshold() { return TMC2209Stepper::SGTHRS(); }
void homing_threshold(int16_t sgt_val) {
Expand All @@ -261,6 +246,7 @@ class TMCMarlin<TMC2209Stepper, AXIS_LETTER, DRIVER_ID, AXIS_ID> : public TMC220

#if HAS_LCD_MENU
inline void refresh_stepper_current() { rms_current(this->val_mA); }

#if ENABLED(HYBRID_THRESHOLD)
inline void refresh_hybrid_thrs() { set_pwm_thrs(this->stored.hybrid_thrs); }
#endif
Expand Down
56 changes: 0 additions & 56 deletions Marlin/src/inc/SanityCheck.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2040,62 +2040,6 @@ static_assert(Y_MAX_LENGTH >= Y_BED_SIZE, "Movement bounds (Y_MIN_POS, Y_MAX_POS
#error "LED_USER_PRESET_STARTUP is required for FYSETC_MINI_12864 2.x displays."
#endif

/**
* Make sure CoolStep settings exist
*/
#if HAS_COOLSTEP
#define NEEDS_COOLSTEP(A) AXIS_HAS_COOLSTEP(A) && !(defined(A##_COOLSTEP_SPEED_THRESHOLD) && defined(A##_COOLSTEP_LOWER_LOAD_THRESHOLD) && defined(A##_COOLSTEP_UPPER_LOAD_THRESHOLD) && defined(A##_COOLSTEP_SEUP) && defined(A##_COOLSTEP_SEDN) && defined(A##_COOLSTEP_SEIMIN))
#if NEEDS_COOLSTEP(X)
#error "X COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(X2)
#error "X2 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(Y)
#error "Y COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(Y2)
#error "Y2 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(Z)
#error "Z COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(Z2)
#error "Z2 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(Z3)
#error "Z3 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(Z4)
#error "Z4 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(E0)
#error "E0 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(E1)
#error "E1 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(E2)
#error "E2 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(E3)
#error "E3 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(E4)
#error "E4 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(E5)
#error "E5 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(E6)
#error "E6 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#if NEEDS_COOLSTEP(E7)
#error "E7 COOLSTEP settings must be defined in Configuration_adv.h."
#endif
#undef NEEDS_COOLSTEP
#endif

/**
* Check existing CS pins against enabled TMC SPI drivers.
*/
Expand Down
Loading

4 comments on commit 18a7276

@Fabi0San
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @thinkyhead, what was the issue here? Something I can help with?

@Fabi0San
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default CoolStep config values we merged left it on and with thresholds that may have been too low, causing drivers to run with 1/4 of the current which is 1/16 of the torque and can easily cause some stalls.
There is also some noise coming from the config change for double stepper axis which seems to have caused axis sync issues as reported here: #16823 and here: #16819.
We can talk about other concerns you may have, but I believe we could bring it back disabled by default with some more comments on how to enable and tune it, as well as documentation on the Marlin website.
Let me know if that is ok with you and I'll start the PRs.

@thinkyhead
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this time @teemuatlut informs us that these settings are not ready for user exposure.

@Fabi0San
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, @teemuatlut, do you have any specific concerns I can try to address?
I believe the driver type auto-assignment which went in the same change had some kinks to iron out and that got confused with being caused by CoolStep.
I understand the desire to make the feature user friendly, but CoolStep like StealthGuard needs individual tuning for each machine.

Please sign in to comment.