Re-Arm 4x TMC2208 stepper improvement

[chrisqwertz]

Requirements

• A Re-ARM board with a RAMPS1.4 on top of it
• 4x TMC2208 stepper driver

Description

Added pin definitions for 4x TMC2208 to be used with RAMPS1.4 + Re-Arm board
Updated to latest TMCStepper lib version.

Benefits

By using the newly improved half duplex mode in the TMCStepper library it is now easily possible to use 4x TMC2208 without the need of having them operate in standalone mode

Related Issues

Fixes #12757

[chrisqwertz]

First pull request, be lenient with me.

[teemuatlut]

I'd rather PIO pull the latest release than latest commit, but I can make a new release soon.

[chrisqwertz]

That'd be awesome!

[teemuatlut]

https://github.com/teemuatlut/TMCStepper/releases/tag/v0.3.2

[chrisqwertz]

Thanks, will improve code format and incorporate your newest release tomorrow

[teemuatlut]

All you need to do is remove the change to platform.ini.

[gloomyandy]

Couple of comments. It is probably not a good idea to include changes to the main configuration files in a PR. The pulse length issue is an interesting one though as it does seem that with some (all?) 32bit boards a longer pulse length is required when using TMC drivers. But I suspect that there is a better way to get this (possibly a conditional change in the Conditionals_post.h file?). Also I'm not sure that the settings in platformio.ini are correct, the original was... TMCStepper@<1.0.0

I'm sure Thinkyhead will sort it all out!

[chrisqwertz]

You are making a very good point.

I guess TMCStepper@<1.0.0 will automatically get the latest release version, is that right?

Regarding the stepper pulse: I took hours until I finally made the effort to hook a oscope to the step pin of the driver slot to figure out what's actually going on.
Anyhow, in my opinion a whole usec is way too long anyway. A few NOPs are probably enough to have the step signal reach its maximum amplitude of 3.3V.
Am I correct, that those NOPs would best be placed in the stepper.cpp?

[chrisqwertz]

Looking into stepper.cpp I found the following code block starting at line 1901:

    // Step E stepper if we have steps
    while (LA_steps) {

      // Set the STEP pulse ON
      #if ENABLED(MIXING_EXTRUDER)
        E_STEP_WRITE(mixer.get_next_stepper(), !INVERT_E_STEP_PIN);
      #else
        E_STEP_WRITE(stepper_extruder, !INVERT_E_STEP_PIN);
      #endif

      // Enforce a minimum duration for STEP pulse ON
      #if MINIMUM_STEPPER_PULSE
        // Just wait for the requested pulse duration
        while (HAL_timer_get_count(PULSE_TIMER_NUM) < pulse_end) { /* nada */ }
      #endif

      // Add the delay needed to ensure the maximum driver rate is enforced
      if (signed(added_step_ticks) > 0) pulse_end += hal_timer_t(added_step_ticks);

      LA_steps < 0 ? ++LA_steps : --LA_steps;

      // Set the STEP pulse OFF
      #if ENABLED(MIXING_EXTRUDER)
        E_STEP_WRITE(mixer.get_stepper(), INVERT_E_STEP_PIN);
      #else
        E_STEP_WRITE(stepper_extruder, INVERT_E_STEP_PIN);
      #endif

      // For minimum pulse time wait before looping
      // Just wait for the requested pulse duration
      if (LA_steps) {
        while (HAL_timer_get_count(PULSE_TIMER_NUM) < pulse_end) { /* nada */ }
        #if MINIMUM_STEPPER_PULSE
          // Add to the value, the time that the pulse must be active (to be used on the next loop)
          pulse_end += hal_timer_t(MIN_PULSE_TICKS);
        #endif
      }
    } // LA_steps

If I understand the code correctly the NOPs should be placed in there

[gloomyandy]

That might work, but doing timing by adding code on these chips can be pretty hit and miss (there is a thread somewhere about it), there is some sort of instruction cache and a flash accelerator (on The LPC176x) at least and these mean that the isntruction timing is not always consistent. Also remember that this code will run on a bunch of different 32 bit systems with different cpu modules. You may want to dig around and see if there is a better way to introduce a small delay, the LPC176x framework has a delay_ns routine, but I'm not sure if it is public or not?

@p3p @thinkyhead @ejtagle @Bob-the-Kuhn any thoughts on what to do about this? The problem seems to be that when using LA the timing of the stepper driver pulses can be different from "normal pulses" and seems to cause problems for TMC (and probably other) stepper drivers. At the moment users are having to work around it by setting a MINIMUM_STEPPER_PULSE, but really that should not be necessary

[ejtagle]

@gloomyandy : As you saw on the code, we are now using the same timer to measure the time spent in high and the time spent in low states. That should work very reliably.
But, as you said, sometimes it does not. Cause could be either that added_step_ticks is not properly being calculated, or it could be a hardware bug:

1. Excessive capacitive load on lines, that makes the pin voltage not to stabilize for enough time
2. Pin ports not set to the "strong" current drive mode (32bit ARM mpus all have that option, usually called "speed")
3. Ground bounce due to improper cabling of the drivers
4. DIR / STEP line with ringing due to high speed transitions without any kind of line termination

The ONLY way to figure it out would be to hook a digital scope (>20mhz bandwidth) in a non working driver and MEASURE the signals. I am not sure if a digital analyzer is enough

I have the equipment necessary to do those measurements, but in my case i have no issues at all...

[gloomyandy]

@ejtagle Do you understand why it is that so many folks are having issues when using linear advance on the LPC176x based boards with TMC drivers? It seems they all need to set MINIMUM_STEPPER_PULSE to 1 or more to get the extruder to work. But the other motors work fine. IS the timing for the extruder (with LA enabled) different to that for the other drivers?

[chrisqwertz]

@ejtagle The signal looks fine to me, 15ns before switching to low again seems quite short, though

[chrisqwertz]

@gloomyandy Thanks for pointing out the linear advance feature as a possible cause.

Disabling LIN_ADVANCE fixes the issue!

[ejtagle]

@chrisqwertz : Thanks for measuring it out! ... You are absolutely right . 15ns is a extremely short pulse. On that signal capture we both see a rise time of about 15nS, but even worse, the scale is 1v per division, so it seems the signal was unable to reach the 3.3v that it should, It only reached 1.8v. That means the actual rise time is 27nS (and the fall time doubles that, about 55nS). Basically, due to (parasite) capacitive loading of that line, the maximum reliable frequency of operation is about 9Mhz.
The Marlin code , as it is right now, does not allow to set less than 1uS, so the solution is to set MINIMUM_STEPPER_PULSE to 1, and probably, the best solution here would be to perform a slight refactoring of the code to allow to specify MINIMUM_STEPPER_PULSE in nanoseconds, and set it to 60nS

[teemuatlut]

Does LIN_ADVANCE calculate the delay in a different manner?
I wonder where we left off with utilizing the TMC double edged trigger feature...

[ejtagle]

@teemuatlut : No, LIN_ADVANCE calculates the delay in the exact same way. But, when using MINIMUM_STEPPER_PULSE = 0, the delay varies between using LIN_ADVANCE and not using it, being the reason that the code that pulses the step line is way more simpler when it is pulsed by LIN_ADVANCE, and so the pulse width becomes narrower. Basically, not using LIN_ADVANCE and MINIMUM_STEPPER_PULSE = 0 is using the execution speed of the processor and the instructions in between changing the step pin status as delay

[ejtagle]

@teemuatlut : Using the double edge feature of the TMC could save some time, without any doubt. But LIN_ADVANCE would have the same issue. MINIMUM_STEPPER_PULSE should be changed to be specified in nanoseconds... and use ceiling rounding for 32bit cpus...
It is actually a relatively easy refactor...

[chrisqwertz]

@teemuatlut : No, LIN_ADVANCE calculates the delay in the exact same way. But, when using MINIMUM_STEPPER_PULSE = 0, the delay varies between using LIN_ADVANCE and not using it, being the reason that the code that pulses the step line is way more simpler when it is pulsed by LIN_ADVANCE, and so the pulse width becomes narrower. Basically, not using LIN_ADVANCE and MINIMUM_STEPPER_PULSE = 0 is using the execution speed of the processor and the instructions in between changing the step pin status as delay

