Skip to content

[2.0.x] Adaptive multi-axis step smoothing, some fixes#10922

Merged
thinkyhead merged 2 commits intoMarlinFirmware:bugfix-2.0.xfrom
ejtagle:bugfix-2.0.x
Jun 10, 2018
Merged

[2.0.x] Adaptive multi-axis step smoothing, some fixes#10922
thinkyhead merged 2 commits intoMarlinFirmware:bugfix-2.0.xfrom
ejtagle:bugfix-2.0.x

Conversation

@ejtagle
Copy link
Contributor

@ejtagle ejtagle commented Jun 3, 2018

Let's go one by one (sorry, fixes were done while implementing the AMASS algorithm. Even if it has the same name and the same idea of Grbl, this is done from 0, as the Grbl one is not well documented, has a round-by-one error, and can't be adapted to the Marlin infrastructure...

First, the "easy" fixes:

  • We removed the CONFIG_STEPPERS_TOSHIBA. The reason is that we already have an option to specify the minimum pulse duration, and, due to several bugfixes, now the compiler properly figures out the maximum and minimum stepper frequency allowed for a given driver, so there is no reason for this configuration at all.

  • We moved the AVR timer initializations to the AVR HAL

  • On the Stepper ISR, we now always use the Stepper timer to measure the pulse width and enforce it. There were several problems with this code, including that in AVR we previously were using the Temperature timer, and that timer has a period of 1 millisecond, with autoreset. So, reading the timer value and adding a number of extra cycles to it and waiting until the timer reaches that value is dangerous, can lead to a system lockup if the read value was close to the period of the timer.
    Now, we use the stepper timer itself, and as we on purpose set its period to the maximum at the Stepper ISR entry, and the timer has a very low value (=0) at the Stepper ISR start of execution, switching reads to the Stepper timer is IDEAL and does not have any problems.

  • We removed the code that calculates delays as a "sum" of "possible" count of cycles: Reason: This was absolutely wrong on ARM, and on AVR it is risky, as the compiler is able to move code around. Now we ALWAYS use the Stepper timer to enforce pulse width. The cost of reading the timer is nothing compared to the rest of the ISR. I have disassembled the AVR isr, and just ONE Bresenhan PULSE_START() / PULSE_STOP() takes 1.2uS to execute. Reading the timer takes 0.2uS

  • All references to PULSE_TIMER_* were removed. Not used anymore.

  • Small tweaks to the MakerParts 3D printer configuration (the one i have)

  • Now we auto-estimate minimum and maximum step frequencies based on the MPU (32bit vs AVR), LINEAR ADVANCE and the minimum step pulse configured. No more guessing. And that also allows to get proper performance from 32bits boards, that are more than capable of stepping at 250kHz with no doubling of pulses - And AVR also has its limits tweaked a bit.

  • For mixing extruders, and to allow them to run using Linear Advance, we changed the way stepping is computed for them: We still use bresenham, but we only execute bresenham for the ratios if we get a step for the E "virtual" stepper. In this way, we can reuse the Linear Advance infrastructure for both.

  • Fixed several problems with Mixing extruders: The motor directions of those extruders were never changed! ... Fixed!

  • Reordered and removed variables that were not used at all in the Stepper class.

  • Fixed a problem where the count of steps per ISR invocation was NOT set when reaching nominal speed ... Ouch!! ...

  • Removed redundant (and/or incorrect) limits on Stepper ISR period. AVR is more capable than you think, And removed ALL calls to SERIAL_ECHO from the stepper ISR. The reason is that even if they are called on edge cases, the compiler is forced to preserve THE FULL environment of those functions (used registers, and in the worst case even floating point state. Please DO NOT call serial_echo from ISR! - Even if the call will mostly never execute, this causes increased resource usage!)

  • Added a folder docs with the deduction of the oversampling bresenham algorithm. Reason is that this algorithm is my own invention, will not find it anywhere else! - And as the proof is "long" , instead of polluting the code with the demonstration, i think it is a better idea to keep it in a separate file.

  • Finally. the adaptive multiaxis step smoothing algorithm:

Oversampling Bresenham algorithm:

Even if Bresenham does NOT lose steps at all, and also does NOT accumulate error, there is a concept i would call "time resolution" - If the quotient between major axis and minor axis (major axis means, in this context, the axis that must create more step pulses compared with the other ones, including the extruder)

Well, if the quotient result is not an integer, then bresenham, at some points in the movement of the major axis, must decide that it has to move the minor axis. It is done in such way that after the full major axis movement has executed, it also has executed the full movements of the minor axis. And the minor axis steps were properly distributed evenly along the major axis movement. So good so far.

But, as said, Bresenham has "discrete" decision points: It can only decide to move (or not to move) minor axis exactly at the moment the major axis moves. And that is not the ideal point (in time) usually.

With slow movements that are composed of a similar, but not equal number of steps in all axis, the problem worsens, as the decision points are distributed very scarcely, and there are large delays between those decision points.

It is nearly trivial to extend bresenham to "oversample" in that situation: Let´s do it:

Assume that we want to use Bresenham to calculate when to step (move in Y direction), but we want to do it, not for integer increments of the X axis, rather than, for fractional increments.

Let's call 'r' the count of subdivisions we want to split an integer increment of the X axis:

m = Δy/Δx = increment of y due to the increment of x1

Every time we move 1/r in the X axis, then the Y axis should move m.1/r

But, as said previously, due to the resolution of the screen, there are 2 choices:

  • It may plot the point (x+(1/r),y), or:
  • It may plot the point (x+(1/r),y+1).

That decision must be made keeping the error as small as possible:

-0.5 < ε < 0.5	

So, the proper condition for that decision is (m/r is the increment of y due to the fractional 1/r increment of x):

y + ε + m/r < y + 0.5
ε + m/r < 0.5									[1]

Once we did the decision, then the error update conditions are:

Decision A:

ε[new] = y + ε + m/r - y				
ε[new] = ε + m/r							[2]

Decision B:

ε[new] = y + ε + m/r - (y+1)		
ε[new] = ε + m/r - 1					[3]

We replace m in the decision inequality [1] by its definition:

ε + m/r < 0.5							
ε + ΔY/(ΔX*r) < 0.5		

Then, we multiply it by 2.Δx.r:

ε + ΔY/(ΔX*r) < 0.5		
2.ΔX.ε.r + 2.ΔY < ΔX.r

If we define ε' = 2.ε.ΔX.r then it becomes:

ε' + 2.ΔY < ΔX.r						[4]

Now, for the update rules, we multiply by 2.r.ΔX

ε[new] = ε + m/r					
2.r.ΔX.ε[new] = 2.r.ΔX.ε + 2.r.ΔX.ΔY/ΔX/r
2.r.ΔX.ε[new] = 2.r.ΔX.ε + 2.ΔY
ε'[new] = ε' + 2.ΔY						[6]
ε[new] = ε + m/r - 1			
2.r.ΔX.ε[new] = 2.r.ΔX.ε + 2.r.ΔX.ΔY/ΔX/r - 1 . 2.r.ΔX
2.r.ΔX.ε[new] = 2.r.ΔX.ε + 2.ΔY - 2.ΔX.r
ε'[new] = ε' + 2.ΔY - 2.ΔX.r				[7]

