[1.1.x] More complete Trinamic driver options by thinkyhead · Pull Request #10101 · MarlinFirmware/Marlin

thinkyhead · 2018-03-14T08:53:07Z

Addressing #10087

Add EEPROM Init, Load, Save, Report for M913 Hybrid Threshold.
Bump EEPROM to V53.
Reorder TMC_AxisEnum to match EEPROM storage.
Reduce PROGMEM string usage with small functions.
Add T parameter to M906/M913 to specific which extruder.
Add I (index) parameter to M906/M913/M914 to specify which dual axis stepper.

teemuatlut · 2018-03-14T09:41:17Z

Marlin/configuration_store.cpp

~~No N argument needed?~~
tmc_hybrid_threshold array should be indexed according to the N argument and not P_AXIS.
Each driver can have their unique threshold.

Already patched. It is needed as an index for the array.

thinkyhead · 2018-03-14T12:18:52Z

Currently the M906 and M913 commands always set the same value for all extruders when given an E parameter. Perhaps those commands should be extended to take a T parameter specifying which extruder to set, and if no T parameter is given they should set the active extruder.

thinkyhead · 2018-03-15T04:54:40Z

@teemuatlut Submitted for your approval….

teemuatlut

I don't see anything obvious why something wouldn't work. The most noteworthy is if the if conditions work as expected.

teemuatlut · 2018-03-15T09:05:59Z

Marlin/Marlin_main.cpp

Will this work alright? What's the boolean result of assigning a value to a variable as it is not a comparison in itself?

Non-zero evaluates as true in all C-like languages.

teemuatlut · 2018-03-15T09:07:10Z

Marlin/Marlin_main.cpp

I believe this will change the behavior of M906 so that the values are no longer reported when setting a new value.
This also applies to the following TMC gcodes.

Correct. That is the intent. We don't want the EEPROM loading to spew more information than the M503 report already produces. So the tmc setters no longer repeat the set value. It's also a standard of Marlin to stay quiet when setting values, but to report values when a command is given without arguments.

teemuatlut · 2018-03-15T09:16:35Z

Marlin/configuration_store.cpp

Something like TMC_COUNT would probably be a bit closer but doesn't really matter.

The EEPROM needs to save placeholders for all potential axes. It shouldn't resize based on flexible options. MAX_EXTRUDERS will only change when Marlin gains support for more extruders.

teemuatlut · 2018-03-15T09:26:30Z

Marlin/configuration_store.cpp

Reminds me that we could remove setCurrent from tmc_init functions in stepper_indirection.cpp as we already set current (and some other parameters) here already. The newer versions of the libraries support a push method that re-sends all the shadow registers to the driver in case the driver was not powered by 12V when initially booting the machine, like when PS_DEFAULT_OFF is enabled. But let's not do that yet as it will require testing.

As another standard behavior, Marlin initialized all settings-based values in the EEPROM load and reset code, so yes, if they are also being initialized in stepper_indirection then we can eliminate the redundancy.

Reminds me that we could remove setCurrent from tmc_init functions

It looks like tmc2130_init call would still be needed in response to the M80 command powering the board, unless we can enable those shadow registers.

Yes, but the push command isn't yet even copied into the TMC2208Stepper library.

Looks like we will need to add our own "shadow registers" because if you power down with M81 and then re-power with M80 the settings will revert to those in the configuration, and won't use the ones saved in the EEPROM.

The word hysterisis should be hysteresis.

I'll fix that but it'll break Marlin for a bit. See stepper_indirection.cpp.

settings.reset or settings.load don't handle all the configuration parameters so we still need to call init. Although there may very well still be some that are duplicates.

M80 can call st.push() to sync to the drivers anything that was attempted while there was no 12V power. I'll add this to TMC2208Stepper v0.0.4 as well.

I'll add this to TMC2208Stepper v0.0.4 as well.

