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 flag optimizations #4433

Open
wants to merge 2 commits into
base: MK3
Choose a base branch
from
Open

🚧 Planner flag optimizations #4433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51f450f
Combine planner busy flag with other flags
leptun Oct 8, 2023
57e7618
Compine planner use_advance_lead with other flags
leptun Oct 8, 2023
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
28 changes: 13 additions & 15 deletions Firmware/planner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit
#ifdef LIN_ADVANCE
uint16_t final_adv_steps = 0;
uint16_t max_adv_steps = 0;
if (block->use_advance_lead) {
if (block->flag & BLOCK_FLAG_USE_ADVANCE_LEAD) {
gudnimg marked this conversation as resolved.
Show resolved Hide resolved
final_adv_steps = final_rate * block->adv_comp;
}
#endif
Expand All @@ -240,7 +240,7 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit
if (accel_decel_steps < block->step_event_count.wide) {
plateau_steps = block->step_event_count.wide - accel_decel_steps;
#ifdef LIN_ADVANCE
if (block->use_advance_lead)
if (block->flag & BLOCK_FLAG_USE_ADVANCE_LEAD)
max_adv_steps = block->nominal_rate * block->adv_comp;
#endif
} else {
Expand Down Expand Up @@ -277,7 +277,7 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit
}

#ifdef LIN_ADVANCE
if (block->use_advance_lead) {
if (block->flag & BLOCK_FLAG_USE_ADVANCE_LEAD) {
if(!accelerate_steps || !decelerate_steps) {
// accelerate_steps=0: deceleration-only ramp, max_rate is effectively unused
// decelerate_steps=0: acceleration-only ramp, max_rate _is_ final_rate
Expand All @@ -295,7 +295,7 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit
// which corresponds to a maximum repeat frequency of 228.57 kHz.
// This blocking is safe in the context of a 10kHz stepper driver interrupt
// or a 115200 Bd serial line receive interrupt, which will not trigger faster than 12kHz.
if (! block->busy) { // Don't update variables if block is busy.
if (!(block->flag & BLOCK_FLAG_BUSY)) { // Don't update variables if block is busy.
block->accelerate_until = accelerate_steps;
block->decelerate_after = accelerate_steps+plateau_steps;
block->initial_rate = initial_rate;
Expand Down Expand Up @@ -726,7 +726,8 @@ void plan_buffer_line(float x, float y, float z, const float &e, float feed_rate
block_t *block = &block_buffer[block_buffer_head];

// Mark block as not busy (Not executed by the stepper interrupt, could be still tinkered with.)
block->busy = false;
// Also reset the block flag.
block->flag = 0;

// Set sdlen for calculating sd position
block->sdlen = 0;
Expand Down Expand Up @@ -1009,7 +1010,7 @@ Having the real displacement of the head, we can calculate the total movement le
{
accel = ceil(cs.retract_acceleration * steps_per_mm); // convert to: acceleration steps/sec^2
#ifdef LIN_ADVANCE
block->use_advance_lead = false;
block->flag &= ~BLOCK_FLAG_USE_ADVANCE_LEAD;
#endif
}
else
Expand All @@ -1024,10 +1025,10 @@ Having the real displacement of the head, we can calculate the total movement le
* delta_mm[E_AXIS] >= 0 : Extruding or traveling, but _not_ retracting.
* |delta_mm[Z_AXIS]| < 0.5 : Z is only moved for leveling (_not_ for priming)
*/
block->use_advance_lead = extruder_advance_K > 0
&& delta_mm[E_AXIS] >= 0
&& fabs(delta_mm[Z_AXIS]) < 0.5;
if (block->use_advance_lead) {
if (extruder_advance_K > 0
&& delta_mm[E_AXIS] >= 0
&& fabs(delta_mm[Z_AXIS]) < 0.5) {
block->flag |= BLOCK_FLAG_USE_ADVANCE_LEAD;
#ifdef LA_FLOWADJ
// M221/FLOW should change uniformly the extrusion thickness
float delta_e = (e - position_float[E_AXIS]) / extruder_multiplier[extruder];
Expand All @@ -1048,7 +1049,7 @@ Having the real displacement of the head, we can calculate the total movement le
// no one will use a retract length of 0mm < retr_length < ~0.2mm and no one will print
// 100mm wide lines using 3mm filament or 35mm wide lines using 1.75mm filament.
if (e_D_ratio > 3.0)
block->use_advance_lead = false;
block->flag &= ~BLOCK_FLAG_USE_ADVANCE_LEAD;
else if (e_D_ratio > 0) {
const uint32_t max_accel_steps_per_s2 = ceil(cs.max_jerk[E_AXIS] / (extruder_advance_K * e_D_ratio) * steps_per_mm);
if (accel > max_accel_steps_per_s2) {
Expand Down Expand Up @@ -1101,9 +1102,6 @@ Having the real displacement of the head, we can calculate the total movement le
}
}

// Reset the block flag.
block->flag = 0;

if (plan_reset_next_e_sched)
{
// finally propagate a pending reset
Expand Down Expand Up @@ -1203,7 +1201,7 @@ Having the real displacement of the head, we can calculate the total movement le
block->speed_factor = block->nominal_rate / block->nominal_speed;

#ifdef LIN_ADVANCE
if (block->use_advance_lead) {
if (block->flag & BLOCK_FLAG_USE_ADVANCE_LEAD) {
// calculate the compression ratio for the segment (the required advance steps are computed
// during trapezoid planning)
float adv_comp = extruder_advance_K * e_D_ratio * cs.axis_steps_per_mm[E_AXIS]; // (step/(mm/s))
Expand Down
8 changes: 5 additions & 3 deletions Firmware/planner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ enum BlockFlag {
BLOCK_FLAG_DDA_LOWRES = 8,
// Block starts with Zeroed E counter
BLOCK_FLAG_E_RESET = 16,
// Block is being executed by the stepper ISR
BLOCK_FLAG_BUSY = 32,
// Whether the block uses LA
BLOCK_FLAG_USE_ADVANCE_LEAD = 64,
};

union dda_isteps_t
Expand Down Expand Up @@ -104,14 +108,12 @@ typedef struct {
uint32_t final_rate; // The minimal rate at exit
uint32_t acceleration_steps_per_s2; // acceleration steps/sec^2
uint8_t fan_speed; // Print fan speed, ranges from 0 to 255
volatile char busy;


// Pre-calculated division for the calculate_trapezoid_for_block() routine to run faster.
float speed_factor;

#ifdef LIN_ADVANCE
bool use_advance_lead; // Whether the current block uses LA
uint16_t advance_rate, // Step-rate for extruder speed
max_adv_steps, // max. advance steps to get cruising speed pressure (not always nominal_speed!)
final_adv_steps; // advance steps due to exit speed
Expand Down Expand Up @@ -233,7 +235,7 @@ FORCE_INLINE block_t *plan_get_current_block()
return(NULL);
}
block_t *block = &block_buffer[block_buffer_tail];
block->busy = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note this is called in the Stepper ISR. Is it safe to drop the volatile keyword?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I worry about race conditions here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think yes. I didn't see any place where volatile was really needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added @wavexx as reviewer, I think he would know best if this is OK.

block->flag |= BLOCK_FLAG_BUSY;
return(block);
}

Expand Down
8 changes: 4 additions & 4 deletions Firmware/stepper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ FORCE_INLINE void stepper_next_block()
acceleration_time = calc_timer(acc_step_rate, step_loops);

#ifdef LIN_ADVANCE
if (current_block->use_advance_lead) {
if (current_block->flag & BLOCK_FLAG_USE_ADVANCE_LEAD) {
target_adv_steps = current_block->max_adv_steps;
}
e_steps = 0;
Expand Down Expand Up @@ -841,7 +841,7 @@ FORCE_INLINE void isr() {
_NEXT_ISR(timer);
acceleration_time += timer;
#ifdef LIN_ADVANCE
if (current_block->use_advance_lead) {
if (current_block->flag & BLOCK_FLAG_USE_ADVANCE_LEAD) {
if (step_events_completed.wide <= (unsigned long int)step_loops) {
la_state = ADV_INIT | ADV_ACC_VARY;
if (e_extruding && current_adv_steps > target_adv_steps)
Expand Down Expand Up @@ -870,7 +870,7 @@ FORCE_INLINE void isr() {
deceleration_time += timer;

#ifdef LIN_ADVANCE
if (current_block->use_advance_lead) {
if (current_block->flag & BLOCK_FLAG_USE_ADVANCE_LEAD) {
if (step_events_completed.wide <= current_block->decelerate_after + step_loops) {
target_adv_steps = current_block->final_adv_steps;
la_state = ADV_INIT | ADV_ACC_VARY;
Expand All @@ -888,7 +888,7 @@ FORCE_INLINE void isr() {
step_loops_nominal = step_loops;

#ifdef LIN_ADVANCE
if(current_block->use_advance_lead) {
if(current_block->flag & BLOCK_FLAG_USE_ADVANCE_LEAD) {
// Due to E-jerk, there can be discontinuities in pressure state where an
// acceleration or deceleration can be skipped or joined with the previous block.
// If LA was not previously active, re-check the pressure level
Expand Down
Loading