[fix]: Updates so that Snapmaker U1 pause/resume and power loss resume works correctly with AFC#728
Merged
jimmyjon711 merged 3 commits intoJun 6, 2026
Conversation
… correctly - Updated CHANGE_TOOL method to call original T(n) if A(n) param is passed in. - Updated M104/M109 macro to heat hot end based off T(n) index if A(n) param was passed in.
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAFC now detects Snapmaker printers via klippy.Printer, special-cases CHANGE_TOOL and M109 commands carrying an ChangesSnapmaker U1 Support
Sequence Diagram(s)sequenceDiagram
participant GcodeSource as G-code source
participant AFC as afc.cmd_CHANGE_TOOL
participant Klippy as klippy.Printer
participant Handlers as gcode.ready_gcode_handlers
GcodeSource->>AFC: CHANGE_TOOL A=<val>
AFC->>Klippy: probe get_snapmaker_config_dir (snapmaker_printer)
Klippy-->>AFC: present / absent
AFC->>Handlers: lookup renamed handler (e.g., _T0)
Handlers-->>AFC: handler exists
AFC->>Handlers: run_script_from_command with A=<val>
AFC-->>GcodeSource: return early (skip normal tool-change)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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
🤖 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_stepper.py`:
- Around line 724-725: The debug log at logger.debug in the homing routine can
reference dist_mm and steps_moved even if the metric calculation (lines
~711-721) raises, causing UnboundLocalError; to fix, initialize or set safe
fallback values for dist_mm and steps_moved before the try/except (or assign
them in the except handler) so they are always defined when logging in the
homing method (referencing the homing method, the logger.debug call, and
variables dist_mm, steps_moved, end_ts, start_ts); make sure the fallback values
meaningfully indicate a failed metric calc (e.g., None or -1) so the log remains
informative.
In `@extras/AFC.py`:
- Around line 2442-2450: The self.tools.get() call on the line where
snapmaker_param_a is not None and self.snapmaker_printer is true currently falls
back to self.toolhead.get_extruder() if the extruder_name is not found, which
silently uses the active extruder and recreates the wrong-hotend behavior.
Instead of providing a default fallback with self.toolhead.get_extruder(), make
the code fail fast by removing the default parameter and either directly raise
an error if the extruder_name is not in self.tools or handle the missing tool
configuration explicitly to prevent silent fallback to the active extruder.
In `@tests/conftest.py`:
- Around line 192-196: The klippy test helper _make_klippy_ready_mock should not
set klippy.Printer = MagicMock() because hasattr(Printer,
"get_snapmaker_config_dir") will be truthy on a MagicMock; instead define
klippy.Printer as a simple concrete class or real Printer type that only exposes
the real attributes (e.g., class DummyPrinter: pass) so hasattr(DummyPrinter,
"get_snapmaker_config_dir") behaves correctly, update the mock factory to assign
that class and keep mod.message_ready; also remove or rename the duplicated
test_check_for_snapmaker_signature in TestCheckForSnapmakerSignature so the
non-Snapmaker test isn’t overwritten and both cases of
extras.AFC::_check_for_snapmaker_signature are covered.
In `@tests/test_AFC.py`:
- Around line 2391-2404: The two test methods both named
test_check_for_snapmaker_signature_false collide so the "false" case is
overwritten; rename one (e.g. change the second to
test_check_for_snapmaker_signature_true) so both run, keeping the second's setup
that sets Printer.get_snapmaker_config_dir = MagicMock(), calling obj =
_make_afc(), invoking obj._check_for_snapmaker_signature(), and asserting
obj.snapmaker_printer is True; ensure the first retains the deletion of
Printer.get_snapmaker_config_dir and asserts not obj.snapmaker_printer.
🪄 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: 43761fe0-22d9-4945-b68d-433e151ad225
📒 Files selected for processing (5)
extras/AFC.pyextras/AFC_prep.pyextras/AFC_stepper.pytests/conftest.pytests/test_AFC.py
- Updated unit tests - Updated issues pointed out by coderabbit - Updating changelog
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)
tests/test_AFC.py (1)
1371-1394:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIsolate
Printermonkeypatching per test.These cases mutate
Printer.get_snapmaker_config_diron the shared mock class and rely on later tests to clean it up. That makes the Snapmaker assertions order-dependent and can leak state across the module.Suggested fix
class TestCmdChangeTool_SnapmakerPath: - def test_setting_A_param(self): + def test_setting_A_param(self, monkeypatch): obj, _, _ = _make_afc_for_change_tool() - setattr(Printer, "get_snapmaker_config_dir", self.get_snapmaker_config_dir) + monkeypatch.setattr( + Printer, "get_snapmaker_config_dir", self.get_snapmaker_config_dir, raising=False + ) gcmd = self._make_gcmd() obj.gcode = MagicMock() obj.gcode.ready_gcode_handlers = {"_T0": MagicMock()} ret = obj.cmd_CHANGE_TOOL(gcmd) obj.gcode.run_script_from_command.assert_called_once() - def test_setting_A_param_snapmaker_false(self): + def test_setting_A_param_snapmaker_false(self, monkeypatch): obj, _, _ = _make_afc_for_change_tool() obj.function.check_homed.return_value = False - if hasattr(Printer, "get_snapmaker_config_dir"): - delattr(Printer, "get_snapmaker_config_dir") + monkeypatch.delattr(Printer, "get_snapmaker_config_dir", raising=False) gcmd = self._make_gcmd() obj.gcode = MagicMock() obj.gcode.ready_gcode_handlers = {"_T0": MagicMock()} ret = obj.cmd_CHANGE_TOOL(gcmd) obj.gcode.run_script_from_command.assert_not_called() assert not ret - def test_setting_A_param_not_in_ready_gcode_handlers(self): + def test_setting_A_param_not_in_ready_gcode_handlers(self, monkeypatch): obj, _, _ = _make_afc_for_change_tool() obj.function.check_homed.return_value = False - setattr(Printer, "get_snapmaker_config_dir", self.get_snapmaker_config_dir) + monkeypatch.setattr( + Printer, "get_snapmaker_config_dir", self.get_snapmaker_config_dir, raising=False + ) obj.function.check_homed.return_value = False gcmd = self._make_gcmd("T5") obj.gcode = MagicMock() obj.gcode.ready_gcode_handlers = {"_T0": MagicMock()} ret = obj.cmd_CHANGE_TOOL(gcmd) @@ class TestCheckForSnapmakerSignature: - def test_check_snapmaker_printer_property_false(self): + def test_check_snapmaker_printer_property_false(self, monkeypatch): obj = _make_afc() obj.printer = Printer - if hasattr(Printer, "get_snapmaker_config_dir"): - delattr(Printer, "get_snapmaker_config_dir") + monkeypatch.delattr(Printer, "get_snapmaker_config_dir", raising=False) assert not obj.snapmaker_printer - def test_check_snapmaker_printer_property_true(self): + def test_check_snapmaker_printer_property_true(self, monkeypatch): obj = _make_afc() obj.printer = Printer - setattr(Printer, "get_snapmaker_config_dir", True) + monkeypatch.setattr(Printer, "get_snapmaker_config_dir", True, raising=False) assert obj.snapmaker_printerAlso applies to: 2399-2406
🤖 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.py` around lines 1371 - 1394, The tests are mutating Printer.get_snapmaker_config_dir on a shared class, causing order-dependent leaks; update each test that sets or deletes Printer.get_snapmaker_config_dir (e.g., test_setting_A_param_snapmaker_false and the similar block around lines referenced) to isolate the monkeypatch by restoring the original attribute after the test (or use the test framework's monkeypatch fixture to set/undo it automatically); specifically, capture the original = getattr(Printer, "get_snapmaker_config_dir", sentinel) before changing it, perform the setattr/delattr and assertions inside the test, then in a finally block restore the original (setattr if original is not sentinel, else delattr), or replace with monkeypatch.setattr/monkeypatch.delenv-equivalent so no state leaks between tests.
🤖 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 `@tests/test_AFC.py`:
- Around line 1371-1394: The tests are mutating Printer.get_snapmaker_config_dir
on a shared class, causing order-dependent leaks; update each test that sets or
deletes Printer.get_snapmaker_config_dir (e.g.,
test_setting_A_param_snapmaker_false and the similar block around lines
referenced) to isolate the monkeypatch by restoring the original attribute after
the test (or use the test framework's monkeypatch fixture to set/undo it
automatically); specifically, capture the original = getattr(Printer,
"get_snapmaker_config_dir", sentinel) before changing it, perform the
setattr/delattr and assertions inside the test, then in a finally block restore
the original (setattr if original is not sentinel, else delattr), or replace
with monkeypatch.setattr/monkeypatch.delenv-equivalent so no state leaks between
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5494ad22-dbfb-40c9-8bde-82f73ae33495
📒 Files selected for processing (5)
CHANGELOG.mdextras/AFC.pyextras/AFC_stepper.pytests/conftest.pytests/test_AFC.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/conftest.py
- extras/AFC_stepper.py
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
1 participant
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
Before these changes U1 printer would select the wrong toolhead when resuming from pause or heat the wrong toolhead when resuming from power loss. These fixes allow pause/resume and power loss resume to work correctly when having more that 4 T(n) macros defined by AFC.
How the changes in this PR are tested
Tested on my U1 printer.
2.4 Printer, Snapmaker Detected not printing out in console during PREP:
Snapmaker U1, Snapmaker Printer Decteted printing out in console during PREP:
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
Behaviour / UX
Tests
Documentation