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

[1.1.x] TMC driver update #8712

Merged
merged 9 commits into from Dec 15, 2017

Conversation

Projects
None yet
7 participants
@teemuatlut
Member

teemuatlut commented Dec 8, 2017

New gcode commands:

  • M122: TMC debugging command. Pulls several registers from all the configured drivers and parses the bits and values.
  • M915: Calibrate Z axis. Basically a script that lowers Z driver current and drives the axis past the configured maximum height. Then home Z again because we intentionally lost some steps. This is quite a new feature still and I haven't used it much (because it's a calibration command). I would've also preferred to give it a G command but didn't know which one to go for. I don't know if Prusa printers have a dedicated gcode command or if it's LCD only.

Features:

  • Support for TMC2208 drivers in HW and SW serial modes.
  • Automatic current control deprecated and replaced with a driver monitoring system. The new system polls the drivers twice a second and lowers the current if overtemperature prewarn is triggered. Can also stop the printer if a driver is in an error state (short to ground or overtemperature). S0/1 argument reports the "actual current scale" of the driver.
  • Store sensorless homing setting into EEPROM.

Default configuration:

  • Tune some blank time and hysterisis settings.
  • Lower default current values. Shouldn't expect everyone to have cooling fans and dual heatsinks for drivers.

This is actually a much bigger update that I'm comfortable with and I feel it might be a bit error prone. But I have been using this fork for quite a while now and it should work ok. Mostly I wanted to stop telling people to use my fork to solve their problems.

@thinkyhead

This comment has been minimized.

Show comment
Hide comment
@thinkyhead

thinkyhead Dec 8, 2017

Member

This is epic, and looks good overall. Can we make a PR for the 2.0 branch to match? I might hold off on merging this till after the 1.1.7 release, since that is long overdue. (The 1.1.8 release will be more on-time, probably around 30 days after 1.1.7.)

Member

thinkyhead commented Dec 8, 2017

This is epic, and looks good overall. Can we make a PR for the 2.0 branch to match? I might hold off on merging this till after the 1.1.7 release, since that is long overdue. (The 1.1.8 release will be more on-time, probably around 30 days after 1.1.7.)

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Dec 8, 2017

Member

I've got a 2.0 based branch ready but I'll go over it tomorrow.
That said, I haven't yet used the v2.0 Marlin on my main printer and the 2.0 PR will be mostly theoretical. I will be moving all my work on v2.0 after this however.

Member

teemuatlut commented Dec 8, 2017

I've got a 2.0 based branch ready but I'll go over it tomorrow.
That said, I haven't yet used the v2.0 Marlin on my main printer and the 2.0 PR will be mostly theoretical. I will be moving all my work on v2.0 after this however.

@thinkyhead

This comment has been minimized.

Show comment
Hide comment
@thinkyhead

thinkyhead Dec 9, 2017

Member

@teemuatlut Sounds good to me! Theoretically, the 1.1.7 release could be the last release that gets new features, and we could move all development to 2.0.x starting this week. The 1.1.x branch has only been waiting for a point where 2.0.x could build reliably for AVR and DUE, and though the jury is still out, I think it's acceptable if some components of the 2.0 official release are labeled as still being in beta, or even alpha….

Leaving 1.1.x behind should help to streamline all our development efforts.

Member

thinkyhead commented Dec 9, 2017

@teemuatlut Sounds good to me! Theoretically, the 1.1.7 release could be the last release that gets new features, and we could move all development to 2.0.x starting this week. The 1.1.x branch has only been waiting for a point where 2.0.x could build reliably for AVR and DUE, and though the jury is still out, I think it's acceptable if some components of the 2.0 official release are labeled as still being in beta, or even alpha….

Leaving 1.1.x behind should help to streamline all our development efforts.

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Dec 9, 2017

Member

I'm going to revert your changes regarding the macros. Otherwise we're going to end up with a situation where we have #if ENABLED(HAVE_TMC2130) || ENABLED(HAVE_TMC2208) || ENABLED(HAVE_TMC2224) || ENABLED(HAVE_TMC5130) || ENABLED(HAVE_TMC2660)

Member

teemuatlut commented Dec 9, 2017

