From 233c74491e30786dab51919df1056938cfb58ff6 Mon Sep 17 00:00:00 2001 From: Jason Smith Date: Thu, 19 Dec 2019 02:09:37 -0800 Subject: [PATCH 1/3] Fix rounding issue computing timer ticks from ns. --- Marlin/src/module/stepper.cpp | 11 +++++++++-- Marlin/src/module/stepper.h | 30 ++++++++++++++++++++---------- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/Marlin/src/module/stepper.cpp b/Marlin/src/module/stepper.cpp index 9babb137075e..f5ab4a304dfa 100644 --- a/Marlin/src/module/stepper.cpp +++ b/Marlin/src/module/stepper.cpp @@ -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))) @@ -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 @@ -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) diff --git a/Marlin/src/module/stepper.h b/Marlin/src/module/stepper.h index b3749d94d303..85ede8d72af4 100644 --- a/Marlin/src/module/stepper.h +++ b/Marlin/src/module/stepper.h @@ -56,7 +56,25 @@ // 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 + /* + * Cycles to perform actions in START_TIMED_PULSE + * + * This was measured on an LPC1768 using a GPIO output surrounding + * START_TIMED_PULSE, with the resulting timespan converted to cycles. + * This value is incorrect for other 32-bit processors. This is ok, as + * long as other processors takes longer, so pulses will grow instead + * of shrink. As an example, an SKR Pro running an stm32f407zgt6 + * processor actually requires approximately 60 cyles. + */ #define TIMER_READ_ADD_AND_STORE_CYCLES 34UL // The base ISR takes 792 cycles @@ -86,6 +104,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 @@ -157,17 +176,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 @@ -178,11 +193,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)) From fe17d0e39898eb8b35cadf0f1097b2bcac2ccf58 Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Thu, 19 Dec 2019 16:06:16 -0600 Subject: [PATCH 2/3] Parens for safety --- Marlin/src/module/stepper.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Marlin/src/module/stepper.cpp b/Marlin/src/module/stepper.cpp index f5ab4a304dfa..51c4e98244fc 100644 --- a/Marlin/src/module/stepper.cpp +++ b/Marlin/src/module/stepper.cpp @@ -340,10 +340,10 @@ xyze_int8_t Stepper::count_direction{0}; #endif #define CYCLES_TO_NS(CYC) (1000UL * (CYC) / ((F_CPU) / 1000000)) -constexpr uint32_t NS_PER_PULSE_TIMER_TICK = 1000000000UL / STEPPER_TIMER_RATE; +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; } +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)) From 310774b2e86669f1fa7d4f1ac0736e1084f6cf8e Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Thu, 19 Dec 2019 16:12:18 -0600 Subject: [PATCH 3/3] Update stepper.h --- Marlin/src/module/stepper.h | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/Marlin/src/module/stepper.h b/Marlin/src/module/stepper.h index 85ede8d72af4..4a686d6f7c08 100644 --- a/Marlin/src/module/stepper.h +++ b/Marlin/src/module/stepper.h @@ -56,7 +56,7 @@ // 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. @@ -65,15 +65,13 @@ */ #ifdef CPU_32_BIT - /* - * Cycles to perform actions in START_TIMED_PULSE + /** + * Duration of START_TIMED_PULSE * - * This was measured on an LPC1768 using a GPIO output surrounding - * START_TIMED_PULSE, with the resulting timespan converted to cycles. - * This value is incorrect for other 32-bit processors. This is ok, as - * long as other processors takes longer, so pulses will grow instead - * of shrink. As an example, an SKR Pro running an stm32f407zgt6 - * processor actually requires approximately 60 cyles. + * ...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