Sounds good. I hope we can get that before the 1.1.9 release. In the meantime for this PR I need to come up with some kind of alternative solution.

Note that the settings.load/settings.reset method is called before the stepper.init function. It looks like we'll need to call stepper.init first. But I hope there's not some "Catch-22" here, where we can't call stepper.init before or during EEPROM loading because it depends on having values already loaded from EEPROM.

If stepper.init is called before settings.load then we no longer have to call tmc_init_cs_pins in setup.

TMC2208Stepper v0.0.4 released.
Adds the push method and fixes the same typo that was in TMC2130Stepper.

I've updated Marlin to use the latest libraries. I hoped to check the version number in the SanityCheck.h but it's not visible to the preprocessor. For the next version that would be a useful addition.

teemuatlut · 2018-03-15T18:04:17Z

Marlin/configuration_store.cpp

Same thing here as I said with the v2.0.0 PR. Need to account for dual steppers and possibility for a better output format.

Followup to #10101

thinkyhead added PR: Improvement F: Trinamic labels Mar 14, 2018

thinkyhead requested a review from teemuatlut March 14, 2018 08:53

thinkyhead force-pushed the bf1_eeprom_M913 branch from 9d70074 to 7c3c8b7 Compare March 14, 2018 08:58

teemuatlut reviewed Mar 14, 2018

View reviewed changes

thinkyhead force-pushed the bf1_eeprom_M913 branch 12 times, most recently from 61d6b30 to 7d68ff2 Compare March 14, 2018 12:06

thinkyhead force-pushed the bf1_eeprom_M913 branch 6 times, most recently from ca55aac to 4d9e8ba Compare March 15, 2018 03:22

thinkyhead changed the title ~~[1.1.x] Add M913 Hybrid Threshold to EEPROM~~ [1.1.x] More complete Trinamic driver options Mar 15, 2018

teemuatlut approved these changes Mar 15, 2018

View reviewed changes

teemuatlut reviewed Mar 15, 2018

View reviewed changes

thinkyhead force-pushed the bf1_eeprom_M913 branch 2 times, most recently from 1e265a4 to ff8841e Compare March 18, 2018 01:15

thinkyhead force-pushed the bf1_eeprom_M913 branch 8 times, most recently from bd31a3f to 67c8ef4 Compare March 20, 2018 02:44

thinkyhead mentioned this pull request Mar 20, 2018

[2.0.x] More complete Trinamic driver options #10102

Merged

thinkyhead added 4 commits March 19, 2018 22:06

Consolidate smart stepper driver initialization

a035020

Improve Trinamic support in the EEPROM

84bbd01

Quiet M906/M913/M914 with axes. Patch for DISTINCT_E_FACTORS.

4995e0e

Add 'I' parameter to M906/M913/M914; 'T' to M906/M913

312bebf

thinkyhead force-pushed the bf1_eeprom_M913 branch from 67c8ef4 to 312bebf Compare March 20, 2018 03:06

thinkyhead merged commit bc01200 into MarlinFirmware:bugfix-1.1.x Mar 21, 2018

thinkyhead deleted the bf1_eeprom_M913 branch March 21, 2018 04:14

thinkyhead added a commit that referenced this pull request Mar 23, 2018

Fix size of EEPROM tmc_hybrid_threshold

2746eb5

Followup to #10101

thinkyhead added a commit that referenced this pull request Sep 22, 2018

Fix size of EEPROM tmc_hybrid_threshold

407556e

Followup to #10101

Uh oh!

Conversation

thinkyhead commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teemuatlut Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thinkyhead commented Mar 14, 2018

Uh oh!

thinkyhead commented Mar 15, 2018

Uh oh!

teemuatlut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thinkyhead Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thinkyhead Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thinkyhead commented Mar 14, 2018 •

edited

Loading

teemuatlut Mar 14, 2018 •

edited

Loading

thinkyhead Mar 15, 2018 •

edited

Loading

thinkyhead Mar 15, 2018 •

edited

Loading