Update ratos_hybrid_corexy.py #81
Conversation
Update to make compatible with Klipper refactoring (Klipper3d/klipper#6815), in particular * idex_modes: Internal refactoring of handling dual carriages
WalkthroughRefactors dual-carriage initialization and selection, standardizes homing calls to pass axis, adds Changes
Sequence Diagram(s)sequenceDiagram
participant Printer
participant RatOSHybridCoreXYKinematics as Kinematics
participant DualCarriages as DC
Printer->>Kinematics: initialize(printer, config)
Kinematics->>DC: new DualCarriages(primary_rails=[rails[0]], secondary_rails=[rails[3]], axes=[0], safe_dist)
Note right of DC #DFF2E1: Dual-carriage object created with explicit axes
loop Homing flow
Printer->>Kinematics: home(homing_state)
alt axis == 0
Kinematics->>DC: home(homing_state, axis)
else other axis
Kinematics->>Kinematics: home_axis(homing_state, axis)
end
end
Printer->>Kinematics: clear_homing_state(clear_axes)
Kinematics->>Kinematics: set limits for each axis in clear_axes to (1.0, -1.0)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (1)
configuration/klippy/kinematics/ratos_hybrid_corexy.py (1)
6-6: Attribution line added.Good practice adding an attribution line for the changes. However, note that the year 2025 is in the future.
Consider updating to the current year:
-# Modified 2025 by Github user 'dbstf' +# Modified 2024 by Github user 'dbstf'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
configuration/klippy/kinematics/ratos_hybrid_corexy.py(5 hunks)
🔇 Additional comments (5)
configuration/klippy/kinematics/ratos_hybrid_corexy.py (5)
46-46: Fixed capitalization of "Cartesian".Proper capitalization of "Cartesian" is correct as it's derived from René Descartes' name.
64-67: Refactored dual carriage initialization for compatibility with Klipper PR #6815.The dual carriage initialization has been updated to match Klipper's refactored IDEX modes handling. This change properly initializes the DualCarriages object with the printer, rail lists, axes specification, and safe distance parameter.
105-106: Simplified primary rail selection logic.The condition to determine when to use the dual carriage module has been simplified, and the axis parameter is now explicitly passed to get_primary_rail(), which aligns with the updated Klipper IDEX API.
110-113: Added method to clear homing state for specified axes.Good addition. This new method provides a way to selectively invalidate the homing state for specific axes, which complements the existing
note_z_not_homedmethod and enhances the kinematics module's functionality.
132-132: Updated dual carriage homing to include axis parameter.The
homemethod call has been updated to pass the axis parameter to the dual carriage module's home method, which is required for compatibility with the refactored Klipper IDEX implementation.
|
Hi Stefan @dbstf - can I ask how much practical testing this change has received and on what machines? |
|
Hi Tom, unfortunately, the code is not tested in production at all, yet. I should have mentioned this in the original commit. I'm currently in the process of piecing all parts together for a V-Core 4. For testing, I flashed the latest release of RatOS and updated Klipper to the recent version on Github. The most recent updates broke the ratos_kinematic (lots of warning on the Mainsail status screen), hence I updated the script to the best of my knowledge till all errors in Mainsail were gone. I committed my modified script to provide a starting point for all the other people developing, updating and testing RatOS. |
|
If you're going to be commissioning a new machine, I'd recommend that you stick with the pinned commit of klipper that's defined in the |
Update to make compatible with Klipper3d/klipper@9399e73 ,i.e., Klipper3d/klipper#7001
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
configuration/klippy/kinematics/ratos_hybrid_corexy.py (3)
46-47: No-op getchoice: confirm intent and config UX.Calling
getchoice('axis', {'x': 'x'}, default='x')purely for validation is fine, but it silently ignores non-xvalues. If users setaxis: xonly to satisfy config parsing, consider adding a brief comment in docs or log a debug explaining it’s ignored.
110-114: New helper looks good; add a short docstring for maintainers.Tiny docs help future readers understand accepted values for
clear_axes.Apply:
- def clear_homing_state(self, clear_axes): + def clear_homing_state(self, clear_axes): + """Reset homing limits for the given axes. + clear_axes: iterable of axis names, e.g. {'x','y'} or 'xz'.""" for axis, axis_name in enumerate("xyz"): if axis_name in clear_axes: self.limits[axis] = (1.0, -1.0)
65-67: Consider a backward/forward-compat shim with pinned Klipper.Given RatOS pins Klipper to a specific commit, guard new-API usage to avoid breaking stable installs. Detect the available signature at runtime and route accordingly (constructor with
axes=[...]vs. legacy,get_primary_rail(axis)vs.get_primary_rail().get_rail(), andhome(homing_state, axis)vs.home(homing_state)).If you want, I can draft a minimal shim using
inspect.signatureto branch on parameter names and method arity.Also applies to: 102-109, 132-132
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
configuration/klippy/kinematics/ratos_hybrid_corexy.py(5 hunks)
🔇 Additional comments (5)
configuration/klippy/kinematics/ratos_hybrid_corexy.py (5)
6-6: Attribution update acknowledged.No functional impact; OK to merge.
70-70: No functional change.Blank line from surrounding edits. Nothing to do.
65-67: DualCarriages init: align with new API and make default explicit.idex_modes.py not found in repo — cannot verify DualCarriages constructor or status keys. Extract safe_dist for readability and pass it explicitly; then confirm the DualCarriages signature and that get_status exposes 'carriage_1'.
File: configuration/klippy/kinematics/ratos_hybrid_corexy.py (around lines 65-67)
- self.dc_module = idex_modes.DualCarriages( - self.printer, [self.rails[0]], [self.rails[3]], axes=[0], - safe_dist=dc_config.getfloat( - 'safe_distance', None, minval=0.)) + safe_dist = dc_config.getfloat('safe_distance', default=None, minval=0.) + self.dc_module = idex_modes.DualCarriages( + self.printer, + [self.rails[0]], + [self.rails[3]], + axes=[0], + safe_dist=safe_dist + )Verify by locating idex_modes.py and checking DualCarriages.init and get_status (ensure 'carriage_1' key).
102-109: Shadowed variable in set_position; small clarity refactor.File: configuration/klippy/kinematics/ratos_hybrid_corexy.py (set_position, ~lines 102–109)
- for i, rail in enumerate(self.rails): - rail.set_position(newpos) + for i, r in enumerate(self.rails): + r.set_position(newpos) for axis in homing_axes: - if self.dc_module and axis == 0: - rail = self.dc_module.get_primary_rail(axis) - else: - rail = self.rails[axis] - self.limits[axis] = rail.get_range() + if self.dc_module and axis == 0: + selected_rail = self.dc_module.get_primary_rail(axis) + else: + selected_rail = self.rails[axis] + self.limits[axis] = selected_rail.get_range()Confirm get_primary_rail(axis) returns an object exposing get_range() (repo search did not locate its definition; ensure it isn’t the older wrapper requiring .get_rail()).
132-132: Confirm DualCarriages.home accepts (homing_state, axis).DualCarriages is instantiated at configuration/klippy/kinematics/ratos_hybrid_corexy.py:64 and called at configuration/klippy/kinematics/ratos_hybrid_corexy.py:131–132; the class definition wasn't found in the repo — verify idex_modes.DualCarriages.home signature matches this call.
2 participants
Update to make compatible with Klipper refactoring (Klipper3d/klipper#6815), in particular
Summary by CodeRabbit
New Features
Improvements