Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coolstep for TMC2130, 2209, 5130, 5160 #16790

Merged
merged 14 commits into from
Feb 9, 2020

Conversation

Fabi0San
Copy link
Contributor

@Fabi0San Fabi0San commented Feb 6, 2020

Requirements

  • Filling out this template is required. Pull Requests without a clear description may be closed at the maintainers' discretion.

Description

This change implements configuration and initialization for the CoolStep feature of TMC2209 drivers.
I followed the config style of other per driver Trinamic features.
The configs are passed in to the init methods via parameters as are other configs but I wonder whether that is preferable to directly using the defines with a switch(axis) given that these are not intended to be adjusted with M commands.

Benefits

This will allow 2209 (and possibly 2130) drivers to use the CoolStep feature which dramatically decrease heat and power use.
Though 2130s only have CoolStep and StallGuard for the SpreadCycle chopper, the 2209 brings those to StealthChop which makes them much more useful for 3D printers and relevant for Marlin.
That said we can implement CoolStep on 2130(and others?) if we feel that is at all useful.

Related Issues

I haven't created one, please let me know if that is a better starting point before creating the PR.

@Fabi0San
Copy link
Contributor Author

Fabi0San commented Feb 6, 2020

Not sure if this is the right place to ask this but is there an automated way to get the new config values to be added to all example config values?

Copy link
Contributor

@sjasonsmith sjasonsmith left a comment

Choose a reason for hiding this comment

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

I posted a few thoughts, but didn't provide any feedback on the actual coolstep implementation. I'm not familiar with that feature or how to use it properly.

Marlin/Configuration_adv.h Outdated Show resolved Hide resolved
Marlin/src/feature/tmc_util.cpp Outdated Show resolved Hide resolved
Marlin/src/module/stepper/trinamic.cpp Outdated Show resolved Hide resolved
@thinkyhead
Copy link
Member

is there an automated way to get the new config values to be added to all example config values?

No. It requires Human intervention to apply changes to the configs in an attentive way, but some of the process is scripted so it's pretty quick.

@thinkyhead
Copy link
Member

thinkyhead commented Feb 7, 2020

Ok, so I was able to add support for coolStep in the TMC2130. And I did have to alter the initializers because without the change the CoolStep defines were required to exist if any TMC driver was present. The TMCStepper library's fallback TMC class definition includes CoolStep methods, so TMCStepper requires no changes.

@Fabi0San
Copy link
Contributor Author

Fabi0San commented Feb 7, 2020

Ok, so I was able to add support for coolStep in the TMC2130.

Thanks driving this tentative PR forward Scott!

@Fabi0San
Copy link
Contributor Author

Fabi0San commented Feb 7, 2020

Marlin/src/module/stepper/trinamic.cpp:803:14: error: 'X_COOLSTEP_SPEED_THRESHOLD' was not declared in this scope

Is there a way to macro a default value when things are not defined?
Something like COALESCE(X_COOLSTEP_SPEED_THRESHOLD, 0)

@thinkyhead
Copy link
Member

Is there a way to macro a default value when things are not defined?

The error you received will be replaced by a sanity check telling the user to add the necessary defines.

@Fabi0San
Copy link
Contributor Author

Fabi0San commented Feb 7, 2020

The error you received will be replaced by a sanity check telling the user to add the necessary defines.

Some tests do use the 2209 and are not defining these, should I change those configs and sanity check?

@thinkyhead
Copy link
Member

The Configurations repo is updated with the new settings, so any failures now will be due to something other than missing settings.

@Fabi0San
Copy link
Contributor Author

Fabi0San commented Feb 7, 2020

Configurations repo?
[Test megaatmega2560] RAMPS | SCARA | Mixed TMC | EEPROM... is still unhappy.

@thisiskeithb
Copy link
Member

Not that they need it thanks to external mosfets, but can this be extended to TMC5160s as well? CoolStep is built-in.

@thinkyhead
Copy link
Member

Configurations repo?

https://github.com/MarlinFirmware/Configurations

