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

Keep stepcompress move history relative to current time #6439

Merged
merged 6 commits into from
Dec 30, 2023
Merged

Keep stepcompress move history relative to current time #6439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b983e7f
Expire history relative to current time rather than last move in hist…
EiNSTeiN- Dec 27, 2023
8ce5a1a
Make steppersync_history_expire static
EiNSTeiN- Dec 27, 2023
b688b3a
Remove all history rather than keeping it where appropriate
EiNSTeiN- Dec 27, 2023
120fe41
Use print time rather than system time for history expiry
EiNSTeiN- Dec 27, 2023
6dc8d59
Clear trapq history consistently everywhere
EiNSTeiN- Dec 29, 2023
3ebea98
Do not flush trapq history where appropriate
EiNSTeiN- Dec 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions klippy/chelper/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@
void steppersync_free(struct steppersync *ss);
void steppersync_set_time(struct steppersync *ss
, double time_offset, double mcu_freq);
int steppersync_flush(struct steppersync *ss, uint64_t move_clock);
int steppersync_flush(struct steppersync *ss, uint64_t move_clock
, uint64_t clear_history_clock);
"""

defs_itersolve = """
Expand Down Expand Up @@ -94,7 +95,8 @@
, double start_pos_x, double start_pos_y, double start_pos_z
, double axes_r_x, double axes_r_y, double axes_r_z
, double start_v, double cruise_v, double accel);
void trapq_finalize_moves(struct trapq *tq, double print_time);
void trapq_finalize_moves(struct trapq *tq, double print_time
, double clear_history_time);
void trapq_set_position(struct trapq *tq, double print_time
, double pos_x, double pos_y, double pos_z);
int trapq_extract_old(struct trapq *tq, struct pull_move *p, int max
Expand Down
29 changes: 23 additions & 6 deletions klippy/chelper/stepcompress.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ struct step_move {
int16_t add;
};

#define HISTORY_EXPIRE (30.0)

struct history_steps {
struct list_node node;
uint64_t first_clock, last_clock;
Expand Down Expand Up @@ -292,6 +290,13 @@ free_history(struct stepcompress *sc, uint64_t end_clock)
}
}

// Expire the stepcompress history older than the given clock
static void
stepcompress_history_expire(struct stepcompress *sc, uint64_t end_clock)
{
free_history(sc, end_clock);
}

// Free memory associated with a 'stepcompress' object
void __visible
stepcompress_free(struct stepcompress *sc)
Expand Down Expand Up @@ -322,9 +327,6 @@ calc_last_step_print_time(struct stepcompress *sc)
{
double lsc = sc->last_step_clock;
sc->last_step_print_time = sc->mcu_time_offset + (lsc - .5) / sc->mcu_freq;

if (lsc > sc->mcu_freq * HISTORY_EXPIRE)
free_history(sc, lsc - sc->mcu_freq * HISTORY_EXPIRE);
}

// Set the conversion rate of 'print_time' to mcu clock
Expand Down Expand Up @@ -731,6 +733,18 @@ steppersync_set_time(struct steppersync *ss, double time_offset
}
}

// Expire the stepcompress history before the given clock time
static void
steppersync_history_expire(struct steppersync *ss, uint64_t end_clock)
{
int i;
for (i = 0; i < ss->sc_num; i++)
{
struct stepcompress *sc = ss->sc_list[i];
stepcompress_history_expire(sc, end_clock);
}
}

// Implement a binary heap algorithm to track when the next available
// 'struct move' in the mcu will be available
static void
Expand Down Expand Up @@ -758,7 +772,8 @@ heap_replace(struct steppersync *ss, uint64_t req_clock)

// Find and transmit any scheduled steps prior to the given 'move_clock'
int __visible
steppersync_flush(struct steppersync *ss, uint64_t move_clock)
steppersync_flush(struct steppersync *ss, uint64_t move_clock
, uint64_t clear_history_clock)
{
// Flush each stepcompress to the specified move_clock
int i;
Expand Down Expand Up @@ -806,5 +821,7 @@ steppersync_flush(struct steppersync *ss, uint64_t move_clock)
// Transmit commands
if (!list_empty(&msgs))
serialqueue_send_batch(ss->sq, ss->cq, &msgs);

steppersync_history_expire(ss, clear_history_clock);
return 0;
}
3 changes: 2 additions & 1 deletion klippy/chelper/stepcompress.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ struct steppersync *steppersync_alloc(
void steppersync_free(struct steppersync *ss);
void steppersync_set_time(struct steppersync *ss, double time_offset
, double mcu_freq);
int steppersync_flush(struct steppersync *ss, uint64_t move_clock);
int steppersync_flush(struct steppersync *ss, uint64_t move_clock
, uint64_t clear_history_clock);

#endif // stepcompress.h
10 changes: 4 additions & 6 deletions klippy/chelper/trapq.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,10 @@ trapq_append(struct trapq *tq, double print_time
}
}

