From e176df1e6d03cb82a46954e2e2064a9dd4c176e7 Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Thu, 6 Nov 2025 23:01:07 -0600 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=A8=20FT=20Motion=20comments,=20style?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves #28151 --- Marlin/src/module/ft_motion.cpp | 21 ++++++++++++++++----- Marlin/src/module/ft_motion/stepping.cpp | 12 ++++++------ Marlin/src/module/stepper.cpp | 18 +++++++++--------- Marlin/src/module/stepper.h | 2 +- Marlin/src/module/stepper/cycles.h | 4 ++-- 5 files changed, 34 insertions(+), 23 deletions(-) diff --git a/Marlin/src/module/ft_motion.cpp b/Marlin/src/module/ft_motion.cpp index 238c6737df..0a2c9f6c3a 100644 --- a/Marlin/src/module/ft_motion.cpp +++ b/Marlin/src/module/ft_motion.cpp @@ -484,7 +484,7 @@ xyze_float_t FTMotion::calc_traj_point(const float dist) { stepper_plan_t FTMotion::calc_stepper_plan(xyze_float_t traj_coords) { // 1) Convert trajectory to step delta - #define _TOSTEPS_q32(A, B) int64_t(traj_coords.A * planner.settings.axis_steps_per_mm[B] * (1ull << 32)) + #define _TOSTEPS_q32(A, B) int64_t(traj_coords.A * planner.settings.axis_steps_per_mm[B] * (1ULL << 32)) XYZEval next_steps_q32_32 = LOGICAL_AXIS_ARRAY( _TOSTEPS_q32(e, block_extruder_axis), _TOSTEPS_q32(x, X_AXIS), _TOSTEPS_q32(y, Y_AXIS), _TOSTEPS_q32(z, Z_AXIS), @@ -493,7 +493,7 @@ stepper_plan_t FTMotion::calc_stepper_plan(xyze_float_t traj_coords) { ); #undef _TOSTEPS_q32 - constexpr uint32_t ITERATIONS_PER_TRAJ_INV_uq0_32 = (1ull << 32) / ITERATIONS_PER_TRAJ; + constexpr uint32_t ITERATIONS_PER_TRAJ_INV_uq0_32 = (1ULL << 32) / ITERATIONS_PER_TRAJ; stepper_plan_t stepper_plan; #define _RUN_AXIS(A) do{ \ @@ -525,10 +525,21 @@ stepper_plan_t FTMotion::calc_stepper_plan(xyze_float_t traj_coords) { */ void FTMotion::fill_stepper_plan_buffer() { while (!stepper_plan_is_full()) { - float total_duration = currentGenerator->getTotalDuration(); // if the current plan is empty, it will have zero duration. + float total_duration = currentGenerator->getTotalDuration(); // If the current plan is empty, it will have zero duration. while (tau + FTM_TS > total_duration) { - // Previous block plan consumed, try to get the next one. - tau -= total_duration; // The exact end of the last block may be in-between trajectory points, so the next one may start anywhere of (-FTM_TS, 0]. + /** + * We’ve reached the end of the current block. + * + * `tau` is the time that has elapsed inside this block. After a block is finished, the next one may + * start at any point between *just before* the last sampled time (one step earlier, i.e. `-FTM_TS`) + * and *exactly at* the last sampled time (0). IOW the real start of the next block could be anywhere + * in the interval (-FTM_TS, 0]. + * + * To account for that uncertainty we simply subtract the duration of the finished block from `tau`. + * This brings us back to a time value that is valid for the next block, while still allowing the next + * block’s start to be offset by up to one time step into the past. + */ + tau -= total_duration; const bool plan_available = plan_next_block(); if (!plan_available) return; total_duration = currentGenerator->getTotalDuration(); diff --git a/Marlin/src/module/ft_motion/stepping.cpp b/Marlin/src/module/ft_motion/stepping.cpp index 96ef4466d8..f44087a7d4 100644 --- a/Marlin/src/module/ft_motion/stepping.cpp +++ b/Marlin/src/module/ft_motion/stepping.cpp @@ -29,20 +29,20 @@ void Stepping::reset() { stepper_plan.reset(); - delta_error_q32.set(LOGICAL_AXIS_ARRAY_1(_BV32(31))); // start as 0.5 in q32 so steps are rounded + delta_error_q32.set(LOGICAL_AXIS_LIST_1(1 << 31)); // Start as 0.5 in q32 so steps are rounded step_bits = 0; bresenham_iterations_pending = 0; } uint32_t Stepping::advance_until_step() { xyze_ulong_t space_q32 = -delta_error_q32 + UINT32_MAX; // How much accumulation until a step in any axis is ALMOST due - // scalar in the right hand because types.h is missing scalar on left cases + // TODO: Add operators to types.h for scalar destination. xyze_ulong_t advance_q32 = stepper_plan.advance_dividend_q0_32; uint32_t iterations = bresenham_iterations_pending; // Per-axis lower-bound approx of floor(space_q32/adv), min across axes (lower bound because this fast division underestimates result by up to 1) - // #define RUN_AXIS(A) if(advance_q32.A > 0) NOMORE(iterations, space_q32.A/advance_q32.A); - #define RUN_AXIS(A) if(advance_q32.A > 0) NOMORE(iterations, uint32_t((uint64_t(space_q32.A) * advance_dividend_reciprocal.A) >> 32)); + //#define RUN_AXIS(A) if (advance_q32.A > 0) NOMORE(iterations, space_q32.A / advance_q32.A); + #define RUN_AXIS(A) if (advance_q32.A > 0) NOMORE(iterations, uint32_t((uint64_t(space_q32.A) * advance_dividend_reciprocal.A) >> 32)); LOGICAL_AXIS_MAP(RUN_AXIS); #undef RUN_AXIS @@ -72,7 +72,7 @@ uint32_t Stepping::plan() { if (bresenham_iterations_pending > 0) { intervals = advance_until_step(); if (bool(step_bits)) return intervals; // steps to make => return the wait time so it gets done in due time - // Else all bresenham iterations were advanced without steps => this is just the frame end, so plan the next one directly and accumulate the wait + // Else all Bresenham iterations were advanced without steps => this is just the frame end, so plan the next one directly and accumulate the wait } if (ftMotion.stepper_plan_is_empty()) { @@ -97,7 +97,7 @@ uint32_t Stepping::plan() { return INTERVAL_PER_TRAJ_POINT; } - // This vector division is unavoidable, but it saves a division per step during bresenham + // This vector division is unavoidable, but it saves a division per step during Bresenham // The reciprocal is actually 2^32/dividend, but that requires dividing a uint64_t, which quite expensive // Since even the real reciprocal may underestimate the quotient by 1 anyway already, this optimisation doesn't // make things worse. This underestimation is compensated for in advance_until_step. diff --git a/Marlin/src/module/stepper.cpp b/Marlin/src/module/stepper.cpp index be8cb766e6..873d1383d0 100644 --- a/Marlin/src/module/stepper.cpp +++ b/Marlin/src/module/stepper.cpp @@ -272,7 +272,7 @@ uint32_t Stepper::advance_divisor = 0, hal_timer_t Stepper::ticks_nominal = 0; #if DISABLED(S_CURVE_ACCELERATION) - uint32_t Stepper::acc_step_rate; // needed for deceleration start point + uint32_t Stepper::acc_step_rate; // Needed for deceleration start point #endif xyz_long_t Stepper::endstops_trigsteps; @@ -1989,7 +1989,7 @@ void Stepper::pulse_phase_isr() { #if HAS_ROUGH_LIN_ADVANCE if (la_active && step_needed.e) { - // don't actually step here, but do subtract movements steps + // Don't actually step here, but do subtract movements steps // from the linear advance step count step_needed.e = false; la_advance_steps--; @@ -2000,14 +2000,14 @@ void Stepper::pulse_phase_isr() { #endif #if HAS_ZV_SHAPING - // record an echo if a step is needed in the primary bresenham + // Record an echo if a step is needed in the primary Bresenham const bool x_step = TERN0(INPUT_SHAPING_X, step_needed.x && shaping_x.enabled), y_step = TERN0(INPUT_SHAPING_Y, step_needed.y && shaping_y.enabled), z_step = TERN0(INPUT_SHAPING_Z, step_needed.z && shaping_z.enabled); if (x_step || y_step || z_step) ShapingQueue::enqueue(x_step, TERN0(INPUT_SHAPING_X, shaping_x.forward), y_step, TERN0(INPUT_SHAPING_Y, shaping_y.forward), z_step, TERN0(INPUT_SHAPING_Z, shaping_z.forward)); - // do the first part of the secondary bresenham + // Do the first part of the secondary Bresenham #if ENABLED(INPUT_SHAPING_X) if (x_step) PULSE_PREP_SHAPING(X, shaping_x.delta_error, shaping_x.forward ? shaping_x.factor1 : -shaping_x.factor1); @@ -2195,7 +2195,7 @@ hal_timer_t Stepper::calc_timer_interval(uint32_t step_rate) { #else - if (step_rate >= 0x0800) { // higher step rate + if (step_rate >= 0x0800) { // Higher step rate // AVR is able to keep up at around 65kHz Stepping ISR rate at most. // So values for step_rate > 65535 might as well be truncated. // Handle it as quickly as possible. i.e., assume highest byte is zero @@ -2208,7 +2208,7 @@ hal_timer_t Stepper::calc_timer_interval(uint32_t step_rate) { const uint8_t gain = uint8_t(pgm_read_byte(table_address + 2)); return base - MultiU8X8toH8(uint8_t(step_rate & 0x00FF), gain); } - else if (step_rate > minimal_step_rate) { // lower step rates + else if (step_rate > minimal_step_rate) { // Lower step rates step_rate -= minimal_step_rate; // Correct for minimal speed const uintptr_t table_address = uintptr_t(&speed_lookuptable_slow[uint8_t(step_rate >> 3)]); return uint16_t(pgm_read_word(table_address)) @@ -2388,7 +2388,7 @@ hal_timer_t Stepper::block_phase_isr() { ticks_nominal = 0; } #endif - time_spent_in_isr = -time_spent; // unsigned but guaranteed to be +ve when needed + time_spent_in_isr = -time_spent; // Unsigned but guaranteed to be +ve when needed time_spent_out_isr = 0; #endif @@ -3295,7 +3295,7 @@ void Stepper::init() { } void Stepper::set_shaping_frequency(const AxisEnum axis, const float freq) { - // enabling or disabling shaping whilst moving can result in lost steps + // Enabling or disabling shaping whilst moving can result in lost steps planner.synchronize(); const bool was_on = hal.isr_state(); @@ -3373,7 +3373,7 @@ void Stepper::_set_position(const abce_long_t &spos) { ); TERN_(HAS_EXTRUDERS, count_position.e = spos.e); #else - // default non-h-bot planning + // Default non-h-bot planning count_position = spos; #endif diff --git a/Marlin/src/module/stepper.h b/Marlin/src/module/stepper.h index 5d5c000577..dfca23715e 100644 --- a/Marlin/src/module/stepper.h +++ b/Marlin/src/module/stepper.h @@ -275,7 +275,7 @@ constexpr ena_mask_t enable_overlap[] = { float zeta; bool enabled : 1; bool forward : 1; - int16_t delta_error = 0; // delta_error for seconday bresenham mod 128 + int16_t delta_error = 0; // delta_error for secondary Bresenham mod 128 uint8_t factor1; uint8_t factor2; int32_t last_block_end_pos = 0; diff --git a/Marlin/src/module/stepper/cycles.h b/Marlin/src/module/stepper/cycles.h index fa76f60127..2f04342538 100644 --- a/Marlin/src/module/stepper/cycles.h +++ b/Marlin/src/module/stepper/cycles.h @@ -115,7 +115,7 @@ constexpr uint32_t min_isr_loop_cycles = isr_mixing_stepper_cycles + LOGICAL_AXE // Calculate the minimum MPU cycles needed per pulse to enforce constexpr uint32_t min_stepper_pulse_cycles = _min_pulse_high_ns * CYCLES_PER_MICROSECOND / 1000; -// The loop takes the base time plus the time for all the bresenham logic for 1 << R pulses plus the time +// The loop takes the base time plus the time for all the Bresenham logic for 1 << R pulses plus the time // between pulses for ((1 << R) - 1) pulses. But the user could be enforcing a minimum time so the loop time is: constexpr uint32_t isr_loop_cycles(const int R) { return ((isr_loop_base_cycles + min_isr_loop_cycles + min_stepper_pulse_cycles) * ((1UL << R) - 1) + _MAX(min_isr_loop_cycles, min_stepper_pulse_cycles)); } @@ -140,7 +140,7 @@ constexpr uint32_t isr_shaping_loop_cycles(const int R) { // HELP ME: What is what? // Directions are set up for MIXING_STEPPERS - like before. // Finding the right stepper may last up to MIXING_STEPPERS loops in get_next_stepper(). - // These loops are a bit faster than advancing a bresenham counter. + // These loops are a bit faster than advancing a Bresenham counter. // Always only one E stepper is stepped. * TERN1(MIXING_EXTRUDER, (MIXING_STEPPERS)) );