All expressions, the decision inequality [4], and the update equations [5] and [6] are integer valued. There is no need for floating point arithmetic at all.

Summarizing:

Condition equation:
		
  ε' + 2.ΔY < ΔX.r						[4]
			
Error update equations:
	
  ε'[new] = ε' + 2.ΔY					[6]
		
  ε'[new] = ε' + 2.ΔY - 2.ΔX.r				[7]

This can be implemented in C++ as:

class OversampledBresenham {
  private:
    long divisor; // stepsX
    long dividend; // stepsY
    long advanceDivisor; // advanceX
    long advanceDividend; // advanceY
    int errorAccumulator; // Error accumulator

  public:
    unsigned int ticker;

    OversampledBresenhan(const long& inDividend, const long& inDivisor, int rate) {
      ticker = 0;
      divisor = inDivisor;
      dividend = inDividend; 
      advanceDivisor = divisor * 2 * rate;
      advanceDividend = dividend * 2;
      errorAccumulator = -divisor * rate;
    }

    bool tick() {
      errorAccumulator += advanceDividend;
      const bool over = errorAccumulator >= 0;
      if (over) {
        ticker++;
        errorAccumulator -= advanceDivisor;
      }
      return over;
    }
};

So, using that algorithm, we improve the spatial and temporal resolution of slow movements. If the stepping frequency is below MIN_FREQUENCY, we just artificially increase the Step ISR call speed, and then account for that increase by changing the "rate" parameter of this new Bresenham algorithm, Easy, and it works beautifully

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 3, 2018

Attached my test code, so you can try it @thinkyhead
Bresenham.zip

@thinkyhead
Copy link
Member

we already have an option to specify the minimum pulse duration, and … the compiler properly figures out the maximum and minimum stepper frequency allowed for a given driver

Toshiba stepper controllers can't handle as high a rate as others. 10kHz is their absolute limit. Other drivers are okay up to 40kHz. Is this limit for Toshiba drivers still enforced by some means?

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 3, 2018

Could you pass me the exact model of the TOSHIBA drivers, so i can peek their datasheet?

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 3, 2018

According to this scerpt, the ARM boards were limiting it to 96kHz, and on AVR to 10khz, so i thought this was more a sw limitation rather than a hw limitation ... I'd try to handle such limitation on a different way, by limiting at the planner level (because otherwise, forcing a hard limit at the stepper will cause an speed discontinuity and probably lost steps)

/**	
 * MAX_STEP_FREQUENCY differs for TOSHIBA	
 */	
#if ENABLED(CONFIG_STEPPERS_TOSHIBA)	
  #ifdef CPU_32_BIT	
    #define MAX_STEP_FREQUENCY STEP_DOUBLER_FREQUENCY // Max step frequency for Toshiba Stepper Controllers, 96kHz is close to maximum for an Arduino Due	
  #else	
    #define MAX_STEP_FREQUENCY 10000 // Max step frequency for Toshiba Stepper Controllers	
  #endif	
#else	
  #ifdef CPU_32_BIT	
    #define MAX_STEP_FREQUENCY (STEP_DOUBLER_FREQUENCY * 4) // Max step frequency for the Due is approx. 330kHz	
  #else	
    #define MAX_STEP_FREQUENCY 40000 // Max step frequency for Ultimaker (5000 pps / half step)	
  #endif	
#endif

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 3, 2018

@thinkyhead : If Toshiba drivers have a hw limitation, it should be handled by proper DEFAULT_MAX_FEEDRATE ... Not by artificially capping the Stepper ISR.
If the limitation is a sw problem, well, things have changed quite a bit ... ;)

@thinkyhead
Copy link
Member

I came late to the Toshiba party. Check the 1.0.x code for earliest and most authoritative references to Toshiba drivers.

@thinkyhead
Copy link
Member

thinkyhead commented Jun 3, 2018

I'd rather that this PR was split up into 3 or more PRs rather than all these changes being glommed together. So, I just spent the last several hours splitting up these changes into a few commits, roughly by category and cleaning them up. Not the best use of my time (yawn!) but someone had to organize this. 😛

So, regarding the MIXING_EXTRUDER let's make sure the mixing factors are being handled the same with these changes. When I implemented the feature, it made more sense to increase the ceiling for each of the mixing channels rather than reduce the addends of the mix steps. That preserves the stepping accuracy better, especially for short moves. If that approach is being preserved, that would be great.

@thinkyhead
Copy link
Member

Let's keep PULSE_TIMER_NUM and PULSE_TIMER_PRESCALE separate from STEP_TIMER_NUM and STEPPER_TIMER_PRESCALE, even though they are currently the same. Keeping them separate in the first place is totally intentional.

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 3, 2018

After some thinking, i found that:

  • Mixing extruder should not be using Bresenham: The problem is in short moves, exactly as you mention. Bresenham has no rounding errors, but the calculations to determine the number of steps has, and quite a bit, if movements are short.
    We should use just fixed point and accumulate the "error" between segments. That way even short moves will not have any error at all.
  • The other problem is the dual/quad stepping per isr limits: The idea that doubling the number of step pulses per ISR will work as a "magic" means to increase rate is not true:
    Lets assume each step pulse takes "n" uS , The ISR will take to execute n * multiplier + fixedoverhead uS to execute. So, the only part that can be saved is the fixed overhead part of the ISR.
    That makes the decision point to start using doubling/quadding a not so trivial thing to determine...
  • Regarding the PULSE_* vs STEP_* .. no problem with that, except that AVR must use the Step timer for pulse measurement, not the Temperature timer, as it was done before...

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 3, 2018

Regarding the Toshiba drivers, the situation is much worse than you think:

According to the datasheet for TB6560, page 6:

  • Clock frequency (this is the STEP pulse frequency): 15kHz maximum
  • And on page 7:
    Minimum pulse clock width: 30uS.
  • Page 11, note 1:
    The recommended clock duty cycle is 50%

