[RFC] Feature: Implement Region-Aware Filament Runout Pause#7274
[RFC] Feature: Implement Region-Aware Filament Runout Pause#7274famtory wants to merge 9 commits into
Conversation
|
Are there any limitations why this can't be implemented through gcode macros and delayed_gcode methods? Let's say, the slicer writes a key gcode command to the gcode file when switching to region "infill", that calls a macro. The macro checks whether the filament sensor has been triggered and, if so, sends the pause command. Along with this, the original "runout_gcode" parameter can set some global flag and activate delayed_gcode which will check the time or filament length, so that if we don't get to the "infill" region for a long time, the pause is still called. This is just my opinion, but adding a new global gcode command to Klipper for such a niche functionality does not sound very practical. |
|
@MRX8024 Here is the exact scenario of why a macro-based check does not work: Imagine the slicer outputs the following G-code: (Line 1) G1 X10 Y10 ; Outer wall (toolhead physically executing this)
(Line 2) G1 X20 Y20 ; Outer wall (filament physically runs out here)
(Line 3) CHECK_RUNOUT_PAUSE ; Macro inserted by slicer to check the flag
(Line 4) G1 X30 Y30 ; Infill start
Why Python is Necessary:
I hope this clarifies why we moved past the macro-based approach and opted for a native Python implementation with lookahead callbacks! |
|
Useful idea! I hope it won't get lost and end up in the "never reviewed" trashbin |
Signed-off-by: Hwang Younsang <famtory@gmail.com>
Signed-off-by: Hwang Younsang <famtory@gmail.com>
Signed-off-by: Hwang Younsang <famtory@gmail.com>
|
Hi @dewi-ny-je, thanks for the support! I really appreciate it. |
|
Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html There are some steps that you can take now:
Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available. Best regards, PS: I'm just an automated script, not a human being. |
|
It would be useful to have an opinion by @KevinOConnor about the feature: once reviewed, would it be accepted? |
dewi-ny-je
left a comment
There was a problem hiding this comment.
[PROBLEM] Missing documentation (hard requirement)
New config options runout_distance and extruder, the new SET_PRINT_FEATURE command, and the feature-code convention (0=other, 1=outer wall, 2=infill) are not documented. At minimum update docs/Config_Reference.md and docs/G-Codes.md. The units of runout_distance (mm of filament/extruder advance) and the slicer-integration requirement should be stated explicitly.
[PROBLEM] No test coverage
A behavioral feature with non-trivial state machine and timer logic ships with no regression test under test/klippy/. Given the dead-code bug above slipped through, a test exercising defer → reinsert and defer → distance-limit would be valuable.
[SUGGESTION] Magic numbers for feature codes
current_feature == 1 / == 2 appear in three places (_runout_distance_check_event, note_filament_present). Define named constants (e.g. FEATURE_OUTER_WALL = 1, FEATURE_INFILL = 2) for readability and to keep the slicer contract in one place.
[SUGGESTION] Redundant initialization
feature_history and deferred_runout are initialized in both __init__ and _handle_ready. Intentional (reset on restart) but worth a one-line comment, or consolidate.
[DISCUSSION] Cosmetic scope of deferral
Deferral triggers only when the runout occurs exactly during feature 1 (outer wall). Runouts during other visible features (e.g. top surfaces, bridges, which may map to 0) pause immediately. If the goal is "protect print cosmetics," the author may want to consider whether other visible features should defer too — or document that only outer walls are protected by design.
Things that are correct (verified)
extruder.find_past_position(print_time)andmcu.estimated_print_time(eventtime)both exist and are used correctly;runout_distanceis effectively measured in mm of commanded extruder advance.- The lookahead-callback → broadcast-event pattern correctly aligns feature state with toolhead position across multiple sensors.
- The single reused timer (
register_timeronce,update_timer) avoids leaking timers — good.
| is_printing = idle_timeout.get_status(now)["state"] == "Printing" | ||
| # Perform filament action associated with status change (if any) | ||
| if is_filament_present: | ||
| if self.deferred_runout: |
There was a problem hiding this comment.
Reinsertion-clearing branch is dead code during deferral
In note_filament_present, the runout branch sets self.min_event_systime = self.reactor.NEVER before starting deferral. While deferred, if filament is reinserted, the method is called with is_filament_present=True, but the guard if eventtime < self.min_event_systime ... is eventtime < NEVER → always True → early return. So the block:
if is_filament_present:
if self.deferred_runout:
self.deferred_runout = False
logging.info("... filament reinserted, clearing deferred runout")can never execute during the deferral window. The deferred runout will therefore still fire and pause the print even if the user reinserts filament in time — defeating the purpose of that branch. The reinsertion check needs to run before/independently of the min_event_systime gate (or the gate needs an exception for the deferred state).
| self.printer.register_event_handler("filament_sensor:set_feature", | ||
| self._handle_set_feature) | ||
| try: | ||
| self.gcode.register_command( |
There was a problem hiding this comment.
[SUGGESTION] register_command with swallowed config_error
SET_PRINT_FEATURE is registered per-RunoutHelper via the global register_command, relying on except self.printer.config_error: pass to absorb the duplicate registration from subsequent sensors. This works (the actual feature data is distributed via the broadcast event), but a blanket except config_error: pass could mask unrelated registration errors. Consider registering the command once (module-level guard, or only the first instance), which also makes the intent explicit.
| self.deferred_runout = False | ||
| self.reactor.register_callback(self._runout_event_handler) | ||
| return self.reactor.NEVER | ||
| return eventtime + 0.100 |
There was a problem hiding this comment.
[SUGGESTION] Hardcoded poll interval
The return eventtime + 0.100 re-arm interval in _runout_distance_check_event is a magic literal; a named constant documents intent.
| current_feature = self._get_feature_at_time(print_time) | ||
| if current_feature == 1 and self.runout_distance > 0.: | ||
| self.deferred_runout = True | ||
| self.runout_trigger_pos = ( |
There was a problem hiding this comment.
[SUGGESTION] Initialize dynamic attribute in __init__
self.runout_trigger_pos is created only inside the deferral branch. It's safe today (only read when deferred_runout is True, set in the same branch), but initializing it in __init__ alongside deferred_runout/runout_check_timer is more robust and consistent.
Signed-off-by: Hwang Younsang <famtory@gmail.com>
- Fix dead code bug in reinsertion clearing logic during deferred runout. - Add module-level constants for feature codes and poll interval. - Consolidate and clarify variable initialization in RunoutHelper. - Safely handle config_error during command registration. Signed-off-by: Hwang Younsang <famtory@gmail.com>
|
Hi @dewi-ny-je, I have just pushed a new commit to address all of your suggestions:
Thanks again for catching that dead code logic and helping me polish this PR! Hopefully, this makes it easier for @KevinOConnor to review when he has the time. |
Signed-off-by: Hwang Younsang <famtory@gmail.com>
First of all, I don't want credit I don't deserve: I spent the time to feed a LLM and I checked some answers, but it's still work of an LLM.
Yes the scope is cosmetic, but the idea you had is really good and it might be a good idea to apply it to bridges as well, since bridges are usually short enough and deferring pause would surely allow completion without any filament feed issue, while preserving the bridge which will definitely fail if printing is stopped and restarted. I would add bridges to the scope, but it's only my personal opinion. |
Signed-off-by: Hwang Younsang <famtory@gmail.com>
|
Hi @dewi-ny-je, You raise an excellent point about bridges! A pause mid-bridge is practically a guaranteed failure due to the lack of underlying support and tension loss, whereas deferring the pause to let the bridge finish on residual filament is much safer. I completely agree with your suggestion, so I have just pushed an update to include Bridges in the deferral scope.
Thanks again for the thoughtful feedback. Your insights are really helping shape this into a much more robust feature! |
|
@famtory can you check the following remarks? "reinsert to cancel a pending runout" behavior does not work Every place
The core invariant
157 elif is_printing and self.runout_gcode is not None:
158 # runout detected
159 self.min_event_systime = self.reactor.NEVER # <-- gate slammed shut
...
166 if current_feature in (self.FEATURE_OUTER_WALL,
167 self.FEATURE_BRIDGE) \
168 and self.runout_distance > 0.:
169 self.deferred_runout = True # <-- deferral armedSo the instant Now, when is 97 self.deferred_runout = False
98 self.reactor.register_callback(self._runout_event_handler)Therefore there is no moment in time where Why that makes lines 145–149 unreachableWhen the user reinserts filament during the deferral window,
Execution never reaches line 144, so the block at lines 145–149: 145 if self.deferred_runout:
146 self.deferred_runout = False
147 logging.info(
148 "Filament Sensor %s: filament reinserted, "
149 "clearing deferred runout" % (self.name,))can never execute while a deferral is active — which is the only situation it was written for. It is dead code. Observable consequence
So the advertised "reinsert to cancel a pending runout" behavior does not work. (Note also that because of the same early return, an insert during deferral can't trigger Fix directionThe reinsertion/deferral-cancel check must run before (or be exempted from) the self.filament_present = is_filament_present
if is_filament_present and self.deferred_runout:
self.deferred_runout = False
logging.info("Filament Sensor %s: filament reinserted, "
"clearing deferred runout" % (self.name,))
return
if eventtime < self.min_event_systime or not self.sensor_enabled:
returnThat keeps the existing gate intact for the normal debounce/event-delay purposes while letting a genuine reinsertion cancel an in-flight deferral. |
I would like to propose a region-aware filament runout pause mechanism for
filament_switch_sensor/filament_motion_sensor(RunoutHelper).Why this feature is needed:
Pausing immediately upon filament runout often leaves an ugly scar on the outer wall of the print. By delaying the pause until the toolhead reaches an infill area (where defects are hidden inside), we can protect the cosmetic print quality.
How it works:
SET_PRINT_FEATURE FEATURE=<N>(1: Outer Wall, 2: Infill, 3: Other) on role changes.RunoutHelperregisters theSET_PRINT_FEATUREcommand and usestoolhead.register_lookahead_callbackto synchronize the active feature state with the lookahead queue.runout_distance(safety margin, user-configurable).Current Status:
I have implemented a working draft in
klippy/extras/filament_switch_sensor.pywhererunout_distancedefaults to0.0(disabled by default, requiring explicit user configuration to activate).I would love to get the community's feedback on this approach and any suggestions regarding naming or architecture before moving forward.
Signed-off-by: Hwang Younsang famtory@gmail.com