Skip to content

Step timing cleanup and rounding fix #16258

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
Dec 22, 2019
Merged

Step timing cleanup and rounding fix #16258

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
233c744
Fix rounding issue computing timer ticks from ns.
sjasonsmith Dec 19, 2019
fe17d0e
Parens for safety
thinkyhead Dec 19, 2019
310774b
Update stepper.h
thinkyhead Dec 19, 2019
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
11 changes: 9 additions & 2 deletions Marlin/src/module/stepper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,13 @@ xyze_int8_t Stepper::count_direction{0};
#if DISABLED(MIXING_EXTRUDER)
#define E_APPLY_STEP(v,Q) E_STEP_WRITE(stepper_extruder, v)
#endif

#define CYCLES_TO_NS(CYC) (1000UL * (CYC) / ((F_CPU) / 1000000))
constexpr uint32_t NS_PER_PULSE_TIMER_TICK = 1000000000UL / (STEPPER_TIMER_RATE);

// Round up when converting from ns to timer ticks
constexpr uint32_t NS_TO_PULSE_TIMER_TICKS(uint32_t NS) { return (NS + (NS_PER_PULSE_TIMER_TICK) / 2) / (NS_PER_PULSE_TIMER_TICK); }

#define TIMER_SETUP_NS (CYCLES_TO_NS(TIMER_READ_ADD_AND_STORE_CYCLES))

#define PULSE_HIGH_TICK_COUNT hal_timer_t(NS_TO_PULSE_TIMER_TICKS(_MIN_PULSE_HIGH_NS - _MIN(_MIN_PULSE_HIGH_NS, TIMER_SETUP_NS)))
Expand Down Expand Up @@ -1430,7 +1437,7 @@ void Stepper::stepper_pulse_phase_isr() {
// Take multiple steps per interrupt (For high speed moves)
bool firstStep = true;
xyze_bool_t step_needed{0};
hal_timer_t end_tick_count;
hal_timer_t end_tick_count = 0;

do {
#define _APPLY_STEP(AXIS) AXIS ##_APPLY_STEP
Expand Down Expand Up @@ -1941,7 +1948,7 @@ uint32_t Stepper::stepper_block_phase_isr() {

// Step E stepper if we have steps
bool firstStep = true;
hal_timer_t end_tick_count;
hal_timer_t end_tick_count = 0;

while (LA_steps) {
#if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE) && DISABLED(I2S_STEPPER_STREAM)
Expand Down
28 changes: 18 additions & 10 deletions Marlin/src/module/stepper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,23 @@
// Estimate the amount of time the Stepper ISR will take to execute
//

/**
* The method of calculating these cycle-constants is unclear.
* Most of them are no longer used directly for pulse timing, and exist
* only to estimate a maximum step rate based on the user's configuration.
* As 32-bit processors continue to diverge, maintaining cycle counts
* will become increasingly difficult and error-prone.
*/

#ifdef CPU_32_BIT
/**
* Duration of START_TIMED_PULSE
*
* ...as measured on an LPC1768 with a scope and converted to cycles.
* Not applicable to other 32-bit processors, but as long as others
* take longer, pulses will be longer. For example the SKR Pro
* (stm32f407zgt6) requires ~60 cyles.
*/
#define TIMER_READ_ADD_AND_STORE_CYCLES 34UL

// The base ISR takes 792 cycles
Expand Down Expand Up @@ -86,6 +102,7 @@
#define ISR_STEPPER_CYCLES 16UL

#else
// Cycles to perform actions in START_TIMED_PULSE
#define TIMER_READ_ADD_AND_STORE_CYCLES 13UL

// The base ISR takes 752 cycles
Expand Down Expand Up @@ -157,17 +174,13 @@
#define MIN_STEPPER_PULSE_CYCLES _MIN_STEPPER_PULSE_CYCLES(1UL)
#endif

// 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
// Calculate the minimum pulse times (high and low)
#if MINIMUM_STEPPER_PULSE && MAXIMUM_STEPPER_RATE
constexpr uint32_t _MIN_STEP_PERIOD_NS = 1000000000UL / MAXIMUM_STEPPER_RATE;
constexpr uint32_t _MIN_PULSE_HIGH_NS = 1000UL * MINIMUM_STEPPER_PULSE;
constexpr uint32_t _MIN_PULSE_LOW_NS = _MAX((_MIN_STEP_PERIOD_NS - _MIN(_MIN_STEP_PERIOD_NS, _MIN_PULSE_HIGH_NS)), _MIN_PULSE_HIGH_NS);
#elif MINIMUM_STEPPER_PULSE
// Assume 50% duty cycle
constexpr uint32_t _MIN_STEP_PERIOD_NS = 1000000000UL / MAXIMUM_STEPPER_RATE;
constexpr uint32_t _MIN_PULSE_HIGH_NS = 1000UL * MINIMUM_STEPPER_PULSE;
constexpr uint32_t _MIN_PULSE_LOW_NS = _MIN_PULSE_HIGH_NS;
#elif MAXIMUM_STEPPER_RATE
Expand All @@ -178,11 +191,6 @@
#error "Expected at least one of MINIMUM_STEPPER_PULSE or MAXIMUM_STEPPER_RATE to be defined"
#endif

// TODO: NS_TO_PULSE_TIMER_TICKS has some rounding issues:
// 1. PULSE_TIMER_TICKS_PER_US rounds to an integer, which loses 20% of the count for a 2.5 MHz pulse tick (such as for LPC1768)
// 2. The math currently rounds down to the closes tick. Perhaps should round up.
constexpr uint32_t NS_TO_PULSE_TIMER_TICKS(uint32_t NS) { return PULSE_TIMER_TICKS_PER_US * (NS) / 1000UL; }
#define CYCLES_TO_NS(CYC) (1000UL * (CYC) / ((F_CPU) / 1000000))

// 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