(And all those values depend on the external components (Oscillator capacitor) being used.

The TB6600 is a "little" bit better:

  • Clock frequency (this is the STEP pulse frequency): 200kHz maximum (page 26)
  • Minimum pulse clock width: 2.2uS. (page 27)
  • The recommended clock duty cycle is 50%

So, if you do the math, you will see that:

 1 / (minClk * 2) = maxFreq.

We don't need special handling for this driver. We need to properly estimate maximum frequency for any driver. And the answer by actual limiting of the maximum feedrate, so the planner has a chance to properly do its job and keep junction deviation/jerk and acceleration under control.

I will revisit the approach that is being used to calculate the doubling/quad stepping and also revisit the MAX_FEEDRATE approach. That is the proper way to handle it.

And if the user wants to use those drivers, it is obvious they NEED to configure the proper pulse width (as it is right now, in the configuration of printer that is using those drivers, the pulse width is set to 2uS, so the default configuration DOES NOT work

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 3, 2018

(Sorry @thinkyhead . I changed the AVR timer used to ensure minimum pulse width to the Stepper. You reverted that change, and on the first try on a bare AVR board, it resulted on a crash (due to the explanation i did). We need to use the same timer as the stepper here, or literally, Marlin crashes as soon as it tries to execute the first pulse

@teemuatlut
Copy link
Member

@ejtagle Do you happen to remember/know the typical step pulse width on AVR? I'm comparing to TMC2130 data sheet and if we should recommend an additional setting for people to use.

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 3, 2018

@teemuatlut : I am redoing those calculations right now. And this PR will add clarification and automatic computing.

According to the measurements that i have just done, in AVR the minimum possible pulse width is 2.75uS in AVR. The bresenhan tracing uses that minimum amount of time PER EXTRUDER.
On ARM it is a completely different thing... ;)

@teemuatlut
Copy link
Member

Okey thanks, that's good. If I understood the information from the data sheet correctly, on TMC2130 the min pulse width would be 100ns and max step frequency around 400kHz. This is based on 12.65MHz internal clock and 1/16th microstepping.

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 3, 2018

@teemuatlut : Yes, according to datasheet, the minimum pulse width is 108nS (tclk + 20ns), assuming a 12mhz clock.
If using 16 microsteps the maximum step rate is 400khz (again, assuming 12mhz clock)

@Sineos
Copy link

Sineos commented Jun 3, 2018

Wouldn't it make sense to add a drivers.h containing the relevant parameters according to the respective data sheets? This would allow adding future driver generations quite easily.

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 3, 2018

@Sineos : It would be a good idea 👍

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 4, 2018

Right now, i am thinking on the mixing extruder "problem" ... That is the only thing i am not happy about this PR... The way i implemented it obviously is losing resolution. The way that was originally implemented was better, but not ideal, as in short moves, due to rounding, there was also problems... And added to that, the complication of LA... Still thinking how to solve the puzzle...

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 4, 2018

-Still not happy with Linear advance and mixing extruders...

@MagoKimbra
Copy link
Contributor

MagoKimbra commented Jun 5, 2018

Hi I tried on a delta with a 32 bit alligator card. With this new Bresenham I have this result:
Perfect Z movement.
Perfect X Y movement.
XYZ movement is like going at triple speed, it trembles everything and loses steps.
Proved by replacing all the Bresenham with the old one, but leaving the management of the steps with the new system of MAX STEP FREQUENCY, everything works perfectly.
I think the problem is in the section delta_error[X_AXIS] = delta_error[Y_AXIS] = delta_error[Z_AXIS] = delta_error[E_AXIS] = -int32_t (step_event_count); and next.
There's something wrong with me in that section ...
If you tell me what proof I can do, since you come before me to the solution ...

Excuse me but you have shifted 1 bit left this way you multiplied by 2 not divided by 2 or am I wrong?

Nothing is not even that.

I make the home of the delta and then I give the command G1 X10 Y0 Z50 F1000 goes down at an incredible speed vibrating throughout.
If I do G1 X0 Y0 Z50 F1000 goes down perfectly.
I can not figure out where the problem is ...

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 5, 2018

That is the error term. I tried the "new" bresenham extensively (there is a test program at the 2nd comment here) if you want to play with it.
The old bresenham algorithm does not account for smoothing...

To debug this problem you are having, what i'd do is just to DUMP the values of the movement block, and inspect them. Maybe there is a problem with the maximum delta accelerations that was uncovered by this PR ?

In planner.h, I replaced the get_next_block function with this:

The Code…
    /**
     * The current block. NULL if the buffer is empty.
     * This also marks the block as busy.
     * WARNING: Called from Stepper ISR context!
     */
    static block_t* get_current_block() {

      // Get the number of moves in the planner queue so far
      uint8_t nr_moves = movesplanned();

      // If there are any moves queued ...
      if (nr_moves) {

        // If there is still delay of delivery of blocks running, decrement it
        if (delay_before_delivering) {
          --delay_before_delivering;
          // If the number of movements queued is less than 3, and there is still time
          //  to wait, do not deliver anything
          if (nr_moves < 3 && delay_before_delivering) return NULL;
            delay_before_delivering = 0;
          }

        // If we are here, there is no excuse to deliver the block
        block_t * const block = &block_buffer[block_buffer_tail];

#if 1
        SERIAL_ECHOLNPAIR("get_current_block: Tail:", block_buffer_tail);
        SERIAL_ECHOLNPAIR("Head:", block_buffer_head);

        SERIAL_ECHOLNPAIR("\nflag:", block->flag);
        SERIAL_ECHOLNPAIR("extrude:", block->active_extruder);
        SERIAL_ECHOLNPAIR("step_event_count:", block->step_event_count);
        SERIAL_ECHOLNPAIR("accelerate_until:", block->accelerate_until);
        SERIAL_ECHOLNPAIR("decelerate_after:", block->decelerate_after);
  #if ENABLED(S_CURVE_CONTROL)
        SERIAL_ECHOLNPAIR("cruise_rate:", block->cruise_rate);
        SERIAL_ECHOLNPAIR("acceleration_time:", block->acceleration_time);
        SERIAL_ECHOLNPAIR("deceleration_time:", block->deceleration_time);
        SERIAL_ECHOLNPAIR("acceleration_time_inverse:", block->acceleration_time_inverse);
        SERIAL_ECHOLNPAIR("deceleration_time_inverse:", block->deceleration_time_inverse);
  #else
        SERIAL_ECHOLNPAIR("acceleration_rate:", block->acceleration_rate);
  #endif
        SERIAL_ECHOLNPAIR("direction_bits:", block->direction_bits);

        SERIAL_ECHOLNPAIR("nominal_speed_sqr:", block->nominal_speed_sqr);
        SERIAL_ECHOLNPAIR("entry_speed_sqr:", block->entry_speed_sqr);
        SERIAL_ECHOLNPAIR("max_entry_speed_sqr:", block->max_entry_speed_sqr);
        SERIAL_ECHOLNPAIR("millimeters:", block->millimeters);
        SERIAL_ECHOLNPAIR("acceleration:", block->acceleration);
        SERIAL_ECHOLNPAIR("nominal_rate:", block->nominal_rate);
        SERIAL_ECHOLNPAIR("initial_rate:", block->initial_rate);
        SERIAL_ECHOLNPAIR("final_rate:", block->final_rate);
        SERIAL_ECHOLNPAIR("acceleration_steps_per_s2:", block->acceleration_steps_per_s2);
        SERIAL_ECHOLNPAIR("segment_time_us:", block->acceleration_steps_per_s2);

  #if ENABLED(LIN_ADVANCE)
        SERIAL_ECHOLNPAIR("use_advance_lead:", block->use_advance_lead);
        SERIAL_ECHOLNPAIR("advance_speed:", block->advance_speed);
        SERIAL_ECHOLNPAIR("max_adv_steps:", block->max_adv_steps);
        SERIAL_ECHOLNPAIR("final_adv_steps:", block->final_adv_steps);
        SERIAL_ECHOLNPAIR("e_D_ratio:", block->e_D_ratio);
  #endif

        SERIAL_ECHOLN("\n");
#endif
				
        // No trapezoid calculated? Don't execute yet.
        if ( TEST(block->flag, BLOCK_BIT_RECALCULATE)) {
					return NULL;
				}

        #if ENABLED(ULTRA_LCD)
          block_buffer_runtime_us -= block->segment_time_us; // We can't be sure how long an active block will take, so don't count it.
        #endif

        // Mark the block as busy, so the planner does not attempt to replan it
        SBI(block->flag, BLOCK_BIT_BUSY);
        return block;
      }
      else {
        // The queue became empty
        #if ENABLED(ULTRA_LCD)
          clear_block_buffer_runtime(); // paranoia. Buffer is empty now - so reset accumulated time to zero.
        #endif
        return NULL;
      }
    }

Then do a home and get the block values -

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 5, 2018

@MagoKimbra : If you disable ADAPTIVE_STEP_SMOOTHING , does everything start working normally? ... Maybe there is an overflow in the loop at Stepper::stepper_block_phase_isr()

      uint8_t oversampling = 0; // Assume we won't use it
      #if ENABLED(ADAPTIVE_STEP_SMOOTHING)
        // At this point, we must decide if we can use Stepper movement axis smoothing.
        uint32_t max_rate = current_block->nominal_rate;  // Get the maximum rate (maximum event speed)
        while (max_rate < (MIN_STEP_ISR_FREQUENCY)) {
          max_rate <<= 1;
          if (max_rate >= (MAX_1X_STEP_ISR_FREQUENCY)) break;
          ++oversampling;
        }
        oversampling_factor = oversampling;
      #endif

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 5, 2018

(also, we found a problem in the serial communication ... I am working on a fix, let's hope it's not the cause of this problem you are seeing)

@MagoKimbra
Copy link
Contributor

MagoKimbra commented Jun 5, 2018

Everything is disabled, I always do the tests with the special functions disabled.

But the problem is not in the home only when it goes down also moving of x y.

Done `G28` and this is the report… ``` 20:49:07.280 : get_current_block: Tail:11 20:49:07.280 : Head:12 20:49:07.280 : flag:2 20:49:07.280 : extrude:0 20:49:07.280 : step_event_count:34400 20:49:07.280 : accelerate_until:39 20:49:07.280 : decelerate_after:34362 20:49:07.280 : cruise_rate:4619 20:49:07.280 : acceleration_time:681841 20:49:07.296 : deceleration_time:681841 20:49:07.296 : acceleration_time_inverse:6299 20:49:07.296 : deceleration_time_inverse:6299 20:49:07.296 : direction_bits:0 20:49:07.296 : nominal_speed_sqr:2500.000488 20:49:07.296 : entry_speed_sqr:0.000000 20:49:07.296 : max_entry_speed_sqr:0.000000 20:49:07.296 : millimeters:372.390930 20:49:07.296 : acceleration:3000.009277 20:49:07.296 : nominal_rate:4619 20:49:07.296 : initial_rate:120 20:49:07.296 : final_rate:120 20:49:07.296 : acceleration_steps_per_s2:277129 20:49:07.296 : segment_time_us:277129 20:49:08.848 : get_current_block: Tail:12 20:49:08.848 : Head:13 20:49:08.848 : flag:2 20:49:08.848 : extrude:0 20:49:08.848 : step_event_count:49200 20:49:08.848 : accelerate_until:67 20:49:08.848 : decelerate_after:49134 20:49:08.848 : cruise_rate:8000 20:49:08.848 : acceleration_time:689500 20:49:08.848 : deceleration_time:689500 20:49:08.848 : acceleration_time_inverse:6229 20:49:08.848 : deceleration_time_inverse:6229 20:49:08.848 : direction_bits:0 20:49:08.864 : nominal_speed_sqr:2500.000000 20:49:08.864 : entry_speed_sqr:0.000000 20:49:08.864 : max_entry_speed_sqr:0.000000 20:49:08.864 : millimeters:307.500000 20:49:08.864 : acceleration:3000.000000 20:49:08.864 : nominal_rate:8000 20:49:08.864 : initial_rate:120 20:49:08.864 : final_rate:120 20:49:08.864 : acceleration_steps_per_s2:480000 20:49:08.864 : segment_time_us:480000 20:49:08.960 : get_current_block: Tail:13 20:49:08.960 : Head:14 20:49:08.960 : flag:2 20:49:08.960 : extrude:0 20:49:08.960 : step_event_count:1600 20:49:08.960 : accelerate_until:67 20:49:08.960 : decelerate_after:1534 20:49:08.960 : cruise_rate:8000 20:49:08.960 : acceleration_time:689500 20:49:08.976 : deceleration_time:689500 20:49:08.976 : acceleration_time_inverse:6229 20:49:08.976 : deceleration_time_inverse:6229 20:49:08.976 : direction_bits:1 20:49:08.976 : nominal_speed_sqr:2500.000000 20:49:08.976 : entry_speed_sqr:0.000000 20:49:08.976 : max_entry_speed_sqr:0.000000 20:49:08.976 : millimeters:10.000000 20:49:08.976 : acceleration:3000.000000 20:49:08.976 : nominal_rate:8000 20:49:08.976 : initial_rate:120 20:49:08.976 : final_rate:120 20:49:08.976 : acceleration_steps_per_s2:480000 20:49:08.976 : segment_time_us:480000 20:49:09.280 : get_current_block: Tail:14 20:49:09.280 : Head:15 20:49:09.280 : flag:2 20:49:09.280 : extrude:0 20:49:09.280 : step_event_count:3200 20:49:09.280 : accelerate_until:17 20:49:09.280 : decelerate_after:3184 20:49:09.280 : cruise_rate:4000 20:49:09.280 : acceleration_time:339500 20:49:09.280 : deceleration_time:339500 20:49:09.296 : acceleration_time_inverse:12650 20:49:09.296 : deceleration_time_inverse:12650 20:49:09.296 : direction_bits:0 20:49:09.296 : nominal_speed_sqr:625.000000 20:49:09.296 : entry_speed_sqr:0.000000 20:49:09.296 : max_entry_speed_sqr:0.000000 20:49:09.296 : millimeters:20.000000 20:49:09.296 : acceleration:3000.000000 20:49:09.296 : nominal_rate:4000 20:49:09.296 : initial_rate:120 20:49:09.296 : final_rate:120 20:49:09.296 : acceleration_steps_per_s2:480000 20:49:09.296 : segment_time_us:480000 20:49:09.792 : get_current_block: Tail:15 20:49:09.792 : Head:16 20:49:09.792 : flag:2 20:49:09.792 : extrude:0 20:49:09.793 : step_event_count:49200 20:49:09.793 : accelerate_until:67 20:49:09.793 : decelerate_after:49134 20:49:09.808 : cruise_rate:8000 20:49:09.808 : acceleration_time:689500 20:49:09.808 : deceleration_time:689500 20:49:09.809 : acceleration_time_inverse:6229 20:49:09.809 : deceleration_time_inverse:6229 20:49:09.809 : direction_bits:0 20:49:09.809 : nominal_speed_sqr:2500.000000 20:49:09.809 : entry_speed_sqr:0.000000 20:49:09.809 : max_entry_speed_sqr:0.000000 20:49:09.809 : millimeters:307.500000 20:49:09.809 : acceleration:3000.000000 20:49:09.809 : nominal_rate:8000 20:49:09.809 : initial_rate:120 20:49:09.809 : final_rate:120 20:49:09.809 : acceleration_steps_per_s2:480000 20:49:09.809 : segment_time_us:480000 20:49:09.904 : get_current_block: Tail:16 20:49:09.921 : Head:17 20:49:09.921 : flag:2 20:49:09.921 : extrude:0 20:49:09.921 : step_event_count:1600 20:49:09.921 : accelerate_until:67 20:49:09.921 : decelerate_after:1534 20:49:09.921 : cruise_rate:8000 20:49:09.921 : acceleration_time:689500 20:49:09.921 : deceleration_time:689500 20:49:09.921 : acceleration_time_inverse:6229 20:49:09.921 : deceleration_time_inverse:6229 20:49:09.921 : direction_bits:2 20:49:09.921 : nominal_speed_sqr:2500.000000 20:49:09.921 : entry_speed_sqr:0.000000 20:49:09.921 : max_entry_speed_sqr:0.000000 20:49:09.921 : millimeters:10.000000 20:49:09.922 : acceleration:3000.000000 20:49:09.922 : nominal_rate:8000 20:49:09.936 : initial_rate:120 20:49:09.936 : final_rate:120 20:49:09.936 : acceleration_steps_per_s2:480000 20:49:09.936 : segment_time_us:480000 20:49:10.224 : get_current_block: Tail:17 20:49:10.224 : Head:18 20:49:10.224 : flag:2 20:49:10.224 : extrude:0 20:49:10.240 : step_event_count:3200 20:49:10.240 : accelerate_until:17 20:49:10.240 : decelerate_after:3184 20:49:10.240 : cruise_rate:4000 20:49:10.240 : acceleration_time:339500 20:49:10.240 : deceleration_time:339500 20:49:10.240 : acceleration_time_inverse:12650 20:49:10.240 : deceleration_time_inverse:12650 20:49:10.240 : direction_bits:0 20:49:10.240 : nominal_speed_sqr:625.000000 20:49:10.240 : entry_speed_sqr:0.000000 20:49:10.240 : max_entry_speed_sqr:0.000000 20:49:10.241 : millimeters:20.000000 20:49:10.241 : acceleration:3000.000000 20:49:10.241 : nominal_rate:4000 20:49:10.241 : initial_rate:120 20:49:10.241 : final_rate:120 20:49:10.256 : acceleration_steps_per_s2:480000 20:49:10.256 : segment_time_us:480000 20:49:10.752 : get_current_block: Tail:18 20:49:10.752 : Head:19 20:49:10.752 : flag:2 20:49:10.752 : extrude:0 20:49:10.752 : step_event_count:49200 20:49:10.752 : accelerate_until:67 20:49:10.752 : decelerate_after:49134 20:49:10.752 : cruise_rate:8000 20:49:10.752 : acceleration_time:689500 20:49:10.752 : deceleration_time:689500 20:49:10.752 : acceleration_time_inverse:6229 20:49:10.753 : deceleration_time_inverse:6229 20:49:10.753 : direction_bits:0 20:49:10.753 : nominal_speed_sqr:2500.000000 20:49:10.753 : entry_speed_sqr:0.000000 20:49:10.753 : max_entry_speed_sqr:0.000000 20:49:10.753 : millimeters:307.500000 20:49:10.768 : acceleration:3000.000000 20:49:10.768 : nominal_rate:8000 20:49:10.768 : initial_rate:120 20:49:10.768 : final_rate:120 20:49:10.768 : acceleration_steps_per_s2:480000 20:49:10.768 : segment_time_us:480000 20:49:10.864 : get_current_block: Tail:19 20:49:10.864 : Head:20 20:49:10.864 : flag:2 20:49:10.864 : extrude:0 20:49:10.864 : step_event_count:1600 20:49:10.864 : accelerate_until:67 20:49:10.864 : decelerate_after:1534 20:49:10.864 : cruise_rate:8000 20:49:10.864 : acceleration_time:689500 20:49:10.864 : deceleration_time:689500 20:49:10.864 : acceleration_time_inverse:6229 20:49:10.864 : deceleration_time_inverse:6229 20:49:10.864 : direction_bits:4 20:49:10.880 : nominal_speed_sqr:2500.000000 20:49:10.880 : entry_speed_sqr:0.000000 20:49:10.880 : max_entry_speed_sqr:0.000000 20:49:10.880 : millimeters:10.000000 20:49:10.880 : acceleration:3000.000000 20:49:10.880 : nominal_rate:8000 20:49:10.880 : initial_rate:120 20:49:10.880 : final_rate:120 20:49:10.880 : acceleration_steps_per_s2:480000 20:49:10.880 : segment_time_us:480000 20:49:11.184 : get_current_block: Tail:20 20:49:11.184 : Head:21 20:49:11.184 : flag:2 20:49:11.185 : extrude:0 20:49:11.185 : step_event_count:3200 20:49:11.185 : accelerate_until:17 20:49:11.185 : decelerate_after:3184 20:49:11.185 : cruise_rate:4000 20:49:11.185 : acceleration_time:339500 20:49:11.185 : deceleration_time:339500 20:49:11.185 : acceleration_time_inverse:12650 20:49:11.185 : deceleration_time_inverse:12650 20:49:11.185 : direction_bits:0 20:49:11.185 : nominal_speed_sqr:625.000000 20:49:11.185 : entry_speed_sqr:0.000000 20:49:11.200 : max_entry_speed_sqr:0.000000 20:49:11.200 : millimeters:20.000000 20:49:11.200 : acceleration:3000.000000 20:49:11.201 : nominal_rate:4000 20:49:11.201 : initial_rate:120 20:49:11.201 : final_rate:120 20:49:11.201 : acceleration_steps_per_s2:480000 20:49:11.201 : segment_time_us:480000 20:49:11.696 : get_current_block: Tail:21 20:49:11.697 : Head:22 20:49:11.697 : flag:2 20:49:11.697 : extrude:0 20:49:11.697 : step_event_count:6700 20:49:11.697 : accelerate_until:154 20:49:11.700 : decelerate_after:6547 20:49:11.700 : cruise_rate:9238 20:49:11.700 : acceleration_time:1381869 20:49:11.700 : deceleration_time:1381869 20:49:11.700 : acceleration_time_inverse:3108 20:49:11.701 : deceleration_time_inverse:3108 20:49:11.712 : direction_bits:7 20:49:11.712 : nominal_speed_sqr:10000.000000 20:49:11.712 : entry_speed_sqr:0.000000 20:49:11.712 : max_entry_speed_sqr:0.000000 20:49:11.712 : millimeters:72.529624 20:49:11.712 : acceleration:3000.009277 20:49:11.712 : nominal_rate:9238 20:49:11.712 : initial_rate:120 20:49:11.712 : final_rate:120 20:49:11.712 : acceleration_steps_per_s2:277129 20:49:11.712 : segment_time_us:277129 ```

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 6, 2018

Well @MagoKimbra : Let's do the analysis of your log:

Detailed Output…
#1 -- All axes up.
20:49:07.280 : step_event_count:34400
20:49:07.280 : accelerate_until:39
20:49:07.280 : decelerate_after:34362
20:49:07.280 : cruise_rate:4619
20:49:07.280 : acceleration_time:681841
20:49:07.296 : deceleration_time:681841
20:49:07.296 : acceleration_time_inverse:6299
20:49:07.296 : deceleration_time_inverse:6299
20:49:07.296 : direction_bits:0
20:49:07.296 : nominal_speed_sqr:2500.000488
20:49:07.296 : entry_speed_sqr:0.000000
20:49:07.296 : max_entry_speed_sqr:0.000000
20:49:07.296 : millimeters:372.390930
20:49:07.296 : acceleration:3000.009277
20:49:07.296 : nominal_rate:4619
20:49:07.296 : initial_rate:120
20:49:07.296 : final_rate:120
20:49:07.296 : acceleration_steps_per_s2:277129

#2 -- All axes up
20:49:08.848 : step_event_count:49200
20:49:08.848 : accelerate_until:67
20:49:08.848 : decelerate_after:49134
20:49:08.848 : cruise_rate:8000
20:49:08.848 : acceleration_time:689500
20:49:08.848 : deceleration_time:689500
20:49:08.848 : acceleration_time_inverse:6229
20:49:08.848 : deceleration_time_inverse:6229
20:49:08.848 : direction_bits:0
20:49:08.864 : nominal_speed_sqr:2500.000000
20:49:08.864 : entry_speed_sqr:0.000000
20:49:08.864 : max_entry_speed_sqr:0.000000
20:49:08.864 : millimeters:307.500000
20:49:08.864 : acceleration:3000.000000
20:49:08.864 : nominal_rate:8000
20:49:08.864 : initial_rate:120
20:49:08.864 : final_rate:120
20:49:08.864 : acceleration_steps_per_s2:480000

#3 - Axis X down
20:49:08.960 : step_event_count:1600
20:49:08.960 : accelerate_until:67
20:49:08.960 : decelerate_after:1534
20:49:08.960 : cruise_rate:8000
20:49:08.960 : acceleration_time:689500
20:49:08.976 : deceleration_time:689500
20:49:08.976 : acceleration_time_inverse:6229
20:49:08.976 : deceleration_time_inverse:6229
20:49:08.976 : direction_bits:1
20:49:08.976 : nominal_speed_sqr:2500.000000
20:49:08.976 : entry_speed_sqr:0.000000
20:49:08.976 : max_entry_speed_sqr:0.000000
20:49:08.976 : millimeters:10.000000
20:49:08.976 : acceleration:3000.000000
20:49:08.976 : nominal_rate:8000
20:49:08.976 : initial_rate:120
20:49:08.976 : final_rate:120
20:49:08.976 : acceleration_steps_per_s2:480000

#4 - Axis X up
20:49:09.280 : step_event_count:3200
20:49:09.280 : accelerate_until:17
20:49:09.280 : decelerate_after:3184
20:49:09.280 : cruise_rate:4000
20:49:09.280 : acceleration_time:339500
20:49:09.280 : deceleration_time:339500
20:49:09.296 : acceleration_time_inverse:12650
20:49:09.296 : deceleration_time_inverse:12650
20:49:09.296 : direction_bits:0
20:49:09.296 : nominal_speed_sqr:625.000000
20:49:09.296 : entry_speed_sqr:0.000000
20:49:09.296 : max_entry_speed_sqr:0.000000
20:49:09.296 : millimeters:20.000000
20:49:09.296 : acceleration:3000.000000
20:49:09.296 : nominal_rate:4000
20:49:09.296 : initial_rate:120
20:49:09.296 : final_rate:120
20:49:09.296 : acceleration_steps_per_s2:480000

#5 - Axis X up
20:49:09.793 : step_event_count:49200
20:49:09.793 : accelerate_until:67
20:49:09.793 : decelerate_after:49134
20:49:09.808 : cruise_rate:8000
20:49:09.808 : acceleration_time:689500
20:49:09.808 : deceleration_time:689500
20:49:09.809 : acceleration_time_inverse:6229
20:49:09.809 : deceleration_time_inverse:6229
20:49:09.809 : direction_bits:0
20:49:09.809 : nominal_speed_sqr:2500.000000
20:49:09.809 : entry_speed_sqr:0.000000
20:49:09.809 : max_entry_speed_sqr:0.000000
20:49:09.809 : millimeters:307.500000
20:49:09.809 : acceleration:3000.000000
20:49:09.809 : nominal_rate:8000
20:49:09.809 : initial_rate:120
20:49:09.809 : final_rate:120
20:49:09.809 : acceleration_steps_per_s2:480000

#6 - Axis Y down
20:49:09.921 : step_event_count:1600
20:49:09.921 : accelerate_until:67
20:49:09.921 : decelerate_after:1534
20:49:09.921 : cruise_rate:8000
20:49:09.921 : acceleration_time:689500
20:49:09.921 : deceleration_time:689500
20:49:09.921 : acceleration_time_inverse:6229
20:49:09.921 : deceleration_time_inverse:6229
20:49:09.921 : direction_bits:2
20:49:09.921 : nominal_speed_sqr:2500.000000
20:49:09.921 : entry_speed_sqr:0.000000
20:49:09.921 : max_entry_speed_sqr:0.000000
20:49:09.921 : millimeters:10.000000
20:49:09.922 : acceleration:3000.000000
20:49:09.922 : nominal_rate:8000
20:49:09.936 : initial_rate:120
20:49:09.936 : final_rate:120
20:49:09.936 : acceleration_steps_per_s2:480000

#7 - Axis Y up
20:49:10.240 : step_event_count:3200
20:49:10.240 : accelerate_until:17
20:49:10.240 : decelerate_after:3184
20:49:10.240 : cruise_rate:4000
20:49:10.240 : acceleration_time:339500
20:49:10.240 : deceleration_time:339500
20:49:10.240 : acceleration_time_inverse:12650
20:49:10.240 : deceleration_time_inverse:12650
20:49:10.240 : direction_bits:0
20:49:10.240 : nominal_speed_sqr:625.000000
20:49:10.240 : entry_speed_sqr:0.000000
20:49:10.240 : max_entry_speed_sqr:0.000000
20:49:10.241 : millimeters:20.000000
20:49:10.241 : acceleration:3000.000000
20:49:10.241 : nominal_rate:4000
20:49:10.241 : initial_rate:120
20:49:10.241 : final_rate:120
20:49:10.256 : acceleration_steps_per_s2:480000

#8 - Axis Y up
20:49:10.752 : step_event_count:49200
20:49:10.752 : accelerate_until:67
20:49:10.752 : decelerate_after:49134
20:49:10.752 : cruise_rate:8000
20:49:10.752 : acceleration_time:689500
20:49:10.752 : deceleration_time:689500
20:49:10.752 : acceleration_time_inverse:6229
20:49:10.753 : deceleration_time_inverse:6229
20:49:10.753 : direction_bits:0
20:49:10.753 : nominal_speed_sqr:2500.000000
20:49:10.753 : entry_speed_sqr:0.000000
20:49:10.753 : max_entry_speed_sqr:0.000000
20:49:10.753 : millimeters:307.500000
20:49:10.768 : acceleration:3000.000000
20:49:10.768 : nominal_rate:8000
20:49:10.768 : initial_rate:120
20:49:10.768 : final_rate:120
20:49:10.768 : acceleration_steps_per_s2:480000

#9 - Axis Z down
20:49:10.864 : step_event_count:1600
20:49:10.864 : accelerate_until:67
20:49:10.864 : decelerate_after:1534
20:49:10.864 : cruise_rate:8000
20:49:10.864 : acceleration_time:689500
20:49:10.864 : deceleration_time:689500
20:49:10.864 : acceleration_time_inverse:6229
20:49:10.864 : deceleration_time_inverse:6229
20:49:10.864 : direction_bits:4
20:49:10.880 : nominal_speed_sqr:2500.000000
20:49:10.880 : entry_speed_sqr:0.000000
20:49:10.880 : max_entry_speed_sqr:0.000000
20:49:10.880 : millimeters:10.000000
20:49:10.880 : acceleration:3000.000000
20:49:10.880 : nominal_rate:8000
20:49:10.880 : initial_rate:120
20:49:10.880 : final_rate:120
20:49:10.880 : acceleration_steps_per_s2:480000

#10 - Axis Z up
20:49:11.185 : step_event_count:3200
20:49:11.185 : accelerate_until:17
20:49:11.185 : decelerate_after:3184
20:49:11.185 : cruise_rate:4000
20:49:11.185 : acceleration_time:339500
20:49:11.185 : deceleration_time:339500
20:49:11.185 : acceleration_time_inverse:12650
20:49:11.185 : deceleration_time_inverse:12650
20:49:11.185 : direction_bits:0
20:49:11.185 : nominal_speed_sqr:625.000000
20:49:11.185 : entry_speed_sqr:0.000000
20:49:11.200 : max_entry_speed_sqr:0.000000
20:49:11.200 : millimeters:20.000000
20:49:11.200 : acceleration:3000.000000
20:49:11.201 : nominal_rate:4000
20:49:11.201 : initial_rate:120
20:49:11.201 : final_rate:120
20:49:11.201 : acceleration_steps_per_s2:480000

#11 - All axis down
20:49:11.697 : step_event_count:6700
20:49:11.697 : accelerate_until:154
20:49:11.700 : decelerate_after:6547
20:49:11.700 : cruise_rate:9238
20:49:11.700 : acceleration_time:1381869
20:49:11.700 : deceleration_time:1381869
20:49:11.700 : acceleration_time_inverse:3108
20:49:11.701 : deceleration_time_inverse:3108
20:49:11.712 : direction_bits:7
20:49:11.712 : nominal_speed_sqr:10000.000000
20:49:11.712 : entry_speed_sqr:0.000000
20:49:11.712 : max_entry_speed_sqr:0.000000
20:49:11.712 : millimeters:72.529624
20:49:11.712 : acceleration:3000.009277
20:49:11.712 : nominal_rate:9238
20:49:11.712 : initial_rate:120
20:49:11.712 : final_rate:120
20:49:11.712 : acceleration_steps_per_s2:277129

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 6, 2018

The acceleration [acceleration] in all blocks is exactly the same: 3000 mm/s^2
The nominal rate [nominal rate] is the speed in steps/s the movement should be done. Some blocks are using 4000 steps/second, and some of them are using 8000 steps/second.
The nominal speed squared [nominal_speed_sqr] is the movement speed in mm/s squared.
Some movements are using 50mm/s, and some moves 25mm/s, and the last movement (all axis down) is using 10cm/s of speed...
All movements start from 0 mm/s speed [entry_speed_sqr]

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 6, 2018

BTW @MagoKimbra : I did test this PR on a cartesian printer based on an Arduino Due 32bit. Did a full print with no issues at all. Maybe this could be related to the Delta printer...

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 10, 2018

Let´s hope this starts fixing the layer shifts... Some of the fixes, in fact, are handling discontinuities in step timing, so they could make a real difference...

@thinkyhead
Copy link
Member

thinkyhead commented Jun 10, 2018

Let's hope so.

Does that include the case where ADAPTIVE_STEP_SMOOTHING is disabled?
Because we should probably disable this new option by default.

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 10, 2018

Yes, the bugfixes applies to both enabled and disabled states for ADAPTIVE_STEP_SMOOTHING.
The adaptive step smoothing algorithm just twiddles bresenham coefficients, it is not a significant change

The ADAPTIVE_STEP_SMOOTHING should be disabled, but i can´t remember if i did so in the config*.h files...

@thinkyhead
Copy link
Member

It's all set now! I will have this ported for bugfix-1.1.x soon…

@thinkyhead
Copy link
Member

I'm not sure I want to merge these changes just yet if it's possible they might break MIXING_EXTRUDER. There's very little chance that code will get tested very much, if at all, for several weeks.

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 10, 2018

There were no changes to the MIXING_EXTRUDER, unless LINEAR_ADVANCE is enabled. But, previously, LINEAR_ADVANCE+MIXING_EXTRUDER were not working at all.
You can verify what i say: Look at planner.cpp:Planner::_populate_block

And also, Stepper::stepper_pulse_phase_isr

In fact, there was a bug in the MIXING_EXTRUDER implementation: Stepper::set_directions() was never setting the directions of the mixing extruder motors, so essentially it was impossible to retract the filaments from the mixing extruder (and it fact, it would advance, instead of retracting them!)

Interestingly enough, i have a mixing extruder, but i have to install it into the printer. Everything is done except installing the extruder itself. Even the steppers are there... But before any kind of improvement to MIXING_EXTRUDER, i must make sure regular extruders work!

@thinkyhead
Copy link
Member

there was a bug in the MIXING_EXTRUDER implementation

Hmm… I thought I had gotten a report and then fixed that recently. Must've gotten lost in the shuffle.

@thinkyhead
Copy link
Member

Do you know why the MakerParts configs have additional changes with this PR?

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 10, 2018

Yes, the MakerParts config was changed a little. It is actually the printer I have (but i have replaced the controller board with an Arduino Due and modified a Ramps to make it compatible with it..
I try to keep that configuration up to date, but, i have no problems reverting most of the changes. It was mostly cosmetic ones. (After all, after the hw modifications i did, i still have a custom config for my specific mods)

@thinkyhead
Copy link
Member

Is the Z acceleration of 10 better than 100?

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 10, 2018

No, the problem with the Makerparts that i have is the MAXIMUM_FEEDRATE for the Z axis. There seems to be a problem there, and right now i suspect it is a hardware problem. If I set the MAXIMUM_FEEDRATE for that axis above 1, then the Z axis motors, instead of turning, hummm ... Yes, they vibrate but do not move.

I still have to hook the digital oscilloscope to see what is going on with them. I suspect that the main cause could be that I am powering the drivers from 12v, and Marlin has to produce 4000 pulses per mm... Essentially that gives no time for stepper coils to energize...

I know it is NOT a software bug. I have analyzed this too much...

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 10, 2018

Well, forget what i said. Using this PR, with the latest changes, it started working -- Initially...

Well, at some point it stops working. I suspect lack of torque at such high steprate. It is undoubtly a hw problem. I will probably have to do the measurements, after all... 👎

@tig33r
Copy link

tig33r commented Jun 10, 2018

I have one suggestion as noob user. Can you add also MAXIMUM_STEPPER_RATE and MINIMUM_STEPPER_PULSE for LV8729 stepstick? I think it is quiet popular device in reprap family like a4988 or tmc.

@thinkyhead
Copy link
Member

Its max rate is 2MHz. But with MINIMUM_STEPPER_PULSE of 1 the fastest possible rate would be 1MHz. (Currently MINIMUM_STEPPER_PULSE can't be set to 0.5.)

@MagoKimbra
Copy link
Contributor

MagoKimbra commented Jun 10, 2018

Two question.
1: The maximum stepper rate if minumum stepper pulse = 0
#define MAXIMUM_STEPPER_RATE (1000000UL / (2UL * (MINIMUM_STEPPER_PULSE)))
In theory it is the same infinite..
Maybe if MINIMUM_STEPPER_PULSE == 0 the MAXIMUM_STEPPER_RATE = 500000.

2: The ISR_S_CURVE_CYCLES not used.
maybe must insert in #define MIN_ISR_LOOP_CYCLES..

@Sineos
Copy link

Sineos commented Jun 10, 2018

Acc to https://www.mouser.com/ds/2/308/LV8729V-D-278452.pdf the driver has:
Max Clock: 130 kHz
Min Pulse Time: 500 ns

Wouldn't this result in max 1 MHz?

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 10, 2018

@Sineos : There are several drivers with those kinds of specifications. The TMC2xxx series has a minimum step pulse width of 108nS (that would give 5Mhz stepping rate) but the maximum step frequency is way less than that: 400Khz.
I don´t know the exact internal implementation, but there are several possible explanations to that:

  1. As the motor windings are current-controlled, and the chopper inside the driver needs at least one cycle of its chopping clock to control the current, then going faster than the internal chopping clock does not make sense, as simply you cannot control the winding currents faster than that.
  2. 2nd explanation is that the step pin is latched asynchronously (with an small filter) and used at the at the internal chopper clock transition: I suspect that to be the case with TMC2xxx drivers.

In any case, you end up having a minimum pulse width and a minimum separation between pulses.

I will add the specification for the LV8729 as a comment 👍

@MagoKimbra : You are absolutely right. I´ll do the modification

@thinkyhead : At some point i have considered to express the minimum pulse width in nanoseconds. For AVR it does not make sense (the minimum pulse width is 2uS because that is the time the bresenham code takes to execute. On ARM, the minimum time seems to be 16 cycles, Assuming 4 motors, that gives a minimum pulse time of 640nS. But perhaps there could be some value in changing the minimum pulse width and express it in nanoseconds ? - I think rounding up to the next uS is safe, and does not incur a terrible decrease in performance (and once you start going beyond 1mhz, all cabling between Arduino and stepsticks starts to be more and more critical...)

Copy link
Member

@thinkyhead thinkyhead Jun 10, 2018

Choose a reason for hiding this comment

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

Note that ((MINIMUM_STEPPER_PULSE) == 0) won't work if MINIMUM_STEPPER_PULSE isn't defined. It will evaluate as (() == 0). (This isn't an #if statement.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thinkyhead : What do you suggest to protect against MINIMUM_STEPPER_PULSE not defined ? --- I´d either force define it to 2 (will work for most cases), or force a compilation failure.

But i have no idea on the proper solution...

Copy link
Member

Choose a reason for hiding this comment

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

For this kind of case we either supply a default that sets it to 0 when undefined, or we add a check everywhere it's used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setting to 0 would be fine, failing the compilation would also be fine.
All the example configurations are defining it to 2 or more

Your choice 👍 ... I can do the changes, or if you like, you can do them. I have no problem 👍

ejtagle added 2 commits June 10, 2018 16:02
- Stepper bugs fixed
- Support MIXING_EXTRUDER with Linear Advance
- Miscellaneous cleanup
The timing value should be properly set for ALL boards. The compiler will check and set maximum step rate calculations based on those values.
@thinkyhead thinkyhead merged commit 4b90cd8 into MarlinFirmware:bugfix-2.0.x Jun 10, 2018
@thinkyhead thinkyhead changed the title [2.0.x] Adaptive multiaxis step smoothing, and tons of fixes [2.0.x] Adaptive multi-axis step smoothing, some fixes Jun 10, 2018
@thinkyhead
Copy link
Member

Postmortem:

  • Submit smaller PRs with incremental changes. It takes extra time for the maintainers to break up large PRs, and delays the merge of the simple and obvious bug-fixing portions.
  • Don't modify and submit your bugfix-2.0.x branch as a PR. Make a copy with a unique name and work with that instead. This will make it possible to work on multiple features concurrently.
  • Use shorter commit messages and follow the guidelines on how to phrase them. It takes extra time for maintainers to rewrite commit messages.

Looking forward to your next contribution!

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 10, 2018

This was not done on purpose, @thinkyhead ... Bugs were found while trying to implement AMASS.
If there were no bugs, life would be much easier - The usual reasons for not splitting is that those bugs impact the new features i am trying to implement, so, you end up in a situation (if splitting them into separate PRs) when you don´t know if the new feature works, or doesn´t...

Of course I ALSO prefer smaller PRs. We all know, you and I, that the stepper required this facelift. The planner already had it. The stepper was reordered, but no facelift was done.

This PR did exactly that, so I hope next PRs will be much, much smaller! 👍

I am on the Serial Port issue... Non atomicity of 8bit accesses and some undocumented AVR hardware bugs... But i think we have nailed the issue and the proper fix! 👍

@chamnit
Copy link

chamnit commented Jun 13, 2018

@ejtagle : Glad to see this concept getting installed into Marlin. Just curious what you mean by the problems with the Grbl implementation. Particularly the "round by one error" statement in the first post.

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 13, 2018

The scaling of the Bresenham coefficients on Grbl is using the >> operator. leading to discarding of the lsbit...

@chamnit
Copy link

chamnit commented Jun 13, 2018

@ejtagle : Nope. If you follow the code, it never does a >> divide below the original step count for the block. The values are premultiplied (or << multiplied) by the max AMASS level. It's always guaranteed to be even and never loses a LSB.

@ejtagle
Copy link
Contributor Author

ejtagle commented Jun 13, 2018

In that case, everything is fine 👍

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.

9 participants