I'm going to revert your changes regarding the macros. Otherwise we're going to end up with a situation where we have #if ENABLED(HAVE_TMC2130) || ENABLED(HAVE_TMC2208) || ENABLED(HAVE_TMC2224) || ENABLED(HAVE_TMC5130) || ENABLED(HAVE_TMC2660)

@thinkyhead

This comment has been minimized.

Show comment
Hide comment
@thinkyhead

thinkyhead Dec 11, 2017

Member

Why the increment to the EEPROM version?

Member

thinkyhead commented Dec 11, 2017

Why the increment to the EEPROM version?

@thinkyhead

This comment has been minimized.

Show comment
Hide comment
@thinkyhead

thinkyhead Dec 11, 2017

Member

I'm going to revert your changes regarding the macros. Otherwise…

The main general-purpose macros.h file is a poor place for TMC-specific macros. If we need to refer to these macros in Configuration_adv.h, I suggest we put them in a separate tmc_macros.h file and include that in MarlinConfig.h before Configuration_adv.h instead.

Another option would be to create a separate Configuration_Steppers.h file and move all the alternative stepper options into it, along with the macros. It would be kind of unprecedented to add a new config file, but the number of options related to these steppers is starting to dominate Configuration_adv.h.

Member

thinkyhead commented Dec 11, 2017

I'm going to revert your changes regarding the macros. Otherwise…

The main general-purpose macros.h file is a poor place for TMC-specific macros. If we need to refer to these macros in Configuration_adv.h, I suggest we put them in a separate tmc_macros.h file and include that in MarlinConfig.h before Configuration_adv.h instead.

Another option would be to create a separate Configuration_Steppers.h file and move all the alternative stepper options into it, along with the macros. It would be kind of unprecedented to add a new config file, but the number of options related to these steppers is starting to dominate Configuration_adv.h.

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Dec 11, 2017

Member

the number of options related to these steppers is starting to dominate Configuration_adv.h

😄
Maybe we should actually consider making some logical dividing of the settings in general (perhaps for v2.0). At this time there are quite a lot of settings both in configuration.h and configuration_adv.h and both are over 1500 lines long. It can take a while to find the right setting if you don't know the correct keyword to search for.

I moved the tmc macros to their own file. It looks a bit silly as there's nothing else in there but maybe there will be use for it in the future as well. I know TRAMS is going to add a lot of things.

Why the increment to the EEPROM version?

I've added EEPROM support for the SENSORLESS_HOMING sensitivity values.

Member

teemuatlut commented Dec 11, 2017

the number of options related to these steppers is starting to dominate Configuration_adv.h

😄
Maybe we should actually consider making some logical dividing of the settings in general (perhaps for v2.0). At this time there are quite a lot of settings both in configuration.h and configuration_adv.h and both are over 1500 lines long. It can take a while to find the right setting if you don't know the correct keyword to search for.

I moved the tmc macros to their own file. It looks a bit silly as there's nothing else in there but maybe there will be use for it in the future as well. I know TRAMS is going to add a lot of things.

Why the increment to the EEPROM version?

I've added EEPROM support for the SENSORLESS_HOMING sensitivity values.

@fiveangle

This comment has been minimized.

Show comment
Hide comment
@fiveangle

fiveangle Dec 12, 2017

Contributor

the 1.1.7 release could be the last release that gets new features

❤️ ❤️ ❤️

Another option would be to create a separate Configuration_Steppers.h file and move all the alternative stepper options into it, along with the macros. It would be kind of unprecedented to add a new config file, but the number of options related to these steppers is starting to dominate Configuration_adv.h.

This 💯 %

-=dave

Contributor

fiveangle commented Dec 12, 2017

the 1.1.7 release could be the last release that gets new features

❤️ ❤️ ❤️

Another option would be to create a separate Configuration_Steppers.h file and move all the alternative stepper options into it, along with the macros. It would be kind of unprecedented to add a new config file, but the number of options related to these steppers is starting to dominate Configuration_adv.h.

This 💯 %

-=dave

@thinkyhead

This comment has been minimized.

Show comment
Hide comment
@thinkyhead

thinkyhead Dec 13, 2017

Member

Maybe we should actually consider making some logical dividing of the settings in general (perhaps for v2.0)