#define HISTORY_EXPIRE (30.0)

// Expire any moves older than `print_time` from the trapezoid velocity queue
void __visible
trapq_finalize_moves(struct trapq *tq, double print_time)
trapq_finalize_moves(struct trapq *tq, double print_time
, double clear_history_time)
{
struct move *head_sentinel = list_first_entry(&tq->moves, struct move,node);
struct move *tail_sentinel = list_last_entry(&tq->moves, struct move, node);
Expand All @@ -190,10 +189,9 @@ trapq_finalize_moves(struct trapq *tq, double print_time)
if (list_empty(&tq->history))
return;
struct move *latest = list_first_entry(&tq->history, struct move, node);
double expire_time = latest->print_time + latest->move_t - HISTORY_EXPIRE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for the record I think we're now clearing the history in places where we wouldn't have before which is a change in behavior. When print_time is NEVER_TIME (or a large value) all the moves would get transferred to the history, but this code would still only clear 30s prior to the last move because of the math on this line. This is why I had the new parameter as 0 initially (to keep the history until this is called from the toolhead code).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I goofed. You are correct and I misread the original code. I think we do want to avoid a change in behavior here, because this is an area of the code where we really want to be able to use tools like git bisect if necessary.

If I'm reading the code correctly, the call in trapq_finalize_moves() doesn't need to flush the history (it can use 0), and the code in DripModeEndSignal doesn't need to flush (it can also use 0). However, the code in manual_stepper.py and force_move.py do need to fully flush (using the current + 99999.9 method) as nothing else will free the history (and nothing uses the history of these modules).

Does that sound correct?

Thanks.
-Kevin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you mean trapq_set_position and not trapq_finalize_moves that looks correct to me (as in, it preserves the current behavior), however I'm not super familiar with this code. I left the other 2 calls with + 99999.9 to clear the history.

for (;;) {
struct move *m = list_last_entry(&tq->history, struct move, node);
if (m == latest || m->print_time + m->move_t > expire_time)
if (m == latest || m->print_time + m->move_t > clear_history_time)
break;
list_del(&m->node);
free(m);
Expand All @@ -206,7 +204,7 @@ trapq_set_position(struct trapq *tq, double print_time
, double pos_x, double pos_y, double pos_z)
{
// Flush all moves from trapq
trapq_finalize_moves(tq, NEVER_TIME);
trapq_finalize_moves(tq, NEVER_TIME, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would introduce a subtle change in behavior. All places that fully cleared the history should continue to fully clear the history - thus trapq_finalize_moves(tq, NEVER_TIME, NEVER_TIME);


// Prune any moves in the trapq history that were interrupted
while (!list_empty(&tq->history)) {
Expand Down
3 changes: 2 additions & 1 deletion klippy/chelper/trapq.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ void trapq_append(struct trapq *tq, double print_time
, double start_pos_x, double start_pos_y, double start_pos_z
, double axes_r_x, double axes_r_y, double axes_r_z
, double start_v, double cruise_v, double accel);
void trapq_finalize_moves(struct trapq *tq, double print_time);
void trapq_finalize_moves(struct trapq *tq, double print_time
, double clear_history_time);
void trapq_set_position(struct trapq *tq, double print_time
, double pos_x, double pos_y, double pos_z);
int trapq_extract_old(struct trapq *tq, struct pull_move *p, int max
Expand Down
3 changes: 2 additions & 1 deletion klippy/extras/force_move.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ def manual_move(self, stepper, dist, speed, accel=0.):
0., 0., 0., axis_r, 0., 0., 0., cruise_v, accel)
print_time = print_time + accel_t + cruise_t + accel_t
stepper.generate_steps(print_time)
self.trapq_finalize_moves(self.trapq, print_time + 99999.9)
self.trapq_finalize_moves(self.trapq, print_time + 99999.9,
print_time + 99999.9)
stepper.set_trapq(prev_trapq)
stepper.set_stepper_kinematics(prev_sk)
toolhead.note_kinematic_activity(print_time)
Expand Down
3 changes: 2 additions & 1 deletion klippy/extras/manual_stepper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ def do_move(self, movepos, speed, accel, sync=True):
0., cruise_v, accel)
self.next_cmd_time = self.next_cmd_time + accel_t + cruise_t + accel_t
self.rail.generate_steps(self.next_cmd_time)
self.trapq_finalize_moves(self.trapq, self.next_cmd_time + 99999.9)
self.trapq_finalize_moves(self.trapq, self.next_cmd_time + 99999.9,
self.next_cmd_time + 99999.9)
toolhead = self.printer.lookup_object('toolhead')
toolhead.note_kinematic_activity(self.next_cmd_time)
if sync:
Expand Down
6 changes: 3 additions & 3 deletions klippy/kinematics/extruder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,8 +211,8 @@ def __init__(self, config, extruder_num):
gcode.register_mux_command("ACTIVATE_EXTRUDER", "EXTRUDER",
self.name, self.cmd_ACTIVATE_EXTRUDER,
desc=self.cmd_ACTIVATE_EXTRUDER_help)
def update_move_time(self, flush_time):
self.trapq_finalize_moves(self.trapq, flush_time)
def update_move_time(self, flush_time, clear_history_time):
self.trapq_finalize_moves(self.trapq, flush_time, clear_history_time)
def get_status(self, eventtime):
sts = self.heater.get_status(eventtime)
sts['can_extrude'] = self.heater.can_extrude
Expand Down Expand Up @@ -313,7 +313,7 @@ def cmd_ACTIVATE_EXTRUDER(self, gcmd):
class DummyExtruder:
def __init__(self, printer):
self.printer = printer
def update_move_time(self, flush_time):
def update_move_time(self, flush_time, clear_history_time):
pass
def check_move(self, move):
raise move.move_error("Extrude when no extruder present")
Expand Down
7 changes: 5 additions & 2 deletions klippy/mcu.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -955,15 +955,18 @@ def request_move_queue_slot(self):
self._reserved_move_slots += 1
def register_flush_callback(self, callback):
self._flush_callbacks.append(callback)
def flush_moves(self, print_time):
def flush_moves(self, print_time, clear_history_time):
if self._steppersync is None:
return
clock = self.print_time_to_clock(print_time)
if clock < 0:
return
for cb in self._flush_callbacks:
cb(print_time, clock)
ret = self._ffi_lib.steppersync_flush(self._steppersync, clock)
clear_history_clock = \
max(0, self.print_time_to_clock(clear_history_time))
ret = self._ffi_lib.steppersync_flush(self._steppersync, clock,
clear_history_clock)
if ret:
raise error("Internal error in MCU '%s' stepcompress"
% (self._name,))
Expand Down
11 changes: 7 additions & 4 deletions klippy/toolhead.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ def add_move(self, move):
MOVE_BATCH_TIME = 0.500
STEPCOMPRESS_FLUSH_TIME = 0.050
SDS_CHECK_TIME = 0.001 # step+dir+step filter in stepcompress.c
MOVE_HISTORY_EXPIRE = 30.

DRIP_SEGMENT_TIME = 0.050
DRIP_TIME = 0.100
Expand Down Expand Up @@ -289,13 +290,15 @@ def _advance_flush_time(self, flush_time):
for sg in self.step_generators:
sg(sg_flush_time)
self.last_sg_flush_time = sg_flush_time
clear_history_time = self.mcu.estimated_print_time(
self.reactor.monotonic() - MOVE_HISTORY_EXPIRE)
# Free trapq entries that are no longer needed
free_time = sg_flush_time - self.kin_flush_delay
self.trapq_finalize_moves(self.trapq, free_time)
self.extruder.update_move_time(free_time)
self.trapq_finalize_moves(self.trapq, free_time, clear_history_time)
self.extruder.update_move_time(free_time, clear_history_time)
# Flush stepcompress and mcu steppersync
for m in self.all_mcus:
m.flush_moves(flush_time)
m.flush_moves(flush_time, clear_history_time)
self.last_flush_time = flush_time
def _advance_move_time(self, next_print_time):
pt_delay = self.kin_flush_delay + STEPCOMPRESS_FLUSH_TIME
Expand Down Expand Up @@ -522,7 +525,7 @@ def drip_move(self, newpos, speed, drip_completion):
self.move_queue.flush()
except DripModeEndSignal as e:
self.move_queue.reset()
self.trapq_finalize_moves(self.trapq, self.reactor.NEVER)
self.trapq_finalize_moves(self.trapq, self.reactor.NEVER, 0)
# Exit "Drip" state
self.reactor.update_timer(self.flush_timer, self.reactor.NOW)
self.flush_step_generation()
Expand Down