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

Planner trapezoidal nominal_rate fix #26881

Merged
merged 33 commits into from
Jul 13, 2024
Merged

Planner trapezoidal nominal_rate fix #26881

Changes from 8 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
6 changes: 0 additions & 6 deletions Marlin/src/module/planner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,8 +730,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 @@ -797,10 +795,6 @@ void Planner::calculate_trapezoid_for_block(block_t * const block, const_float_t
uint32_t initial_rate = CEIL(block->nominal_rate * entry_factor),
final_rate = CEIL(block->nominal_rate * exit_factor); // (steps per second)

// Limit minimal step rate (Otherwise the timer will overflow.)
NOLESS(initial_rate, uint32_t(MINIMAL_STEP_RATE));
NOLESS(final_rate, uint32_t(MINIMAL_STEP_RATE));

Copy link
Contributor

Choose a reason for hiding this comment

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

@tombrazier in the latest diff MINMAL_STEP_RATE and these limits are removed entirely.

Are you confident that this is a correct thing to do, and that Otherwise the timer will overflow is just an obsolete relic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine we should have some testing on this in its final form before merging it.

Copy link

@GelidusResearch GelidusResearch Apr 7, 2024 •
edited by thinkyhead
Loading

Choose a reason for hiding this comment

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

I believe there is likely a need to limit the minimum with AVR HAL hardware timers e.g. ATM2560 etc.
see https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.1.x/Marlin/src/HAL/AVR/timers.h
Don't think you can remove them entirely.
regards @descipher

#define TEMP_TIMER_FREQUENCY    (((F_CPU) + 0x2000) / 0x4000)

#define STEPPER_TIMER_RATE      HAL_TIMER_RATE
#define STEPPER_TIMER_PRESCALE  8
#define STEPPER_TIMER_TICKS_PER_US ((STEPPER_TIMER_RATE) / 1000000)

#define PULSE_TIMER_RATE         STEPPER_TIMER_RATE
#define PULSE_TIMER_PRESCALE     STEPPER_TIMER_PRESCALE
#define PULSE_TIMER_TICKS_PER_US STEPPER_TIMER_TICKS_PER_US

#define ENABLE_STEPPER_DRIVER_INTERRUPT()  SBI(TIMSK1, OCIE1A)
#define DISABLE_STEPPER_DRIVER_INTERRUPT() CBI(TIMSK1, OCIE1A)
#define STEPPER_ISR_ENABLED()             TEST(TIMSK1, OCIE1A)

#define ENABLE_TEMPERATURE_INTERRUPT()     SBI(TIMSK0, OCIE0A)
#define DISABLE_TEMPERATURE_INTERRUPT()    CBI(TIMSK0, OCIE0A)
#define TEMPERATURE_ISR_ENABLED()         TEST(TIMSK0, OCIE0A)

FORCE_INLINE void HAL_timer_start(const uint8_t timer_num, const uint32_t) {
  switch (timer_num) {
    case MF_TIMER_STEP:
      // waveform generation = 0100 = CTC
      SET_WGM(1, CTC_OCRnA);

      // output mode = 00 (disconnected)
      SET_COMA(1, NORMAL);

      // Set the timer pre-scaler
      // Generally we use a divider of 8, resulting in a 2MHz timer
      // frequency on a 16MHz MCU. If you are going to change this, be
      // sure to regenerate speed_lookuptable.h with
      // create_speed_lookuptable.py
      SET_CS(1, PRESCALER_8);  //  CS 2 = 1/8 prescaler

      // Init Stepper ISR to 122 Hz for quick starting
      // (F_CPU) / (STEPPER_TIMER_PRESCALE) / frequency
      OCR1A = 0x4000;
      TCNT1 = 0;
      break;

    case MF_TIMER_TEMP:
      // Use timer0 for temperature measurement
      // Interleave temperature interrupt with millies interrupt
      OCR0A = 128;
      break;
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@tombrazier in the latest diff MINMAL_STEP_RATE and these limits are removed entirely.

Are you confident that this is a correct thing to do, and that Otherwise the timer will overflow is just an obsolete relic?

Not confident enough to merge it without testing. I am fairly sure it is redundant but it needs to be tried.

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to add some code to the test suite that would run a set of values from very small to very large through routines like this to ensure they don't overflow. Meanwhile, those with questions about a math routine can use the Simulator or make a set of test files — e.g., test.h, test.cpp — containing only the Marlin routines that need checking, plus the supporting types, etc. I have just such a set of files that I've occasionally used to figure out macro behavior and things like that.

With that sage advice out of the way, has anyone explored and tested for overflow/underflow yet?

#if ANY(S_CURVE_ACCELERATION, LIN_ADVANCE)
// If we have some plateau time, the cruise rate will be the nominal rate
uint32_t cruise_rate = block->nominal_rate;
Expand Down
Loading