Skip to content

[2.0.x] Fix stepper/planner race condition, Stepper pulse timer #11081

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thinkyhead merged 3 commits into from
Jun 22, 2018
Merged

[2.0.x] Fix stepper/planner race condition, Stepper pulse timer #11081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b5b139
Planner: Removal of some race conditions between Stepper ISR and Plan…
ejtagle Jun 21, 2018
cf7f80e
Fixing timing on stepper pulses adding a compensation for the non nul…
ejtagle Jun 22, 2018
2594cff
Update planner.cpp
thinkyhead Jun 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 33 additions & 7 deletions Marlin/src/module/planner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,11 @@ void Planner::calculate_trapezoid_for_block(block_t* const block, const float &e
const bool was_enabled = STEPPER_ISR_ENABLED();
if (was_enabled) DISABLE_STEPPER_DRIVER_INTERRUPT();

// Don't update variables if block is busy: It is being interpreted by the planner
// Don't update variables if block is busy; it is being interpreted by the planner.
// If this happens, there's a problem... The block speed is inconsistent. Some values
// have already been updated, but the Stepper ISR is already using the block. Fortunately,
// the values being used by the Stepper ISR weren't touched, so just stop here...
// TODO: There may be a way to update a running block, depending on the stepper ISR position.
if (!TEST(block->flag, BLOCK_BIT_BUSY)) {
block->accelerate_until = accelerate_steps;
block->decelerate_after = accelerate_steps + plateau_steps;
Expand Down Expand Up @@ -862,10 +866,13 @@ void Planner::reverse_pass_kernel(block_t* const current, const block_t * const
? max_entry_speed_sqr
: MIN(max_entry_speed_sqr, max_allowable_speed_sqr(-current->acceleration, next ? next->entry_speed_sqr : sq(MINIMUM_PLANNER_SPEED), current->millimeters));
if (current->entry_speed_sqr != new_entry_speed_sqr) {
current->entry_speed_sqr = new_entry_speed_sqr;

// Need to recalculate the block speed
// Need to recalculate the block speed - Mark it now, so the stepper
// ISR does not consume the block before being recalculated
SBI(current->flag, BLOCK_BIT_RECALCULATE);

// Set the new entry speed
current->entry_speed_sqr = new_entry_speed_sqr;
}
}
}
Expand Down Expand Up @@ -925,14 +932,15 @@ void Planner::forward_pass_kernel(const block_t* const previous, block_t* const
// If true, current block is full-acceleration and we can move the planned pointer forward.
if (new_entry_speed_sqr < current->entry_speed_sqr) {

// Mark we need to recompute the trapezoidal shape, and do it now,
// so the stepper ISR does not consume the block before being recalculated
SBI(current->flag, BLOCK_BIT_RECALCULATE);

// Always <= max_entry_speed_sqr. Backward pass sets this.
current->entry_speed_sqr = new_entry_speed_sqr; // Always <= max_entry_speed_sqr. Backward pass sets this.

// Set optimal plan pointer.
block_buffer_planned = block_index;

// And mark we need to recompute the trapezoidal shape
SBI(current->flag, BLOCK_BIT_RECALCULATE);
}
}

Expand Down Expand Up @@ -1019,6 +1027,12 @@ void Planner::recalculate_trapezoids() {
if (current) {
// Recalculate if current block entry or exit junction speed has changed.
if (TEST(current->flag, BLOCK_BIT_RECALCULATE) || TEST(next->flag, BLOCK_BIT_RECALCULATE)) {

// Mark the current block as RECALCULATE, to protect it from the Stepper ISR running it.
// Note that due to the above condition, there's a chance the current block isn't marked as
// RECALCULATE yet, but the next one is. That's the reason for the following line.
SBI(current->flag, BLOCK_BIT_RECALCULATE);

// NOTE: Entry and exit factors always > 0 by all previous logic operations.
const float current_nominal_speed = SQRT(current->nominal_speed_sqr),
nomr = 1.0 / current_nominal_speed;
Expand All @@ -1030,7 +1044,10 @@ void Planner::recalculate_trapezoids() {
current->final_adv_steps = next_entry_speed * comp;
}
#endif
CBI(current->flag, BLOCK_BIT_RECALCULATE); // Reset current only to ensure next trapezoid is computed

// Reset current only to ensure next trapezoid is computed - The
// stepper is free to use the block from now on.
CBI(current->flag, BLOCK_BIT_RECALCULATE);
}
}

Expand All @@ -1043,6 +1060,12 @@ void Planner::recalculate_trapezoids() {

// Last/newest block in buffer. Exit speed is set with MINIMUM_PLANNER_SPEED. Always recalculated.
if (next) {

// Mark the next(last) block as RECALCULATE, to prevent the Stepper ISR running it.
// As the last block is always recalculated here, there is a chance the block isn't
// marked as RECALCULATE yet. That's the reason for the following line.
SBI(next->flag, BLOCK_BIT_RECALCULATE);

const float next_nominal_speed = SQRT(next->nominal_speed_sqr),
nomr = 1.0 / next_nominal_speed;
calculate_trapezoid_for_block(next, next_entry_speed * nomr, (MINIMUM_PLANNER_SPEED) * nomr);
Expand All @@ -1053,6 +1076,9 @@ void Planner::recalculate_trapezoids() {
next->final_adv_steps = (MINIMUM_PLANNER_SPEED) * comp;
}
#endif

// Reset next only to ensure its trapezoid is computed - The stepper is free to use
// the block from now on.
CBI(next->flag, BLOCK_BIT_RECALCULATE);
}
}
Expand Down
52 changes: 38 additions & 14 deletions Marlin/src/module/stepper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
//

#ifndef MINIMUM_STEPPER_PULSE
#define MINIMUM_STEPPER_PULSE 0
#define MINIMUM_STEPPER_PULSE 0UL
#endif

#ifndef MAXIMUM_STEPPER_RATE
Expand Down Expand Up @@ -86,7 +86,10 @@
// Stepper Loop base cycles
#define ISR_LOOP_BASE_CYCLES 4UL

// And each stepper takes 16 cycles
// To start the step pulse, in the worst case takes
#define ISR_START_STEPPER_CYCLES 13UL

// And each stepper (start + stop pulse) takes in worst case
#define ISR_STEPPER_CYCLES 16UL

#else
Expand All @@ -111,51 +114,72 @@
// Stepper Loop base cycles
#define ISR_LOOP_BASE_CYCLES 32UL

// And each stepper takes 88 cycles
// To start the step pulse, in the worst case takes
#define ISR_START_STEPPER_CYCLES 57UL

// And each stepper (start + stop pulse) takes in worst case
#define ISR_STEPPER_CYCLES 88UL

#endif

// Add time for each stepper
#ifdef HAS_X_STEP
#define ISR_X_STEPPER_CYCLES ISR_STEPPER_CYCLES
#define ISR_START_X_STEPPER_CYCLES ISR_START_STEPPER_CYCLES
#define ISR_X_STEPPER_CYCLES ISR_STEPPER_CYCLES
#else
#define ISR_X_STEPPER_CYCLES 0UL
#define ISR_START_X_STEPPER_CYCLES 0UL
#define ISR_X_STEPPER_CYCLES 0UL
#endif
#ifdef HAS_Y_STEP
#define ISR_Y_STEPPER_CYCLES ISR_STEPPER_CYCLES
#define ISR_START_Y_STEPPER_CYCLES ISR_START_STEPPER_CYCLES
#define ISR_Y_STEPPER_CYCLES ISR_STEPPER_CYCLES
#else
#define ISR_Y_STEPPER_CYCLES 0UL
#define ISR_START_Y_STEPPER_CYCLES 0UL
#define ISR_Y_STEPPER_CYCLES 0UL
#endif
#ifdef HAS_Z_STEP
#define ISR_Z_STEPPER_CYCLES ISR_STEPPER_CYCLES
#define ISR_START_Z_STEPPER_CYCLES ISR_START_STEPPER_CYCLES
#define ISR_Z_STEPPER_CYCLES ISR_STEPPER_CYCLES
#else
#define ISR_Z_STEPPER_CYCLES 0UL
#define ISR_START_Z_STEPPER_CYCLES 0UL
#define ISR_Z_STEPPER_CYCLES 0UL
#endif

// E is always interpolated, even for mixing extruders
#define ISR_E_STEPPER_CYCLES ISR_STEPPER_CYCLES
#define ISR_START_E_STEPPER_CYCLES ISR_START_STEPPER_CYCLES
#define ISR_E_STEPPER_CYCLES ISR_STEPPER_CYCLES

// If linear advance is disabled, then the loop also handles them
#if DISABLED(LIN_ADVANCE) && ENABLED(MIXING_EXTRUDER)
#define ISR_START_MIXING_STEPPER_CYCLES ((MIXING_STEPPERS) * (ISR_START_STEPPER_CYCLES))
#define ISR_MIXING_STEPPER_CYCLES ((MIXING_STEPPERS) * (ISR_STEPPER_CYCLES))
#else
#define ISR_START_MIXING_STEPPER_CYCLES 0UL
#define ISR_MIXING_STEPPER_CYCLES 0UL
#endif

// Calculate the minimum time to start all stepper pulses in the ISR loop
#define MIN_ISR_START_LOOP_CYCLES (ISR_START_X_STEPPER_CYCLES + ISR_START_Y_STEPPER_CYCLES + ISR_START_Z_STEPPER_CYCLES + ISR_START_E_STEPPER_CYCLES + ISR_START_MIXING_STEPPER_CYCLES)

// And the total minimum loop time, not including the base
#define MIN_ISR_LOOP_CYCLES (ISR_X_STEPPER_CYCLES + ISR_Y_STEPPER_CYCLES + ISR_Z_STEPPER_CYCLES + ISR_E_STEPPER_CYCLES + ISR_MIXING_STEPPER_CYCLES)

// Calculate the minimum MPU cycles needed per pulse to enforce, limited to the max stepper rate
#define _MIN_STEPPER_PULSE_CYCLES(N) MAX((F_CPU) / (MAXIMUM_STEPPER_RATE), ((F_CPU) / 500000UL) * (N))
#if MINIMUM_STEPPER_PULSE
#define MIN_STEPPER_PULSE_CYCLES _MIN_STEPPER_PULSE_CYCLES(MINIMUM_STEPPER_PULSE)
#define MIN_STEPPER_PULSE_CYCLES _MIN_STEPPER_PULSE_CYCLES((MINIMUM_STEPPER_PULSE))
#else
#define MIN_STEPPER_PULSE_CYCLES _MIN_STEPPER_PULSE_CYCLES(1)
#define MIN_STEPPER_PULSE_CYCLES _MIN_STEPPER_PULSE_CYCLES(1UL)
#endif

#define MIN_PULSE_TICKS ((PULSE_TIMER_TICKS_PER_US) * (MINIMUM_STEPPER_PULSE))
#define ADDED_STEP_TICKS ((MIN_STEPPER_PULSE_CYCLES) / (PULSE_TIMER_PRESCALE) - MIN_PULSE_TICKS)
// Calculate the minimum ticks of the PULSE timer that must elapse with the step pulse enabled
// adding the "start stepper pulse" code section execution cycles to account for that not all
// pulses start at the beginning of the loop, so an extra time must be added to compensate so
// the last generated pulse (usually the extruder stepper) has the right length
#define MIN_PULSE_TICKS (((PULSE_TIMER_TICKS_PER_US) * (MINIMUM_STEPPER_PULSE)) + ((MIN_ISR_START_LOOP_CYCLES) / (PULSE_TIMER_PRESCALE)))

// Calculate the extra ticks of the PULSE timer between step pulses
#define ADDED_STEP_TICKS (((MIN_STEPPER_PULSE_CYCLES) / (PULSE_TIMER_PRESCALE)) - (MIN_PULSE_TICKS))

// But the user could be enforcing a minimum time, so the loop time is
#define ISR_LOOP_CYCLES (ISR_LOOP_BASE_CYCLES + MAX(MIN_STEPPER_PULSE_CYCLES, MIN_ISR_LOOP_CYCLES))
Expand Down