Skip to content

[2.0.x] Configure stepper drivers per axis#11264

Merged
thinkyhead merged 2 commits intoMarlinFirmware:bugfix-2.0.xfrom
teemuatlut:bf2_define_drivers
Jul 25, 2018
Merged

[2.0.x] Configure stepper drivers per axis#11264
thinkyhead merged 2 commits intoMarlinFirmware:bugfix-2.0.xfrom
teemuatlut:bf2_define_drivers

Conversation

@teemuatlut
Copy link
Member

Builds upon #11133

  • Add axis specific driver definitions to determine proper timing parameters like step pulse width, delay after asserting dir level and maximum stepper frequency.
  • The user can use settings in Configuration_adv.h to override the parsed driver IC defaults.
  • The new driver definitions are used to determine which TMC or L6470 are in use and need initialization.
  • By default the Allegro A4988 drivers are enabled for X, Y, Z and E0. More on this below.
  • Add new TMC macros that work by checking for a certain feature, rather than the dev checking for a list of driver types that support the feature.
  • References to TRAMS removed and will simply be handled by defining the four axis as TMC5130. However this PR doesn't yet move Marlin to the library that supports the driver.
  • Differentiating between TMC2130 in SPI configured mode and standalone mode is done by having two driver definitions; TMC2130 and TMC2130_STANDALONE. The way I see it, the default use case for that driver is in configured mode and using standalone mode is a special case. In standalone mode only the stepper timings are affected and they pretty much act the same as TMC2100 as far as Marlin is concerned.
  • Updates Travis to use the new driver definition method so that they actually are tested.

I decided to use the Allegro drivers as the default to match what is the most common configuration in the user base. Specifying any drivers is not forced but will default to DRV8825 configuration, except for DIR delay. This could be improved, or force defining at least one driver (unless overriden in _adv.h).
I think the main question is if we know the parameters different drivers require, which should be used as the default? I think we should not use 0 delays as there is no driver compatible with such timings.
However, this doesn't take into account the natural delay there is no matter what we do.
This also leads to a more general configuration change that affects the stepper ISR. Perhaps we should no longer check if MINIMUM_STEPPER_PULSE is defined, because from hereon it always is, and rather check if the defined pulse width is greater than 20 for example. @ejtagle ?


The compile time driver IC macros:

AXIS_DRIVER_TYPE(A, T)
Check if an axis A has a driver type T associated with it.

HAS_DRIVER(T)
Check if the user has driver type T in any of the axis enabled.

HAS_TRINAMIC
Check if any supported Trinamic drivers are enabled in any axis.

AXIS_IS_TMC(A)
Check if the specified axis A has a supported Trinamic driver enabled.

@GMagician
Copy link
Contributor

@teemuatlut may I close my pull request?

@teemuatlut teemuatlut force-pushed the bf2_define_drivers branch from a9f1788 to 68b4875 Compare July 14, 2018 15:28
@teemuatlut
Copy link
Member Author

Yes, I believe so.

@ejtagle
Copy link
Contributor

ejtagle commented Jul 14, 2018

@teemuatlut : This PR does not impact the stepper ISR, i have no problems with it.
I still have to do fine tuning of timing on the stepper, but that will not affect this PR, so no problems

@teemuatlut
Copy link
Member Author

Currently in upstream we have a condition for the step width: #if MINIMUM_STEPPER_PULSE.
I think this condition can be removed as from now on we will always have it defined.
My reasoning then was that we will then also always have a defined delay compiled in, contrary to how it currently is.

Copy link
Member

Choose a reason for hiding this comment

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

Is this sanity-check obsolete already, or is it just now becoming obsolete with this PR?

Copy link
Member Author

@teemuatlut teemuatlut Jul 18, 2018

Choose a reason for hiding this comment

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

It's mostly just shuffling the code around to bring more sense into it. Nothing is being removed.

Copy link
Member

Choose a reason for hiding this comment

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

Does this TMC_Z_CALIBRATION test get replaced elsewhere, or is TMC_Z_CALIBRATION being dropped?

Copy link
Member Author

Choose a reason for hiding this comment

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

@thinkyhead thinkyhead force-pushed the bf2_define_drivers branch from c3789ed to 221466f Compare July 18, 2018 01:59
Copy link
Member

Choose a reason for hiding this comment

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

I see in a few configurations that ENDSTOP_INTERRUPTS_FEATURE is being disabled. Is this because the pins that go with these configs are not interrupt-capable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not intentional. I'll need to redo the search&replace operation.

@thinkyhead
Copy link
Member

Overall it looks very good. It's much better than the previous method of configuration, with the details now hidden from the user and a much more expressive connection between stepper drivers. This will get merged pretty soon, methinks.

@thinkyhead thinkyhead force-pushed the bf2_define_drivers branch 6 times, most recently from 23dadfd to 8a2c9ae Compare July 25, 2018 02:04
thinkyhead and others added 2 commits July 25, 2018 02:47
@thinkyhead thinkyhead force-pushed the bf2_define_drivers branch from 8a2c9ae to fbcdf5e Compare July 25, 2018 07:51
@thinkyhead thinkyhead changed the title [2.0.x] Add driver definitions and rework TMC macros [2.0.x] Configure stepper drivers per axis Jul 25, 2018
@thinkyhead thinkyhead merged commit 4dfbae0 into MarlinFirmware:bugfix-2.0.x Jul 25, 2018
@teemuatlut teemuatlut deleted the bf2_define_drivers branch August 13, 2018 16:54
@tcm0116
Copy link
Contributor

tcm0116 commented Aug 31, 2018

@teemuatlut - FYI it looks like the new DRIVER_TYPE definitions weren't added to the generic delta example configuration as part of this PR.

@teemuatlut
Copy link
Member Author

So it seems. Must be because for whatever reason the endstop inverting is turned on by default. I don't know why that should be the case.
I do all config file updates with regex replacements and that's why the file wasn't picked up.

@thinkyhead
Copy link
Member

509e1cf

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.

5 participants