@ejtagle This is not correct, it's the other way round. Disabling LIN_ADVANCE creates a normal pulse length with MINIMUM_STEPPER_PULSE set to 0. Enabling it shortens it!

[chrisqwertz]

@teemuatlut Just to clarify:

MINIMUM_STEPPER_PULSE is commented out in both cases. Enabling LIN_ADVANCE shortens the stepper pulse width to what you see on my oscope screenshot above.

It also breaks z-moves when homing with about 70% probability on each G28 Z

[teemuatlut]

@ejtagle It might not make the ISR much faster if there's not much code being dropped but I'm more thinking about the rise/fall times of the signals and how that shouldn't be an issue anymore if the step signal is only toggled once per ISR. This way the code could be as "freerunning" as possible with no artificial delays, even on faster 32bit.

[teemuatlut]

@chrisqwertz So my understanding is that the delay (NOP) code is not compiled in at all if min stepper pulse is defined as 0? But there is some code in between the pulse and this code is faster to execute when using linear advance. I'll check myself in a couple of hours but need to do a few things first.

Oh and you can drop any changes to platformio.ini as they're not needed. The version condition works as intended.

[gloomyandy]

I'm being lazy because I could look, but it is not that easy to do so at the moment. Does anyone know if the HALs expose a nanosecond time value that can be used by this code? I think in theory for the LPC176x at least the resolution of the timer/counter used for the stepper could be set to be nanoseconds rather than microseconds but that would limit the max time period which may not be a good idea.

@teemuatlut I don't think there are any nops involved. Rather when a min pulse of 1 or more is defined the code uses a timer value and compares against that (but currently the timer min value is 1uS which may be too long/crude a value). When no min pulse is specified you just get a delay that depends on the actual code being executed. Because the lin advance code is simpler than the "normal code" you get a shorter delay in this case, which creates this problem.

I wonder if now would be a good time to look again at using a timer interrupt to turn on and off the pulse (so no delays, just the timer(s) generating the pulse on and pulse off interrupts. I think @p3p did some investigation of this.

[chrisqwertz]

I don't know if it helps, but doing a bed leveling with UBL, I also can hear a bit of noise/stuttering on the deceleration ramp coming from the x & y stepper

[chrisqwertz]

@teemuatlut To wrap this up and make the changes ready to be pulled:

4x TMC2208 in combination with a Re-ARM controller and a RAMPS1.4 on top of it works. I did quite a bit of testing, so from my point of view this PR is ready to be incorporated.

( There is an unrelated issue with LIN_ADVANCE, see #12403 for more information. )

[hobiseven]

@ejtagle The signal looks fine to me, 15ns before switching to low again seems quite short, though

Is that supposed to be a CLK signal? That looks awfull Guys, those 32 bits CPUs run @ 72Mhz.... and there should be no issue to toggle CPU pins. Usually, you just have one gate load, and only a 1 inch of track. This is a really ugly trace you have.

[gloomyandy]

I think you probably need to start again with this PR. It is better to have a PR on a branch not directly on 2.0.x that makes it much easier to work with. There also seem to be a number of commits and reversions of code that make it very hard to follow exactly what is being proposed. So I would create a branch from the latest 2.0.x and just add the actual changes that are needed, don't include any configuration for your printer. The create a new PR based on that branch and reference it here.

[chrisqwertz]

Yes, I'll do that. Thanks for your input!

[chrisqwertz]

@hobiseven You are absolutely right: Datasheet says 5ns max rise/fall time:

[gloomyandy]

As to the signals. Where are you probing the value? On the ReArm board, on the Ramps board, on the stepper driver board? Are you attaching the probe directly to the pin (or using an extension wire of some sort), what sort of probe are you using? What Scope? All of the above will impact the sort of output you will see.

[hobiseven]

For the STM32/ Alfawise issue , I will do that directly on the digital trace, with a TDS784, with 1Gb bandwidth. And as this scope has instavue, I will accumulate traces. Just to start, as to be honnest, I do not know yet what I will see, but the printer feels so rough with Marlin compared Alfawise, I expect to get something a bit nasty there.

[hobiseven]

@gloomyandy Yes, in principle you are right, but remember that those stepper drivers are driving real electromechanic stuff which is Waaayyyyy slower than anything digital. Such a trace above is really ugly, as you are prone to any power supply hickup for example, and that is really not a good design practice.

[gloomyandy]

@hobiseven what STM32/Alfawise issue? There is no reference to any of that here )at least not that I can find).

