[2.0.x] Specify stepper drivers per axis

[GMagician]

Use drivers type to initialize timings

[GMagician]

This is an attempt to satisfy #11119. Maybe is not the best, maybe some define are in wrong place but at the moment is just an idea.

[thinkyhead]

A good start. But note that this is in conflict with HAVE_TMC2130, HAVE_TMC2208, HAVE_L6470DRIVER, and their sub-options, so more changes will be needed throughout the code to supplant those.

[iosonopersia]

You can have a look at what @MagoKimbra did in MK4duo (obviously the file structure there is quite different, but not that much) MKFirmware/MK4duo@4dc684a

[teemuatlut]

this is in conflict with HAVE_TMC2130, HAVE_TMC2208

I know I'm repeating myself but never hurts to reiterate even if just for documentation purposes;
these configuration settings will no longer be used as such, but rather you tell Marlin the types of (TMC) drivers you're using. Then we use collective macros to determine if you have a specific driver type in use, or in more general terms, if you have drivers in use that are capable of a certain feature, such as stallGuard for example. We'd be moving away from supporting specific drivers and more towards supporting features and technologies.
I have no problems with moving the driver selection section to Configuration.h.

tmc_models.h
Conditionals_post.h

[thinkyhead]

these configuration settings will no longer be used as such

It's just a matter of whether to do this now and include the change in 1.1.9, or retain the current method for the final 1.1.x release and apply this new method only to 2.0.x.

[teemuatlut]

I'd say leave 1.1.x be and apply only to 2.0.x after the 1.1.9 release.

[GMagician]

@thinkyhead

this is in conflict with HAVE_TMC2130, HAVE_TMC2208

For spi control used/not used I was thinking of
#define X_DRIVER TMC2130 + SPI
where SPI is defined as 0x10000 and all drivers value will be masked with 0xFFFF.

with this I can simply redefine HAVE_TMC2130 with HASDRIVER(TMC2130) && DRIVERSHASSPI(TMC2130) removing almost everything from configuration_adv.h

My 'driver' definition is more general than @teemuatlut since it apply also to driver with no features at all, but I think he can use mine as base to link in his collective macros.

eg #define HAVE_TMC(MODEL) [cut] may simply become #define HAVE_TMC(MODEL) HASDRIVER(TMC##MODEL)

so this will simplify also his work.

Now I don't know if it's better to use 'HAVE' or 'HAS'. I meant printer/system this is why I used 'HAS'.

[GMagician]

@iosonopersia

You can have a look at what @MagoKimbra

I'll take a look at it

[GMagician]

Ok, now it's almost to the end. Missing only driver and timing in examples

Since I saw @teemuatlut will add 2660 and 5130 I also prepared defines for them

[p3p]

I could be wrong but I think the logic added to configuration_adv should be in the sanity check, the configuration files should be pure data.

[GMagician]

@p3p sanity should check mismatch thing and raise error, maybe these should go in conditional_post.h but something must be done to let user override assumed values...but really someone need to override them?

[teemuatlut]

Configuration.h
#define X_DRIVER_TYPE DRV8825
//#define Y_DRIVER_TYPE TMC2100
...

Configuration_adv.h:
// Optional override
//#define STEP_PULSE_WIDTH 5

Configuration_post.h
#define HAS_DRIVER(IC) (X_DRIVER_TYPE == IC) || (Y_DRIVER_TYPE == IC) || ...

#ifndef STEP_PULSE_WIDTH
  // Pulse width definition in order
  #if HAS_DRIVER(DRV8825)
    #define STEP_PULSE_WIDTH 10
  #elif HAS_DRIVER(TMC2100)
    #define STEP_PULSE_WIDTH 9
  #elif HAS_DRIVER(A4988)
    #define STEP_PULSE_WIDTH 8
  #else
    // Probably don't need this unless compiler expects it to be defined.
    // By default let Marlin run as fast as possible
    #define STEP_PULSE_WIDTH 0
  #endif
#endif

This leaves the driver selection as optional, while also giving an override option for those who would want to tweak the timings.
If the user leaves all driver type definitions disabled and doesn't specify a pulse width override, then we just assume an appropriate default value.

EDIT:
It's possible the DRIVER_TYPE == TMC2100 comparison doesn't work
and we need to define TMC2100 1234.

[GMagician]

@teemuatlut I agree with you but your solution is to arrange STEP_PULSE_WIDTH test from worst to best.
I though to do same solution but there is an "issues" with it:
PROS

• Simple test

CONS

• You have more sections to edit (now 3 but who knows in future, pulse width, step/dir delay and max freq) with different drivers test order. And new addition must be inserted in correct place.

My solution
PROS

• to add a new driver just copy and paste one, put wherever you want and adjust values.

CONS

• Perverse coding

If I would be the only maintainer I would choose for sure your solution, but since many people may put hands on it, I think mine is safer.

survey for maintainers: what do you think is better?

Also, macros can be reduced (coping @MagoKimbra solution) and I'll do that also to clean up code.

[Sineos]

Likely that I have missed it but I couldn't find the logic to tune the enitre system to comply with the slowest timings / lowest frequency in case of mixed configurations.

[teemuatlut]

@GMagician I'd rather not see your definition ladder in a configuration file.

It's quite rare for us to add new drivers, and especially drivers that would need special timing constraints. If for example the user would define the driver type as ABC1234 (sounds like a Alphabet product), then Marlin would just use the default values. There would be nothing to add. Only in the case the driver wouldn't like our default values.
It's likely that the parameters can be grouped together. Maybe the driver that needs the longest pulse width also needs the longest dir delay.
And even in the case where the end user was using a new driver that needed special attention but wasn't supported yet, there would still be an easy way to override the parameters.

Also we go to some lengths to improve usability at the expense of added code lines.

[GMagician]

@teemuatlut

There would be nothing to add. Only in the case the driver wouldn't like our default values.

Current defaults
MINIMUM_STEPPER_DIR_DELAY 0
MINIMUM_STEPPER_PULSE 2
are worst for most drivers but not valid for TB6560

It's likely that the parameters can be grouped together. Maybe the driver that needs the longest pulse width also needs the longest dir delay.

Unfortunately this is not true:
MINIMUM_STEPPER_DIR_DELAY TMC:20 A4988:200
MAXIMUM_STEPPER_RATE TMC:400000 A4988:500000

But I can't understand correctly

Also we go to some lengths to improve usability at the expense of added code lines.

[GMagician]

@Sineos

Likely that I have missed it but I couldn't find the logic to tune the enitre system to comply with the slowest timings / lowest frequency in case of mixed configurations.

look at configuration_adv.h

#if HAS_DRIVER(TB6600)
  #if MINIMUM_STEPPER_DIR_DELAY < 1500 // guess, no info in datasheet
    #undef MINIMUM_STEPPER_DIR_DELAY
    #define MINIMUM_STEPPER_DIR_DELAY 1500
  #endif
  #if MINIMUM_STEPPER_PULSE < 3
    #undef MINIMUM_STEPPER_PULSE
    #define MINIMUM_STEPPER_PULSE 3
  #endif
  #if !defined(MAXIMUM_STEPPER_RATE) || MAXIMUM_STEPPER_RATE > 150000
    #undef MAXIMUM_STEPPER_RATE
    #define MAXIMUM_STEPPER_RATE 150000
  #endif
#endif

every driver type has its own tests

[GMagician]

@teemuatlut

it's possible the DRIVER_TYPE == TMC2100 comparison doesn't work and we need to define TMC2100 1234.

that's already done, TMC2100 is not a string but a number and macro are already working

[Sineos]

Thanks @GMagician
I was thinking that, for example, I have X,Y,Z as TB6600 and now put in a TMC for E that the HAS_DRIVER(TMC...) would override the HAS_DRIVER(TB6600)

[Sineos]

Additional question:

[Sineos]

Absolutely possible that I do not fully understand this but:

• Wouldn't it make sense to have a #define MAXIMUM_STEPPER_RATE 0 here as well?
• To allow manual override in the following shouldn't the constructs all be analog to #if !defined(MINIMUM_STEPPER_DIR_DELAY) || MINIMUM_STEPPER_DIR_DELAY < 200

[GMagician]

This is wrong "default and overriding value", but I'll change thing like suggested by teemuatlut, moving defines in other files and simplifying test as magokimbra has done

[GMagician]

@thinkyhead now I think is ready to evaluate if merge or not.

Do travis need to be changed for this? I don't know how

[teemuatlut]

What's the purpose for DRIVER_USES_SPI?

[GMagician]

@teemuatlut DRIVER_TYPE is used to set up timings and nothing more, DRIVER_USES_SPI is used to add all communication code required. You may have also TMC without using SPI, I think

[thinkyhead]

Since we're still allowing these values to be overridden, we might want to preserve the comments that describe the timings for each type of driver. If for no other reason that it provides some extra documentation.

[thinkyhead]

I think we should set these to the values for DRV8825, but leave them commented-out, since we mention that DRV8825 is the default on the main driver type config option. And it makes it clear that these have to have a numeric value.

[thinkyhead]

I think these should all have the default value of DRV8825, but still remain commented out, since that is the default and it makes it clear that these must be defined as one of the listed options, and are not just on/off switches.

[thinkyhead]

Rebased, squashed, and added some commits based on my review comments.

[GMagician]

@thinkyhead I think that there are some more missing sanitychecks to add (?_IS_TMC2130 and ?_IS_TMC2208) but I'm having some issue with git to get back it aligned with what you did...

EDIT @thinkyhead may you help me with github to resync my branch? don't know which commands to use and I don't want to mess up

[GMagician]

@thinkyhead I solved github... deleted local branch and re-got it from remote

When everything is reputed ok I'll start 1.1.x (if required)

[thinkyhead]

I solved github... deleted local branch and re-got it from remote

Cool. This also works:

git fetch origin
git checkout 2.0.x-define-drivers-axis
git reset --hard origin/2.0.x-define-drivers-axis

[GMagician]

@thinkyhead TMC2660 & TMC5130 + SPI are not yet implemented, maybe @teemuatlut is working on that but it will be something coming next. Don't know if change again all .h...

[thinkyhead]

TMC2660 & TMC5130 + SPI are not yet implemented

Since this is limited to the 2.0.x branch I'm not too worried about it right now.

Note that TMC5130 will be SPI-only. And currently these drivers are implied by IS_TRAMS, which is also not yet implemented.

I've rebased and squashed the branch, so you can fetch the updated version anytime using the git commands I listed above.

[GMagician]

@thinkyhead

Note that TMC5130 will be SPI-only

Sure?

1.4.2 STEP/DIR InterfaceThe motor can optionally be controlled by a step and direction input. In this case, the motion controller remains unused.Active edges on the STEP input can be rising edges or both rising and falling edgesas controlled by another mode bit (dedge).

[thinkyhead]

I'm not aware of a stepstick style driver based on TMC5130. The only implementations I've seen are SPI-based to take advantage of the built-in motion controller. (There's no reason to use the expensive TMC5130 while eschewing the motion controller.)

[thinkyhead]

I've reverted it just in case someone decides (for some reason) they want to use a 5130 without SPI.

[p3p]

Active edges on the STEP input can be rising edges or both rising and falling edgesas controlled by another mode bit (dedge)

Wonder what other drivers support double edge stepping, getting rid of the wasted cycles during pulses sounds like an optimisation waiting to happen

[AnHardt]

There is a simple way to eliminate all step pulse waiting when not double/quad stepping.
Doubling the amount of steps while toggling the step lines will give a highly symmetric duty cycle for all axes. Double/quad stepping may still need some waiting.

[GMagician]

Since this is limited to the 2.0.x branch I'm not too worried about it right now.

Then no 1.1.x counterpart?

[thinkyhead]

Then no 1.1.x counterpart?

Maybe… I know @teemuatlut wants to give it a review and suggest more changes. And it's possible 1.1.9 is on the verge of release. Just a couple of subtle bugs to work out at the moment.

[teemuatlut]

@GMagician I'm forking your branch from here and combining it with what I had before for the TMC drivers. Maybe this way we can get most of the macro changes done in one go.

[GMagician]

No more required after teemuatlut pull request