[Test megaatmega2560] RAMPS | SCARA | Mixed TMC | EEPROM... is still unhappy.

GitHub told me while I was taking a break to stretch my legs.

@thinkyhead
Copy link
Member

Not that they need it thanks to external mosfets, but can this be extended to TMC5160s as well? CoolStep is built-in.

It looks like TMCStepper might need an update for that. I'll try compiling it in to see if it barks.

@thinkyhead thinkyhead changed the title Coolstep for TMC2209 Coolstep for TMC2130, 2209, 5130, 5160 Feb 7, 2020
@teemuatlut
Copy link
Member

I'll try to go over this during the weekend.

@SheelGreg
Copy link

SheelGreg commented Feb 8, 2020

Hello !
Sorry i'm a noob, i hope i'm in the right place for that.
This feature looks great ! But, i've a problem in Visual Code Studio (platform.io) on a BTT SKR 1.2 mini E3 while compiling :

d:\marlin\marlin-bugfix-2.0.x\marlin\configuration_adv.h:2297:24: error: missing binary operator before token "("
   #if AXIS_HAS_COOLSTEP(X)
                        ^

d:\marlin\marlin-bugfix-2.0.x\marlin\configuration_adv.h:2306:24: error: missing binary operator before token "("
   #if AXIS_HAS_COOLSTEP(X2)
                        ^

d:\marlin\marlin-bugfix-2.0.x\marlin\configuration_adv.h:2315:24: error: missing binary operator before token "("
   #if AXIS_HAS_COOLSTEP(Y)
                        ^

d:\marlin\marlin-bugfix-2.0.x\marlin\configuration_adv.h:2324:24: error: missing binary operator before token "("
   #if AXIS_HAS_COOLSTEP(Y2)`

Etc etc for all the axis and extruders.
I just downloaded the latest bugfix 2.0.X, few minutes ago.

Thanks a lot

@Fabi0San Fabi0San removed the request for review from teemuatlut February 8, 2020 17:41
Marlin/Configuration_adv.h Outdated Show resolved Hide resolved
@MarlinFirmware MarlinFirmware deleted a comment from Calimerorulez Feb 9, 2020
@thinkyhead thinkyhead merged commit 8947622 into MarlinFirmware:bugfix-2.0.x Feb 9, 2020
@daleckystepan
Copy link
Contributor

daleckystepan commented Feb 9, 2020

Hi guys, CoolStep is enabled by default. I'm using TMC 2130. Is it better than stealtchop for precision or print quality? Or only power consumption and cooling?

I have disabled CoolStep by
#define X_COOLSTEP_LOWER_LOAD_THRESHOLD 0
but the motor sound is definitely different than before this patch. Probably SpreadCycle or CoolStep is still active.

https://github.com/daleckystepan/Marlin/blob/bugfix-2.0.x/Marlin/Configuration.h
https://github.com/daleckystepan/Marlin/blob/bugfix-2.0.x/Marlin/Configuration_adv.h

@thinkyhead
Copy link
Member

This will be going away. It's not a system that users should be messing with at this time.

thinkyhead added a commit that referenced this pull request Feb 10, 2020
Reverting #16790 as not ready for primetime.
@Thinkersbluff
Copy link

This will be going away. It's not a system that users should be messing with at this time.

Rats. I got excited there, for a minute.
I had to replace my first BTT SKR e3 mini, 3 weeks after installing it, because the X-Axis drive failed. (My X, Y and Z motors were all hot enough to fry eggs. Not quite sure why, yet, but I suspect the TMC2209 burned-out.)
I am now having similar problems with the replacement board (so far just the X Motor has overheated but no "deaths"), and I am trying to quickly determine what to adjust to prevent killing the new board. Based on the TMC 2209 datasheet activating the Coolstep feature looked like a promising option, but I cannot find it by name it in the Marlin config files.

I have uncommented this line in Configuration_adv.h, in the hopes that it will detect any motor overheat and automatically drop the current another 50mA: #define MONITOR_DRIVER_STATUS.
I was not able to compile with TMC_DEBUG activated, however (did not fit rom), so I am not sure whether the MONITOR will work.