But anyway other than the duration (which is really what this thread is all about), I'm struggling to see how anything in Marlin can influence the "quality" of the pulse, it basically just turns it on and then off. The main question here is what is the best way to control the time it is on for and what (minimum) period that should be. At least that's my take on it.

Oh and no nothing to do with any aviation stuff, sorry.

[hobiseven]

@gloomyandy
There is a mix of 2 threads here. Sorry for this mistake I made. We are having skipped pulses, or what looks to be skipped pulses on STM32. This seems to be too short pulses. And we made 10 people trying all sorts of setup, to come to the conclusion that we need to increase stepper VREF to an unresonnable level to not get layer shifts. > That looks to me pulse timings are too short, and I want to check this on an oscilloscope . It is issue # 12403

[gloomyandy]

@hobiseven Yeah I've seen that thread but it has so much noise in it, it is hard to pick out the real issues. Just in case you didn't see the most recent comments, your step calculations seem to be out by 16, the microstepping is already included in the steps per mm value. Good luck with the measurements.

[hobiseven]

@gloomyandy Yes I saw that. In any case I want now to get rid of speculations, and catch the real issue. We have made so many tests, and I quite sure that the issue is in the step generation / timing , which could actually also have an effect on TMC2208.
I will only post real info, based on measurements with pro oscilloscope.

[hobiseven]

@chrisqwertz Looks like I am looking at the same issue in the other thread #12403 .
There is either one missed step, or one too many, and that of coures heavily disturbs the stepper motors.
And I have that , about once every 30 minutes or so now.

A tiny little glitch of 1 or 2 CPU cycles. Please look at my suggestion to locate in time this glitch compared to direction signals, or pre and post clock cycles.

[ManuelMcLure]

@chrisqwertz I wonder whether you could explain the rationale behind the pin ordering for UART - if all 4 drivers are TMCs it doesn't matter, but I was thinking that if only X and Y are being used as TMC drivers the following order would be better:

#define X_SERIAL_TX_PIN  P0_01
#define X_SERIAL_RX_PIN  P0_01
#define Y_SERIAL_TX_PIN  P0_00
#define Y_SERIAL_RX_PIN  P0_00
#define Z_SERIAL_TX_PIN  P2_13
#define Z_SERIAL_RX_PIN  P2_13
#define E0_SERIAL_TX_PIN P2_08
#define E0_SERIAL_RX_PIN P2_08

This would still allow two extruders on a system with TMC drivers on X and Y.

[chrisqwertz]

Hey @ManuelMcLure sorry for the late reply. I would have to look through my notes why that pinout was exactly chosen, but to me the pins you chose look very suspicious. Are you sure that your defines meet all criteria (interrupt capable pin Not every port is hardware interrupt capable, not used by other periphery like USART, LCD, and sometimes a RAMPS pin might be NC or tripple mapped from the rea-rm, et cetera). Maybe they even need to be 5V0 tolerant. Again, I have a lot of pins in use and or blue wired somewhere else on my raps setup.

TLDR:
I am a bit short on spare time atm. I'd greatly appreciate it, if you could verify yourself that your pins meet the same hardware criteria as mine.

[ManuelMcLure]

I used the same pins that were in your commit from Apr 28 (df8c412) - the only change I made was to swap the pins used by X and Z.

In any case, the limitation on pins having to be interrupt-capable is no longer an issue per @gloomyandy - pretty much any pin can be used as long as it's not used for anything else and there's no odd external circuitry connected to it.

[chrisqwertz]

In this case, sorry for adding confusion to the matter.
@gloomyandy Do you by chance know with which commit introduced that int pin enhancement?

[gloomyandy]

The software serial code is not part of Marlin, it is included in the LPC176x Arduino framework. The new code was merged into the framework on Mar 1st 2019. The version of the framework with the software serial code in it was released on the same day. I'm pretty sure that software serial did not work on the LPC176x prior to this commit.