At this point I would only break out settings that are rare, so that we don't have to duplicate them in every example configuration. Also there's the issue of having settings at hand. Even breaking out a Configuration_LCD.h file starts to make it harder to find a particular setting that you want to tweak. And, I will never stop repeating what a hassle it is to maintain just 2 configuration files across all the examples, let alone 3 or 4… or 10…

Member

thinkyhead commented Dec 13, 2017

Maybe we should actually consider making some logical dividing of the settings in general (perhaps for v2.0)

At this point I would only break out settings that are rare, so that we don't have to duplicate them in every example configuration. Also there's the issue of having settings at hand. Even breaking out a Configuration_LCD.h file starts to make it harder to find a particular setting that you want to tweak. And, I will never stop repeating what a hassle it is to maintain just 2 configuration files across all the examples, let alone 3 or 4… or 10…

thinkyhead and others added some commits Dec 13, 2017

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Dec 13, 2017

Member

I'd actually argue the other way that because the configuration files are getting so massive I find it harder to find a particular setting.

How do you approach updating all the config files? The way I've done it for these two PRs is using the replace method in sublime text. A regex like // @section TMC2130[\s\S]+#endif // TMC2130 would find the whole section in all the files and I'd use the new updated section as the string that replaces it. If you know the number of _adv files you can get an idea of whether or not it found all of them.

Member

teemuatlut commented Dec 13, 2017

I'd actually argue the other way that because the configuration files are getting so massive I find it harder to find a particular setting.

How do you approach updating all the config files? The way I've done it for these two PRs is using the replace method in sublime text. A regex like // @section TMC2130[\s\S]+#endif // TMC2130 would find the whole section in all the files and I'd use the new updated section as the string that replaces it. If you know the number of _adv files you can get an idea of whether or not it found all of them.

@p3p

This comment has been minimized.

Show comment
Hide comment
@p3p

p3p Dec 13, 2017

Member

The best solution I could think of to the example config issue was to have a json (or any other preferred structure) of the settings required for each example, then regenerate the example config files from the current default config. It actually worked quite well in my python proof of concept, (I generated the diffs from the current examples) .. the obvious extension of this was to always generate the default config from a json data structure too and use that as the master.. which would be preferred method.

Member

p3p commented Dec 13, 2017

The best solution I could think of to the example config issue was to have a json (or any other preferred structure) of the settings required for each example, then regenerate the example config files from the current default config. It actually worked quite well in my python proof of concept, (I generated the diffs from the current examples) .. the obvious extension of this was to always generate the default config from a json data structure too and use that as the master.. which would be preferred method.

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Dec 13, 2017

Member

Maybe the example configuration sets can be handled by the configurator that may be coming in the far far future.

Member

teemuatlut commented Dec 13, 2017

Maybe the example configuration sets can be handled by the configurator that may be coming in the far far future.

@thinkyhead

This comment has been minimized.

Show comment
Hide comment
@thinkyhead

thinkyhead Dec 15, 2017

Member

I'd actually argue the other way that because the configuration files are getting so massive I find it harder to find a particular setting.

I hope most users will use Find in their text editor. Of course, the first time configuring you may scan the first file, reading each option in turn, but once you've done a config or two, you just search for the options you want. For more obscure features, we provide web pages and point out the advanced options there, so users who want to know about them can Google for them. Or, enterprising users can search Configuration_adv.h for their keyword and probably find it.

The debate over whether to split up the configurations further has been had before, and the consensus that developed last time was that adding more files simply adds more headaches, while two files (our minimum) are relatively easy to maintain and keep in sync.

A "configurator" can hide most options and then array the rest in 2D space. A Configurator can also include presets and other helpful shortcuts and cues about which options are relevant or important. A configurator can include audio-visual guidance. The configurator by @akaJes is pretty much what we would like to have as a web-based utility, but unfortunately it was written with total dependency on Windows.

Member

thinkyhead commented Dec 15, 2017

I'd actually argue the other way that because the configuration files are getting so massive I find it harder to find a particular setting.