Any comment/advice?

@thinkyhead
Copy link
Member

Unfortunately we project maintainers are far from experts in the code we maintain, and though we try to have some general concept of what is going on we are not ourselves very familiar will all kinds of hardware and how to deal with their quirks. So I must throw you a canned reply:

This Issue Queue is for Marlin bug reports and development-related issues, and we prefer not to handle user-support questions here. (As noted on this page.) For best results getting help with configuration and troubleshooting, please use the following resources:

After seeking help from the community, if the consensus points to to a bug in Marlin, then you should post a bug report.

@Thinkersbluff
Copy link

Thinkersbluff commented Feb 16, 2020 via email

@Fabi0San Fabi0San deleted the TMCCoolStep branch February 29, 2020 06:48
StJa added a commit to StJa/Marlin that referenced this pull request Mar 9, 2020
* Add sanity-check for new Advanced Pause option

Followup to MarlinFirmware#16372

* Include macros for delta ABC

* Update Russian language (MarlinFirmware#16745)

* Fix BTT SKR 1.4 extra endstop pins (MarlinFirmware#16738)

* Option for Trigorilla 1.4 with add-on endstops board (MarlinFirmware#16737)

* Consistent M112 with Emergency Parser (MarlinFirmware#16747)

* Improve mfadd helper script

- Use the original branch name if none is supplied
- Set the remote tracking to the source
- Accept User/Branch or User:Branch syntax

* Clean up i2c encoder, sanitize serial

* Misc cleanup, whitespace

* Encapsulate probe as singleton class (MarlinFirmware#16751)

* G34 automatic point assignment (MarlinFirmware#16473)

* Fix Temperature::over_autostart_threshold (MarlinFirmware#16749)

* Update Russian language (MarlinFirmware#16750)

* Fix CURRENT_STEP_DOWN compile error

* Drop obsolete SD special char handling

See MarlinFirmware#14035

* Probe singleton patch

Followup to MarlinFirmware#16751

* Fix RGB / Neopixel white color bug

See MarlinFirmware#16752

* Suppress a compile warning

* More 8-extruder fixups

* Add EXP labels to SKR pins

* Fix LPC build with USE_WATCHDOG off

* Minor string storage optimization

* Apply REPEAT, RREPEAT, and loop macros (MarlinFirmware#16757)

* Revert breaking change to _FAN_PWM macro

* Add Z_AFTER_HOMING to raise Z more in G28 (MarlinFirmware#16755)

* Corner Leveling: Add inset for each side (MarlinFirmware#16759)

* (c) 2020

* Tweak mfqp script

* Use a different Configurations branch for CI

* Fix LCD Z Move character LCD display line (MarlinFirmware#16772)

* Fix warning for ESP32 (MarlinFirmware#16771)

* [cron] Bump distribution date

* Force T0 in UBL G29 on all multi-hotend setups (MarlinFirmware#16774)

* Keep secure credentials in a separate config file (MarlinFirmware#16773)

* STM32duino - Use SDIO for onboard SD (MarlinFirmware#16756)

* Fix E stepper stays on bug

Fixes MarlinFirmware#16753

* Fix Arduino IDE compile for DUE

Fixes MarlinFirmware#16767

* Fix CALIBRATION_GCODE pin handling

* Upgrade an ifdef

* More updates for 8 extruders, REPEAT

* [cron] Bump distribution date (2020-02-05)

* Add MKS Base 1.6 board (MarlinFirmware#16783)

* Direct download link for configs

* [cron] Bump distribution date (2020-02-06)

* Split up MKS_RUMBA32 into two variants (MarlinFirmware#16781)

* G26: Allow to set retraction for UBL mesh test (MarlinFirmware#16511)

* Remove extraneous Serial init (MarlinFirmware#16794)

* Fix probe with multi-endstops (MarlinFirmware#16793)

* [cron] Bump distribution date (2020-02-07)

* [cron] Bump distribution date (2020-02-08)

* Clean up Makefle indentation

* Add .editorconfig file

* Tweak ABL logging, document probing

* [cron] Bump distribution date (2020-02-09)

* Coolstep for TMC2130, 2209, 5130, 5160 (MarlinFirmware#16790)

* Better probe fail handling (MarlinFirmware#16811)

* Adafruit Grand Central M4 fixes (MarlinFirmware#16812)

* Minor HAL cleanup

* Move MSG_MARLIN

* Show print time with PRINTER_EVENT_LEDS

* Tweak parser warning

* Bump config version to 020004 (MarlinFirmware#16816)

* Add PID, probe offsets to ExtUI (MarlinFirmware#16792)

* [cron] Bump distribution date (2020-02-10)

* Tweak LPC1768 upload py script

* Add mftest -b (auto-build) and -u (upload)

- Implement the equivalent of auto-build for the shell environment by using the MOTHERBOARD setting to look up the env: entries.

* Revert change to AXIS_DRIVER_TYPE_X2

- Revisit this to figure out why it breaks

* Revert "Coolstep for TMC2130, 2209, 5130, 5160"

Reverting MarlinFirmware#16790 as not ready for primetime.

* Add a caution to drivers.h

* Update MKS BASE and v1.6 pins (MarlinFirmware#16806)

* Add g-code quoted strings, improve stream code (MarlinFirmware#16818)

* Fix out-of-order M0 after SD printing

Fixes MarlinFirmware#14774

Co-Authored-By: tol2cj <tol2cj@users.noreply.github.com>

* Fix out-of-order M0 after SD printing

Fixes MarlinFirmware#14774

Co-Authored-By: tol2cj <tol2cj@users.noreply.github.com>

* Enable hotend / bed PID separately in ExtUI (MarlinFirmware#16827)

* Fix MKS Robin Nano platformio.ini entry (MarlinFirmware#16826)

* Unify step pulse timing of ISR / babystep (MarlinFirmware#16813)

* [cron] Bump distribution date (2020-02-11)

* Update SAMD51 EEPROM repo link (MarlinFirmware#16832)

* Undo driver type auto-assignment for now

Good general concept but needs more time to develop and group with a stepper suite.

* No Z sensorless req'd if homing with probe

Fixes MarlinFirmware#16674

* Recommend Z Safe Homing

Co-Authored-By: Vertabreaker <opyrus@hotmail.com>

* Use prior babystep delay method (MarlinFirmware#16833)

* Function-style critical section macros

* Fix up tests

* Simplify old safe homing sanity check

* Prevent pin glitches on out commutation (MarlinFirmware#16835)

Better for switching from pulled input to output and also set real output (with no input enabled).

* [cron] Bump distribution date (2020-02-12)

* Define MarlinSerial instances for DGUS (MarlinFirmware#16841)

* No limit needed on this raise

Remove an extraneous limit from MarlinFirmware#16811.

* [cron] Bump distribution date (2020-02-13)

* [cron] Bump distribution date (2020-02-14)

* Fix G-code line parsing (MarlinFirmware#16840)

* Ping the job timer in M140 (MarlinFirmware#16849)

* Remove unused queue.stopped_N (MarlinFirmware#16850)

* Don't assert safe homing for delta/scara

* Fix ESP32 warning, specify supported version

* Add ESPAsyncTCP to lib_ignore (MarlinFirmware#16844)

* Add RAMPS 1.4.4 to AGCM4 (MarlinFirmware#16606)

* Clean up host actions code (MarlinFirmware#16856)

* Optimize "Dismiss" string

* Clean up stepper and babystep (MarlinFirmware#16857)

* Fysetc S6 pins / LCD updates (MarlinFirmware#16830)

* [cron] Bump distribution date (2020-02-15)

* Fix mftest -b and -u. Add --help.

* Fix a BORG compile warning

* Fix byte-to-percent display

Fixes MarlinFirmware#16866

* Conceal float rounding errors on display

Fix MarlinFirmware#16866

* [cron] Bump distribution date (2020-02-16)

* Double ADC read frequency (MarlinFirmware#16864)

* EXPERIMENTAL integrated BABYSTEPPING (MarlinFirmware#16829)

* Show '*' for zero 'stst' flag

* Require TMCStepper 0.6.2

* Defer updated ADC

* Move SAMD51 Temperature timer to RTC (MarlinFirmware#16868)

* Fix unknown command on empty lines (MarlinFirmware#16867)

* Fix mftest -b -u line match

* Update French language (MarlinFirmware#16877)

* Put ESP32 I2S stepper task and Marlin on the same core (MarlinFirmware#16874)

* Fix babystep include, typos in stepper.cpp

Fix MarlinFirmware#16881

* [cron] Bump distribution date (2020-02-17)

* [cron] Bump distribution date (2020-02-18)

* [cron] Bump distribution date (2020-02-19)

* [cron] Bump distribution date (2020-02-20)

* Serial redirect for Move Command when stopping (MarlinFirmware#16906)

* [cron] Bump distribution date (2020-02-21)

* Single envs for specific boards

* Inline manage_inactivity, tweak autoreport_paused

* Function for CONFIG_ECHO_HEADING

* Show end prompt with Print Event LEDs

* Add a note on EEPROM todo

* Tweak process_line_done for speed

* More EEPROM cleanup

* Followup to autoreport patch (MarlinFirmware#16914)

See a1f026f

* Disable spreadcycle in tmc_enable_stallguard<2209> (MarlinFirmware#16890)

* Fix EEPROM errors with EXTRUDERS == 0 (MarlinFirmware#16898)

* Add PICA shields support (MarlinFirmware#16891)

* Tweak pins spacing, comments

* Version 2.0.4 Release

* [cron] Bump distribution date (2020-02-22)

* Use moves_free in ok_to_send

* Hotfix for Babystepping

* CoreXY Babystepping hotfix

* Use moves_free in ok_to_send

* [cron] Bump distribution date (2020-02-22)

* Hotfix for Babystepping

* [ESP32] Allow user to define pins for hardware Serial1 and Serial2 (MarlinFirmware#16918)

* CoreXY Babystepping hotfix

* Version 2.0.4.1 Release

* [cron] Bump distribution date (2020-02-23)

* Finish Custom User Menu sanity-check (MarlinFirmware#16917)

* Followup to babystep hotfix

* Fix M0/M1 broken wait loop (MarlinFirmware#16921)

* Define ANET_FULL_GRAPHICS_LCD pins for SKR 1.4 (MarlinFirmware#16928)

* Version 2.0.4.2 Release

* Suppress "packed member" warning

* Suppress "packed member" warning

* Commit last SD line before fileHasFinished

* Allow LCD_PIXEL_WIDTH/HEIGHT override

* Allow USE_GCODE_SUBCODES for debugging

* Sync Italian language (MarlinFirmware#16935)

* Reduce default TMC baudrate to 57600 with Software Serial (MarlinFirmware#16930)

* [cron] Bump distribution date (2020-02-24)

* Fix AXIS_HAS_SW_SERIAL

* Simplified E_AXIS_HAS macro

* "Init. Media" => "Attach Media"

* Fix Babystepping loop (again)

* BS_TOTAL_AXIS => BS_TOTAL_IND

* Allow Z_SAFE_HOMING_POINT outside bed (MarlinFirmware#16945)

* Restore tabs in Makefile (MarlinFirmware#16944)

* Fix card_eof error

* Version 2.0.4.3 Release

* Allow Z_SAFE_HOMING_POINT outside bed (MarlinFirmware#16945)

* Restore tabs in Makefile (MarlinFirmware#16944)

* Fix card_eof error

* [cron] Bump distribution date (2020-02-25)

* Update POWER_LOSS_PIN comment (MarlinFirmware#16957)

* Update Italian language (MarlinFirmware#16947)

* Fix LCD cutter/bed icons overlapping (MarlinFirmware#16956)

* Fix SKR 1.4 Turbo SD_DETECT_PIN (MarlinFirmware#16955)

* Fix the wait loop in M0 / M1

* [cron] Bump distribution date (2020-02-26)

* Ensure proper SD print completion (MarlinFirmware#16967)

* HAS_SDCARD_CONNECTION is more obsolete

* Fix GTR10 overlapping defines (MarlinFirmware#16976)

* Toolchange improvements (MarlinFirmware#16979)

* Language: "failsafe" => "Defaults"

* Use a STR_ prefix for non-translated strings

* Set LCD status for EEPROM errors (MarlinFirmware#16977)

* More serial strings

* Add LPC1768 Serial ports for pinsDebug (MarlinFirmware#16980)

* Correct SKR expansion port pins (MarlinFirmware#16974)

* String optimize followup

* Fix Trinamic pulse rate auto-assignment (MarlinFirmware#16966)

* Allow weird probe values in G33

* Serial strings in macros

* Sanity check for LPC serial pin conflict (MarlinFirmware#16981)

* Allow servo features in combination (MarlinFirmware#16960)

* Add TRAVEL_EXTRA_XYJERK option

See MarlinFirmware#16949

Co-Authored-By: josedpedroso <josedpedroso@users.noreply.github.com>

* Quick-homing sensorless back-off (MarlinFirmware#16872)

* Prevent park_point compile error

* More extra travel jerk changes

Co-Authored-By: josedpedroso <josedpedroso@users.noreply.github.com>

* Fix unified status bed temp display

* Allow print recovery after parking

* Case-insensitive g-code option (MarlinFirmware#16932)

* Define DIAG pins for MKS SGen-L

* Handle print completed LED event in M0

* [cron] Bump distribution date (2020-02-27)

* Fix planner.cpp compile (MarlinFirmware#16996)

* Update Slovak language (MarlinFirmware#17002)

* Version 2.0.4.4 Release

Co-authored-by: Scott Lahteine <github@thinkyhead.com>
Co-authored-by: Acenotass <44540957+Acenotass@users.noreply.github.com>
Co-authored-by: rebel1 <453277+rebel1@users.noreply.github.com>
Co-authored-by: Jason Smith <jason.inet@gmail.com>
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Co-authored-by: InsanityAutomation <38436470+InsanityAutomation@users.noreply.github.com>
Co-authored-by: ellensp <ellensp@hotmail.com>
Co-authored-by: felixstorm <felix.storm@glueckkanja.com>
Co-authored-by: Bob Kuhn <bob.kuhn@att.net>
Co-authored-by: thisiskeithb <13375512+thisiskeithb@users.noreply.github.com>
Co-authored-by: Tanguy Pruvot <tpruvot@users.noreply.github.com>
Co-authored-by: Robby Candra <robbycandra.mail@gmail.com>
Co-authored-by: Robert Stein <robert.stein@requisis.com>
Co-authored-by: Fabio Santos <fabiosan@live.com>
Co-authored-by: Giuliano Zaro <3684609+GMagician@users.noreply.github.com>
Co-authored-by: Daniel Mazurkiewicz <daniel@latanie.info>
Co-authored-by: tol2cj <tol2cj@users.noreply.github.com>
Co-authored-by: proferabg <proferabg@users.noreply.github.com>
Co-authored-by: darksiah <siatam@gmail.com>
Co-authored-by: Vertabreaker <opyrus@hotmail.com>
Co-authored-by: Gaston Dombiak <gdombiak@gmail.com>
Co-authored-by: vivian-ng <vivian@maplerain.com>
Co-authored-by: George Fu <nailao_5918@163.com>
Co-authored-by: Karl Andersson <karl@iaccess.se>
Co-authored-by: Jamie <vector76@users.noreply.github.com>
Co-authored-by: ZMiguel Alves <zmiguel@users.noreply.github.com>
Co-authored-by: Marcio T <mlt4356-github@yahoo.com>
Co-authored-by: josedpedroso <josedpedroso@users.noreply.github.com>
Co-authored-by: Makoto Schoppert <makotosan@gmail.com>
Co-authored-by: Roman Moravčík <roman.moravcik@gmail.com>
@cryptoAlgorithm
Copy link

Would love to see this merged again soon, messing with TMC_ADV isn't my cup of tea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants