Skip to content
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

Jump to bottom

Planner trapezoidal nominal_rate fix #26881

Merged
merged 33 commits into from
Jul 13, 2024
Merged

Planner trapezoidal nominal_rate fix #26881

Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
f4562be
Fix planner wrong trap generation
HoverClub Mar 17, 2024
a7a4152
Update planner.cpp
HoverClub Mar 17, 2024
0257384
Update planner.h
HoverClub Mar 17, 2024
cdb0b1c
Update planner.h
HoverClub Mar 17, 2024
3b887b4
Update planner.h
HoverClub Mar 17, 2024
0c8c741
Merge branch 'MarlinFirmware:bugfix-2.1.x' into Trap-nominal-fix
HoverClub Apr 7, 2024
5e0158a
Update planner.cpp
HoverClub Apr 7, 2024
3da5d0c
Update planner.h
HoverClub Apr 7, 2024
ddf9681
Merge branch 'bugfix-2.1.x' into pr/26881
thinkyhead May 9, 2024
9f2b99b
Revert "Update planner.cpp"
HoverClub May 11, 2024
22f7de5
Update planner.cpp
HoverClub May 11, 2024
2d46cc3
Update planner.h
HoverClub May 11, 2024
e2ecb03
Update planner.cpp
HoverClub May 11, 2024
8556962
Merge branch 'bugfix-2.1.x' into pr/26881
thinkyhead May 11, 2024
bf5a532
ws
thinkyhead May 11, 2024
2396c0f
Apply to min_step_rate
thinkyhead May 11, 2024
a67be24
misc cosmetics
thinkyhead May 15, 2024
db58fa9
Merge branch 'bugfix-2.1.x' into pr/26881
thinkyhead May 15, 2024
e5cb811
Merge branch 'bugfix-2.1.x' into pr/26881
thinkyhead May 17, 2024
551dfa2
comment
thinkyhead May 17, 2024
7c6daa7
allow merge sooner
thinkyhead May 17, 2024
f2d6cfd
not needed
thinkyhead May 17, 2024
e7d301f
Merge branch 'bugfix-2.1.x' into pr/26881
thinkyhead May 17, 2024
cd6b8c8
use _MAX, remove NOMOREs
thinkyhead May 17, 2024
481369e
tweak
thinkyhead May 17, 2024
a63df47
type happy
thinkyhead May 17, 2024
65af7d2
shorten lines
thinkyhead Jul 1, 2024
fe400a2
hygdmfsc
thinkyhead Jul 1, 2024
83697e9
Merge branch 'bugfix-2.1.x' into pr/26881
thinkyhead Jul 1, 2024
c32f6c6
merge followup
thinkyhead Jul 1, 2024
03ef8d2
keep comment
thinkyhead Jul 1, 2024
02f9dc0
followup
thinkyhead Jul 5, 2024
3b27a8e
Merge branch 'bugfix-2.1.x' into pr/26881
thinkyhead Jul 7, 2024
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
3 changes: 1 addition & 2 deletions Marlin/src/HAL/ESP32/timers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,11 @@ typedef uint64_t hal_timer_t;
#if ENABLED(I2S_STEPPER_STREAM)
#define STEPPER_TIMER_PRESCALE 1
#define STEPPER_TIMER_RATE 250000 // 250khz, 4µs pulses of i2s word clock
#define STEPPER_TIMER_TICKS_PER_US ((STEPPER_TIMER_RATE) / 1000000) // stepper timer ticks per µs // wrong would be 0.25
#else
#define STEPPER_TIMER_PRESCALE 40
#define STEPPER_TIMER_RATE ((HAL_TIMER_RATE) / (STEPPER_TIMER_PRESCALE)) // frequency of stepper timer, 2MHz
#define STEPPER_TIMER_TICKS_PER_US ((STEPPER_TIMER_RATE) / 1000000) // stepper timer ticks per µs
#endif
#define STEPPER_TIMER_TICKS_PER_US ((STEPPER_TIMER_RATE) / 1000000) // stepper timer ticks per µs

#define STEP_TIMER_MIN_INTERVAL 8 // minimum time in µs between stepper interrupts

Expand Down
2 changes: 1 addition & 1 deletion Marlin/src/HAL/TEENSY31_32/timers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ typedef uint32_t hal_timer_t;
#define FTM0_TIMER_PRESCALE_BITS 0b011
#define FTM1_TIMER_PRESCALE_BITS 0b010

#define FTM0_TIMER_RATE (F_BUS / (FTM0_TIMER_PRESCALE)) // 60MHz / 8 = 7500kHz
#define FTM0_TIMER_RATE (F_BUS / (FTM0_TIMER_PRESCALE)) // 60MHz / 8 = 7.5MHz
#define FTM1_TIMER_RATE (F_BUS / (FTM1_TIMER_PRESCALE)) // 60MHz / 4 = 15MHz

#define HAL_TIMER_RATE (FTM0_TIMER_RATE)
Expand Down
12 changes: 3 additions & 9 deletions Marlin/src/module/planner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,8 +729,6 @@ void Planner::init() {
#endif
#endif

#define MINIMAL_STEP_RATE 120

/**
* Get the current block for processing
* and mark the block as busy.
Expand Down Expand Up @@ -796,13 +794,9 @@ void Planner::calculate_trapezoid_for_block(block_t * const block, const_float_t
uint32_t initial_rate = entry_speed ? LROUND(entry_speed * spmm) : block->initial_rate,
final_rate = LROUND(exit_speed * spmm);

// Removing code to constrain values produces judder in direction-switching moves because the
// current discrete stepping math diverges from physical motion under constant acceleration
// when acceleration_steps_per_s2 is large compared to initial/final_rate.
NOLESS(initial_rate, long(MINIMAL_STEP_RATE));
NOLESS(final_rate, long(MINIMAL_STEP_RATE));
NOMORE(initial_rate, block->nominal_rate); // NOTE: The nominal rate may be less than MINIMAL_STEP_RATE!
NOMORE(final_rate, block->nominal_rate);
NOLESS(initial_rate, stepper.minimal_step_rate);
NOLESS(final_rate, stepper.minimal_step_rate);
NOLESS(block->nominal_rate, stepper.minimal_step_rate);

#if ANY(S_CURVE_ACCELERATION, LIN_ADVANCE)
// If we have some plateau time, the cruise rate will be the nominal rate
Expand Down
8 changes: 3 additions & 5 deletions Marlin/src/module/stepper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2201,12 +2201,10 @@ hal_timer_t Stepper::calc_timer_interval(uint32_t step_rate) {
#ifdef CPU_32_BIT

// A fast processor can just do integer division
constexpr uint32_t min_step_rate = uint32_t(STEPPER_TIMER_RATE) / HAL_TIMER_TYPE_MAX;
return step_rate > min_step_rate ? uint32_t(STEPPER_TIMER_RATE) / step_rate : HAL_TIMER_TYPE_MAX;
return step_rate > minimal_step_rate ? uint32_t(STEPPER_TIMER_RATE) / step_rate : HAL_TIMER_TYPE_MAX;
Copy link
Contributor

@mh-dm mh-dm Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I took another look and the step_rate <= minimal_step_rate case is not correct, although it wasn't correct before either.

STEPPER_TIMER_RATE is at 100-500Mhz, divided by some 4/8 prescale. So somewhere around 10-200 million.
However, HAL_TIMER_TYPE_MAX is usually 0xFFFFFFFF or ~4 billion. So if we're at the minimal_step_rate of 1 (usually) we'll be waiting for HAL_TIMER_TYPE_MAX ticks which at 10-200 million rate will take in the range of 400-20 seconds.

Maybe switch to:
return step_rate > minimal_step_rate ? uint32_t(STEPPER_TIMER_RATE) / step_rate : MIN(STEPPER_TIMER_RATE, HAL_TIMER_TYPE_MAX);

Copy link
Member

@thinkyhead thinkyhead Jul 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The choice to use HAL_TIMER_TYPE_MAX to prevent a divide by zero in calc_timer_interval originally comes from #25557. The aim was to use the value closest to infinity when the step_rate was equal to zero.

return step_rate ? uint32_t(STEPPER_TIMER_RATE) / step_rate : HAL_TIMER_TYPE_MAX;

This was updated in #25696 to not just apply to a step_rate of zero, but to any step rate below a minimum threshold.

constexpr uint32_t min_step_rate = uint32_t(STEPPER_TIMER_RATE) / HAL_TIMER_TYPE_MAX;
return step_rate > min_step_rate ? uint32_t(STEPPER_TIMER_RATE) / step_rate : HAL_TIMER_TYPE_MAX;

If the value returned from calc_timer_interval is later used in a divide by STEPPER_TIMER_RATE operation that expects a result >= 1, it makes sense to make that our substitute for infinity, but with this value being 1/8 of HAL_TIMER_TYPE_MAX, it means more likelihood of a divide result of 1 or greater.

There is some code that expects HAL_TIMER_TYPE_MAX to be returned. Specifically, code that looks for LA_ADV_NEVER expects it to be equal to HAL_TIMER_TYPE_MAX, and when this value is returned it essentially represents "never."

So, can this really be done? You and I and @tombrazier will need to look closer at this question.


#else

constexpr uint32_t min_step_rate = (F_CPU) / 500000U; // i.e., 32 or 40
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.
Expand All @@ -2220,8 +2218,8 @@ 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 > min_step_rate) { // lower step rates
step_rate -= min_step_rate; // Correct for minimal speed
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))
- ((uint16_t(pgm_read_word(table_address + 2)) * uint8_t(step_rate & 0x0007)) >> 3);
Expand Down
10 changes: 10 additions & 0 deletions Marlin/src/module/stepper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,16 @@ class Stepper {

public:

// The minimal step rate ensures calculations stay within limits
// and avoid the most unreasonably slow step rates.
static constexpr uint32_t minimal_step_rate = (
#ifdef CPU_32_BIT
_MAX((STEPPER_TIMER_RATE) / HAL_TIMER_TYPE_MAX, 1U) // 32-bit shouldn't go below 1
#else
(F_CPU) / 500000U // AVR shouldn't go below 32 (16MHz) or 40 (20MHz)
#endif
);

#if ANY(HAS_EXTRA_ENDSTOPS, Z_STEPPER_AUTO_ALIGN)
static bool separate_multi_axis;
#endif
Expand Down
Loading