I hope most users will use Find in their text editor. Of course, the first time configuring you may scan the first file, reading each option in turn, but once you've done a config or two, you just search for the options you want. For more obscure features, we provide web pages and point out the advanced options there, so users who want to know about them can Google for them. Or, enterprising users can search Configuration_adv.h for their keyword and probably find it.

The debate over whether to split up the configurations further has been had before, and the consensus that developed last time was that adding more files simply adds more headaches, while two files (our minimum) are relatively easy to maintain and keep in sync.

A "configurator" can hide most options and then array the rest in 2D space. A Configurator can also include presets and other helpful shortcuts and cues about which options are relevant or important. A configurator can include audio-visual guidance. The configurator by @akaJes is pretty much what we would like to have as a web-based utility, but unfortunately it was written with total dependency on Windows.

@thinkyhead thinkyhead merged commit 0ac0324 into MarlinFirmware:bugfix-1.1.x Dec 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

MoellerDi added a commit to MoellerDi/Marlin that referenced this pull request Dec 15, 2017

Merge branch 'bugfix-1.1.x' of https://github.com/MarlinFirmware/Marlin
… into bugfix-1.1.x-HD-CR10

* 'bugfix-1.1.x' of https://github.com/MarlinFirmware/Marlin:
  [1.1.x] TMC driver update (#8712)
  Correct unskew, after all
  Missing HAS_HEAT_BED conditional
@byebyedizzy

This comment has been minimized.

Show comment
Hide comment
@byebyedizzy

byebyedizzy Dec 16, 2017

I wonder if it's possible to add support for TMC2208 when using single shared UART interface and analog commutator like 77hc4066 (scheme can be taken from 2208 datasheet) ? The main concern is that there is no SoftwareSerial library for some boards, for example Arduino Due. And amount of available hardware serials is rather limited (4 total for Due). This will require, as I understand, some sort of CS pin (for use with commutator) along with shared RX/TX pins.

byebyedizzy commented Dec 16, 2017

I wonder if it's possible to add support for TMC2208 when using single shared UART interface and analog commutator like 77hc4066 (scheme can be taken from 2208 datasheet) ? The main concern is that there is no SoftwareSerial library for some boards, for example Arduino Due. And amount of available hardware serials is rather limited (4 total for Due). This will require, as I understand, some sort of CS pin (for use with commutator) along with shared RX/TX pins.

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Dec 16, 2017

Member

@thinkyhead Was this PR completely left out on the v1.1.7 release notes?

Member

teemuatlut commented Dec 16, 2017

@thinkyhead Was this PR completely left out on the v1.1.7 release notes?

@teemuatlut teemuatlut deleted the teemuatlut:TMC_update_PR branch Dec 16, 2017

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Dec 16, 2017

Member

@byebyedizzy I'd actually rather see the software serial library improved with support for DUE.

Member

teemuatlut commented Dec 16, 2017

@byebyedizzy I'd actually rather see the software serial library improved with support for DUE.

notyal added a commit to notyal/Marlin that referenced this pull request Dec 16, 2017

Merge remote-tracking branch 'origin/1.1.x' into wanhaoi3
* origin/1.1.x: (346 commits)
  Version 1.1.7
  Update README.md for bugfix
  Adding support for the Tronxy and Zonestar LCD
  Apply ZONESTAR_LCD to example configs
  More externs and functions in Marlin.h
  Add NANODLP_ALL_AXIS to config examples
  Extended NanoDLP_Z_Move_Sync feature to G4 and G28, Added optional move_sync for all axis.
  [1.1.x] TMC driver update (#8712)
  Correct unskew, after all
  Missing HAS_HEAT_BED conditional
  Reduce code with no heated bed
  General code/bitmaps cleanup
  Fix Planner::unskew parity with skew
  Comment, improve filament width sensor
  Separate Dual X un-park from movement
  Skew should apply per-segment
  Extend Skew Correction to UBL
  Travis Test for non-segmented UBL
  One or the other?
  UBL devel debugging flag
  ...
@byebyedizzy

This comment has been minimized.

Show comment
Hide comment
@byebyedizzy

byebyedizzy Dec 17, 2017

Well, I could not find any suitable replacement. Actually there are too few software serial implementations for Due. One that I found (https://github.com/antodom/soft_uart/blob/master/README.md) has too much limitations to be used in Marlin code. So shared UART with commutator is rather attractive - simple schematic and (I hope) not too complex to implement ...

byebyedizzy commented Dec 17, 2017

Well, I could not find any suitable replacement. Actually there are too few software serial implementations for Due. One that I found (https://github.com/antodom/soft_uart/blob/master/README.md) has too much limitations to be used in Marlin code. So shared UART with commutator is rather attractive - simple schematic and (I hope) not too complex to implement ...

@thinkyhead

This comment has been minimized.

Show comment
Hide comment
@thinkyhead

thinkyhead Dec 25, 2017

Member

@teemuatlut It appears that some of this PR and #8769 are out of sync, especially configuration_store.cpp. Also the code that was added to configuration_store.cpp is incomplete. It will only compile with HAVE_TMC2130 enabled. See line 2063 in the 1.1.x version of that file for one error. It seems that it should be #if ENABLED(HAVE_TMC2130) || ENABLED(HAVE_TMC2208).

Member

thinkyhead commented Dec 25, 2017

@teemuatlut It appears that some of this PR and #8769 are out of sync, especially configuration_store.cpp. Also the code that was added to configuration_store.cpp is incomplete. It will only compile with HAVE_TMC2130 enabled. See line 2063 in the 1.1.x version of that file for one error. It seems that it should be #if ENABLED(HAVE_TMC2130) || ENABLED(HAVE_TMC2208).

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Dec 25, 2017

Member

Both 1.1.x and 2.0.x should be HAS_TRINAMIC
Other than that there's no really nothing much else in the configuration_store.cpp. The bit position calculations are purely cosmetic and ofc need to be updated but that doesn't affect functionality.

Did you find something else as well?

Member

teemuatlut commented Dec 25, 2017

Both 1.1.x and 2.0.x should be HAS_TRINAMIC
Other than that there's no really nothing much else in the configuration_store.cpp. The bit position calculations are purely cosmetic and ofc need to be updated but that doesn't affect functionality.

Did you find something else as well?

@thinkyhead

This comment has been minimized.

Show comment
Hide comment
@thinkyhead

thinkyhead Dec 25, 2017

Member

Nope. Everything else is perrrrrrfectly ok. e0d487e

Member

thinkyhead commented Dec 25, 2017

Nope. Everything else is perrrrrrfectly ok. e0d487e

@jmdearras

This comment has been minimized.

Show comment
Hide comment
@jmdearras

jmdearras Jan 19, 2018

Contributor

why was automatic current control removed?

Contributor

jmdearras commented Jan 19, 2018

why was automatic current control removed?

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Jan 19, 2018

Member

It didn't work as intended so I reworked it into current state but kept the current step down feature.

Member

teemuatlut commented Jan 19, 2018

It didn't work as intended so I reworked it into current state but kept the current step down feature.

@jmdearras

This comment has been minimized.

Show comment
Hide comment
@jmdearras

jmdearras Jan 19, 2018

Contributor

My son just had got a printer halt due to overtemp with that feature turned on. He's going to debug it this evening.

Contributor

jmdearras commented Jan 19, 2018

My son just had got a printer halt due to overtemp with that feature turned on. He's going to debug it this evening.

@bleem313

This comment has been minimized.

Show comment
Hide comment
@bleem313

bleem313 Jan 20, 2018

Hey, My Y driver is acting weird. It seems to be moving as if it is set for 8 micro steps instead of 16. When I pull the debug info, the Y has a different vsense. Do you have any ideas what might be causing this and how to fix it?

M122
X Y Z E0
Enabled false false false false
Set current 900 1000 800 800
RMS current 887 994 795 795
MAX current 1251 1402 1121 1121
Run current 28/31 17/31 25/31 25/31
Hold current 7/31 4/31 6/31 6/31
CS actual 7/31 4/31 6/31 6/31
PWM scale 42 57 33 1
vsense 1=.18 0=.325 1=.18 1=.18
stealthChop true true true true
msteps 16 16 16 16
tstep 1048575 1048575 1048575 1048575
pwm
threshold 0 0 0 0
[mm/s] - - - -
OT prewarn false false false false
OT prewarn has
been triggered false false false false
off time 5 5 5 5
blank time 24 24 24 24
hysterisis
-end 2 2 2 2
-start 3 3 3 3
Stallguard thrs 0 0 0 0
DRVSTATUS X Y Z E0
stallguard
sg_result 0 0 0 0
fsactive
stst X X X X
olb
ola
s2gb
s2ga
otpw
ot
Driver registers:
X = 0x80:07:00:00
Y = 0x80:04:00:00
Z = 0x80:06:00:00
E0 = 0x80:06:00:00

ok

bleem313 commented Jan 20, 2018

Hey, My Y driver is acting weird. It seems to be moving as if it is set for 8 micro steps instead of 16. When I pull the debug info, the Y has a different vsense. Do you have any ideas what might be causing this and how to fix it?

M122
X Y Z E0
Enabled false false false false
Set current 900 1000 800 800
RMS current 887 994 795 795
MAX current 1251 1402 1121 1121
Run current 28/31 17/31 25/31 25/31
Hold current 7/31 4/31 6/31 6/31
CS actual 7/31 4/31 6/31 6/31
PWM scale 42 57 33 1
vsense 1=.18 0=.325 1=.18 1=.18
stealthChop true true true true
msteps 16 16 16 16
tstep 1048575 1048575 1048575 1048575
pwm
threshold 0 0 0 0
[mm/s] - - - -
OT prewarn false false false false
OT prewarn has
been triggered false false false false
off time 5 5 5 5
blank time 24 24 24 24
hysterisis
-end 2 2 2 2
-start 3 3 3 3
Stallguard thrs 0 0 0 0
DRVSTATUS X Y Z E0
stallguard
sg_result 0 0 0 0
fsactive
stst X X X X
olb
ola
s2gb
s2ga
otpw
ot
Driver registers:
X = 0x80:07:00:00
Y = 0x80:04:00:00
Z = 0x80:06:00:00
E0 = 0x80:06:00:00

ok

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Jan 20, 2018

Member

Your microsteps are reported correctly so I can see three possibilities.

  • Wrong steps per mm setting in Marlin. Check with M503.
  • For some reason the MCU sends too many pulses. Need an oscilloscope or a logic analyzer for this one.
  • double edged stepping is turned on. Needs added code to see the register value.

#9230 Perhaps this issue is related?

Y has different vsense bit because of your current setting.

Member

teemuatlut commented Jan 20, 2018

Your microsteps are reported correctly so I can see three possibilities.

  • Wrong steps per mm setting in Marlin. Check with M503.
  • For some reason the MCU sends too many pulses. Need an oscilloscope or a logic analyzer for this one.
  • double edged stepping is turned on. Needs added code to see the register value.

#9230 Perhaps this issue is related?

Y has different vsense bit because of your current setting.

@thinkyhead

This comment has been minimized.

Show comment
Hide comment
@thinkyhead

thinkyhead Jan 22, 2018

Member

Users on the "3Dtechtalk" Facebook group are wondering about Automatic Current Control, and whether MONITOR_DRIVER_STATUS is an adequate replacement.

image

Can we provide some guidance?

Member

thinkyhead commented Jan 22, 2018

Users on the "3Dtechtalk" Facebook group are wondering about Automatic Current Control, and whether MONITOR_DRIVER_STATUS is an adequate replacement.

image

Can we provide some guidance?

@thinkyhead

This comment has been minimized.

Show comment
Hide comment
@thinkyhead

thinkyhead Jan 22, 2018

Member

Another question arises. When using TMC2130 steppers, I assume it is required to define X_CS_PIN, Y_CS_PIN, etc., for each TMC2130-based stepper driver? If so, then I should add a proper sanity check.

Member

thinkyhead commented Jan 22, 2018

Another question arises. When using TMC2130 steppers, I assume it is required to define X_CS_PIN, Y_CS_PIN, etc., for each TMC2130-based stepper driver? If so, then I should add a proper sanity check.

@bleem313

This comment has been minimized.

Show comment
Hide comment
@bleem313

bleem313 Jan 22, 2018

Another question arises. When using TMC2130 steppers, I assume it is required to define X_CS_PIN, Y_CS_PIN, etc., for each TMC2130-based stepper driver? If so, then I should add a proper sanity check.

I know that at least when using any Ramps based boards, those pin are already defined in the pin_ramps.h file. So maybe changing it so they are defined in the config file and adding a sanity check would be a good thing, as I know I have to redefine them to run the SD card at the same time on my setup.

bleem313 commented Jan 22, 2018

Another question arises. When using TMC2130 steppers, I assume it is required to define X_CS_PIN, Y_CS_PIN, etc., for each TMC2130-based stepper driver? If so, then I should add a proper sanity check.

I know that at least when using any Ramps based boards, those pin are already defined in the pin_ramps.h file. So maybe changing it so they are defined in the config file and adding a sanity check would be a good thing, as I know I have to redefine them to run the SD card at the same time on my setup.

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Jan 22, 2018

Member

@thinkyhead We have the predefined default pins in pins_ramps.h.

Sean is correct that Automatic increased the stepper current until OTPW was triggered and then back down a bit.
Monitoring will just tune down the current if the OTPW is triggered. It will also react to other error conditions like short to ground and open load. By Trinamic's design Short to ground will turn off the driver which of course ruins the print as the other steppers will happily continue moving. The monitoring feature can then abort the print so material is not wasted.

When I get to it, I'll add support for coolStep which further improves the current management capabilities making the stepper current react to load and not just temperature.

Member

teemuatlut commented Jan 22, 2018

@thinkyhead We have the predefined default pins in pins_ramps.h.

Sean is correct that Automatic increased the stepper current until OTPW was triggered and then back down a bit.
Monitoring will just tune down the current if the OTPW is triggered. It will also react to other error conditions like short to ground and open load. By Trinamic's design Short to ground will turn off the driver which of course ruins the print as the other steppers will happily continue moving. The monitoring feature can then abort the print so material is not wasted.

When I get to it, I'll add support for coolStep which further improves the current management capabilities making the stepper current react to load and not just temperature.

@thinkyhead

This comment has been minimized.

Show comment
Hide comment
@thinkyhead

thinkyhead Jan 22, 2018

Member

We have the predefined default pins in pins_ramps.h.

Right, so sanity check would handle the case where TMC2130 is selected with non-RAMPS.

Member

thinkyhead commented Jan 22, 2018

We have the predefined default pins in pins_ramps.h.

Right, so sanity check would handle the case where TMC2130 is selected with non-RAMPS.

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Jan 22, 2018

Member

Well the compiler would tell you that much but I guess having a sanity check would be more descriptive for the end user.

Member

teemuatlut commented Jan 22, 2018

Well the compiler would tell you that much but I guess having a sanity check would be more descriptive for the end user.

@thinkyhead

This comment has been minimized.

Show comment
Hide comment
@thinkyhead

thinkyhead Jan 22, 2018

Member

Yes. The aim of sanity check is to catch what would otherwise be indecipherable compiler errors.

changing it so they are defined in the config file

A PR is in to allow them to be overridable. This is probably not an uncommon need.

Member

thinkyhead commented Jan 22, 2018

Yes. The aim of sanity check is to catch what would otherwise be indecipherable compiler errors.

changing it so they are defined in the config file

A PR is in to allow them to be overridable. This is probably not an uncommon need.

@teemuatlut

This comment has been minimized.

Show comment
Hide comment
@teemuatlut

teemuatlut Jan 22, 2018

Member

The CS pins likely need to be moved to AUX2 as the X_CS is also SDSS and Y_CS is SD_DETECT_PIN. The current defaults are from when Marlin originally used the other TMC library.

Member

teemuatlut commented Jan 22, 2018

The CS pins likely need to be moved to AUX2 as the X_CS is also SDSS and Y_CS is SD_DETECT_PIN. The current defaults are from when Marlin originally used the other TMC library.

#if ENABLED(HAVE_TMC2208)
delay(100);
tmc2208_init();

This comment has been minimized.

@thinkyhead

thinkyhead Mar 18, 2018

Member

@teemuatlut — Re-init not needed in M80?

@thinkyhead

thinkyhead Mar 18, 2018

Member

@teemuatlut — Re-init not needed in M80?

damicreabox added a commit to damicreabox/Marlin that referenced this pull request Sep 14, 2018

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