(feat): Adding detecting toolhead runout for standalone toolhead for toolchangers#730
Merged
Merged
Conversation
- Fixed infinite runout to work correctly with toolchangers - Added parking and wiping when waiting for next extruder temperature
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors infinite-runout changeover and TOOL_UNLOAD behavior, adds standalone toolhead sensor runout support and related lane/extruder state flags, updates small function callsites and release notes, and expands and refactors unit tests and fixtures for the new flows. ChangesInfinite Runout & Standalone Toolhead Implementation & Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
CHANGELOG.md (1)
10-10: 💤 Low valueMinor wording improvement for clarity.
The phrase "for standalone toolheads for toolchangers" contains awkward repetition. Consider rewording for better readability.
📝 Suggested rewording
-- Added support for toolhead sensor runout for standalone toolheads for toolchangers +- Added support for toolhead sensor runout on standalone toolheads in toolchanger setupsor
-- Added support for toolhead sensor runout for standalone toolheads for toolchangers +- Added support for toolhead sensor runout for standalone toolheads in toolchangers🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` at line 10, The changelog line "Added support for toolhead sensor runout for standalone toolheads for toolchangers" is repetitive; update the phrasing in CHANGELOG.md (the affected changelog entry) to a clearer form such as "Added support for toolhead sensor runout on standalone toolheads used in toolchangers" or "Added support for toolhead sensor runout for standalone toolheads in toolchangers" to remove the duplicate "for" and improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extras/AFC_lane.py`:
- Around line 1590-1603: The unconditional cleanup after runout corrupts a
newly-loaded lane because _perform_infinite_runout() lets CHANGE_TOOL load the
new lane but the subsequent set_tool_unloaded() calls clear the active spool;
modify the logic in the branch that calls
_perform_infinite_runout()/_perform_pause_runout() so that set_tool_unloaded(),
set_unloaded(), and afc.save_vars() are only executed when infinite runout was
NOT performed (e.g., check self.runout_lane/is_standalone or set a local flag
like performed_infinite_runout when calling _perform_infinite_runout() and skip
the unload/save calls when that flag is true).
In `@extras/AFC.py`:
- Around line 2248-2264: The park/wipe macros use unload_lane which can be
undefined when current_lane_name is None; guard those calls by checking
unload_lane (or current_lane_name) before invoking
self.gcode.run_script_from_command: where you see the blocks referencing
unload_lane and calling f"{self.park_cmd}
EXTRUDER={unload_lane.extruder_obj.name}" and f"{self.wipe_cmd}
EXTRUDER={unload_lane.extruder_obj.name}" (and the surrounding if
self.park/self.wipe checks), wrap or augment those conditionals to ensure
unload_lane is defined (e.g., if unload_lane is not None and self.park and
self.park_cmd is not None) so the park/wipe macros are only executed when
unload_lane exists.
In `@tests/test_AFC_extruder.py`:
- Line 1086: Replace the non-specific assertion on the mock with a
cardinality-specific one: change the call to ext.afc.save_vars.assert_called()
to ext.afc.save_vars.assert_called_once() so the test fails if save_vars is
invoked more than once; update the assertion in the test that references
ext.afc.save_vars to use assert_called_once() to enforce a single call.
In `@tests/test_AFC_vivid.py`:
- Line 537: The tests set lane.raw_load_state to a PropertyMock instance on the
MagicMock itself, making the attribute truthy and preventing the prep_load(lane)
loop from observing the mocked property behavior; instead patch the property on
the lane's class so property access triggers side_effects. Replace assignments
like lane.raw_load_state = PropertyMock(...) with a patch.object call on
type(lane) for "raw_load_state" using new_callable=PropertyMock (create=True)
and set mock_prop.side_effect = [...] before calling unit.prep_load(lane); do
the same for the second test (line ~558) and ensure prep_state remains correctly
set so the loop conditions exercise the property reads.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 10: The changelog line "Added support for toolhead sensor runout for
standalone toolheads for toolchangers" is repetitive; update the phrasing in
CHANGELOG.md (the affected changelog entry) to a clearer form such as "Added
support for toolhead sensor runout on standalone toolheads used in toolchangers"
or "Added support for toolhead sensor runout for standalone toolheads in
toolchangers" to remove the duplicate "for" and improve readability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a1989b47-2fad-4de0-8952-6e20761125bd
⛔ Files ignored due to path filters (1)
.coderabbit/rules.yamlis excluded by!.coderabbit/**
📒 Files selected for processing (12)
CHANGELOG.mdextras/AFC.pyextras/AFC_extruder.pyextras/AFC_lane.pytests/conftest.pytests/test_AFC.pytests/test_AFC_BoxTurtle.pytests/test_AFC_error.pytests/test_AFC_extruder.pytests/test_AFC_hub.pytests/test_AFC_lane.pytests/test_AFC_vivid.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
extras/AFC.py (1)
2236-2239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the active spool after forced ejection.
This new infinite-runout path ejects the old lane with
LANE_UNLOAD(), but it never clears the active spool. If the followingTOOL_LOAD()fails or the printer pauses before another lane becomes active, spoolman still points at the ejected spool.extras/AFC_spool.py:240-252already supportsset_active_spool(None)for this state.Proposed fix
if (force_unload and not unload_lane.is_direct_hub()): # Eject spool before loading next lane for infinite rollover self.LANE_UNLOAD(unload_lane) + self.spool.set_active_spool(None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extras/AFC.py` around lines 2236 - 2239, The infinite-rollover path calls LANE_UNLOAD(unload_lane) but never clears the active spool, so after the forced ejection (inside the branch with force_unload and not unload_lane.is_direct_hub()) call the spool manager's set_active_spool(None) to clear the pointer; locate the branch where LANE_UNLOAD(unload_lane) is invoked and add a call to set_active_spool(None) (using the same manager object used elsewhere in AFC, matching extras/AFC_spool.py's set_active_spool signature) so the active spool is cleared before any subsequent TOOL_LOAD() or pause.
🧹 Nitpick comments (3)
tests/test_AFC_vivid.py (2)
447-447: 💤 Low valueConsolidate
PropertyMockimport with line 14.
PropertyMockis being imported here, butunittest.mockis already imported at line 14. Consider addingPropertyMockto the line 14 import statement for consistency.♻️ Consolidate imports
At line 14:
-from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, patch, PropertyMockThen remove line 447:
-from unittest.mock import PropertyMock🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_AFC_vivid.py` at line 447, The test imports duplicate PropertyMock separately; add PropertyMock to the existing unittest.mock import on the earlier import line (the import that currently imports Mock/patch/etc.) and remove the standalone "from unittest.mock import PropertyMock" import so all unittest.mock names are consolidated into a single import; update references to PropertyMock if needed to use the consolidated import name.
549-563: ⚡ Quick winUse consistent
PropertyMockpattern forraw_load_state.Line 559 assigns
PropertyMockdirectly to thelaneinstance attribute, which doesn't work as a property descriptor (it becomes a truthy object instead). While this doesn't affect this test's behavior (sinceprep_state = Falseshort-circuits the loop condition andraw_load_stateis never accessed), it's inconsistent with the refactored pattern used in all otherprep_loadtests in this file (lines 461, 497, 517, 537).For maintainability and semantic correctness, use
_make_afc_lane()and thepatch.object(type(lane), "raw_load_state", new_callable=PropertyMock)pattern.♻️ Align with the refactoring pattern
def test_uncalibrated_lane_updates_dist_hub_no_prep(self): - from unittest.mock import PropertyMock unit = _make_vivid() - lane = MagicMock() + lane = _make_afc_lane() lane.calibrated_lane = False lane.prep_state = False - lane.move_to.return_value = (True, 300.0, False) + lane.move_to = MagicMock(return_value=(True, 300.0, False)) unit.lane_loading = MagicMock() unit.select_lane = MagicMock() unit.lane_loaded = MagicMock() - lane.raw_load_state = PropertyMock(side_effect=[False]) - - unit.prep_load(lane) + with patch.object(type(lane), "raw_load_state", new_callable=PropertyMock) as mock_prop: + mock_prop.return_value = False + unit.prep_load(lane) assert lane.calibrated_lane is False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_AFC_vivid.py` around lines 549 - 563, The test test_uncalibrated_lane_updates_dist_hub_no_prep should use the same PropertyMock pattern as other tests: create the lane via _make_afc_lane()/or _make_vivid() helper and replace the direct assignment lane.raw_load_state = PropertyMock(...) with a patch.object on the lane's class (e.g. patch.object(type(lane), "raw_load_state", new_callable=PropertyMock, side_effect=[False])) so raw_load_state becomes a proper property descriptor; update imports/usages accordingly and keep lane.calibrated_lane, lane.prep_state, lane.move_to, unit.select_lane, unit.lane_loading, and unit.lane_loaded interactions unchanged.tests/test_AFC_lane.py (1)
1528-1535: 💤 Low valueUnused variable
sensorcan be removed.The variable
sensor = "None"is declared but never used sincehandle_toolhead_runout()is called without arguments.♻️ Suggested cleanup
def test_standalone_runout_disabled_None(self): - sensor = "None" lane = self._make_lane_for_toolhead_runout(standalone=True) lane.extruder_obj.fila_tool_end.runout_helper.sensor_enabled = False lane.handle_toolhead_runout() warn_msgs = [m for lvl, m in lane.logger.messages if lvl == "warning"] assert any("toolhead runout has been detected," in m for m in warn_msgs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_AFC_lane.py` around lines 1528 - 1535, Remove the unused local variable `sensor = "None"` from the test `test_standalone_runout_disabled_None`; the test calls `lane.handle_toolhead_runout()` with no arguments so `sensor` is never used—delete the `sensor` assignment line in that test to clean up the code and avoid the unused-variable warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@extras/AFC.py`:
- Around line 2236-2239: The infinite-rollover path calls
LANE_UNLOAD(unload_lane) but never clears the active spool, so after the forced
ejection (inside the branch with force_unload and not
unload_lane.is_direct_hub()) call the spool manager's set_active_spool(None) to
clear the pointer; locate the branch where LANE_UNLOAD(unload_lane) is invoked
and add a call to set_active_spool(None) (using the same manager object used
elsewhere in AFC, matching extras/AFC_spool.py's set_active_spool signature) so
the active spool is cleared before any subsequent TOOL_LOAD() or pause.
---
Nitpick comments:
In `@tests/test_AFC_lane.py`:
- Around line 1528-1535: Remove the unused local variable `sensor = "None"` from
the test `test_standalone_runout_disabled_None`; the test calls
`lane.handle_toolhead_runout()` with no arguments so `sensor` is never
used—delete the `sensor` assignment line in that test to clean up the code and
avoid the unused-variable warning.
In `@tests/test_AFC_vivid.py`:
- Line 447: The test imports duplicate PropertyMock separately; add PropertyMock
to the existing unittest.mock import on the earlier import line (the import that
currently imports Mock/patch/etc.) and remove the standalone "from unittest.mock
import PropertyMock" import so all unittest.mock names are consolidated into a
single import; update references to PropertyMock if needed to use the
consolidated import name.
- Around line 549-563: The test test_uncalibrated_lane_updates_dist_hub_no_prep
should use the same PropertyMock pattern as other tests: create the lane via
_make_afc_lane()/or _make_vivid() helper and replace the direct assignment
lane.raw_load_state = PropertyMock(...) with a patch.object on the lane's class
(e.g. patch.object(type(lane), "raw_load_state", new_callable=PropertyMock,
side_effect=[False])) so raw_load_state becomes a proper property descriptor;
update imports/usages accordingly and keep lane.calibrated_lane,
lane.prep_state, lane.move_to, unit.select_lane, unit.lane_loading, and
unit.lane_loaded interactions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ff91020-a993-40eb-9959-2252f064f001
📒 Files selected for processing (6)
extras/AFC.pyextras/AFC_functions.pyextras/AFC_lane.pytests/test_AFC_extruder.pytests/test_AFC_lane.pytests/test_AFC_vivid.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_AFC_extruder.py
jimmyjon711
force-pushed
the
toolhead_runout
branch
from
e0771ec to
25da48f
Compare
kekiefer
requested changes
Jun 8, 2026
kekiefer
left a comment
kekiefer
left a comment
Contributor
There was a problem hiding this comment.
There were some behavioral changes to tool change temp drop that we can discuss out of band if you like. Maybe the unified code path is desirable (at least for the heater sequencing) but the park and wipe on reheat aren't needed for this case.
ejsears
approved these changes
Jun 8, 2026
ejsears
left a comment
ejsears
left a comment
Collaborator
There was a problem hiding this comment.
From a purely non-toolchanger, functional point of view (not code based), I couldn't find any regressions .
- Added back original hotend cooldown and added not infinite runout qualifier to this cooldown - Added infinite rundown qualifier to hotend cooldown, park and wipe after TOOL_UNLOAD
kekiefer
approved these changes
Jun 9, 2026
Contributor
|
Lgtm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Major Changes in this PR
Note to reviewers
This PR is not meant to allow toolhead runout for toolheads that have units attached to them, that should be address with issue #729
How the changes in this PR are tested
On my Voron 2.4 toolchanger and Snapmaker U1
2.4 toolchanger logs:
Snapmaker log
PR Checklist: (Checked-off items are either done or do not apply to this PR)
NOTE: GitHub Copilot may be used for automated code reviews, however as it is an automated system, it's suggestions
may not be correct. Please do not rely on it to catch all issues. Please review any suggestions it makes and use your
own judgement to determine if they are correct.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation