-
-
Notifications
You must be signed in to change notification settings - Fork 53
Fix Orbitool O2S fan pin definitions and add 4-pin fan support #104
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.
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
base: development
Are you sure you want to change the base?
Conversation
Updated board-definition.json for ldo-orbitool-o2s
Updated toolboard fan pin aliases to match real Orbitool O2s hardware
WalkthroughChange board configuration for ldo-orbitool-o2s: swap inverted pin logic PB0 → PA10, add Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
configuration/boards/ldo-orbitool-o2s/toolboard-config.cfg (1)
17-17: Consider trailing comma consistency.The trailing comma added here is stylistically inconsistent with other non-fan pin entries (lines 11-15), though it's functionally fine and aids future edits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
configuration/boards/ldo-orbitool-o2s/board-definition.json(1 hunks)configuration/boards/ldo-orbitool-o2s/toolboard-config.cfg(1 hunks)
🔇 Additional comments (4)
configuration/boards/ldo-orbitool-o2s/board-definition.json (1)
16-17: LGTM! Pin logic and 4-pin fan configuration correctly updated.The
invertPinLogicchange from PB0 to PA10 aligns with the fan pin reassignment intoolboard-config.cfg, andfourPinFanConnectorCount: 2correctly reflects the two 4-pin fan connectors (part cooling and toolhead cooling) exposed in the pin aliases.configuration/boards/ldo-orbitool-o2s/toolboard-config.cfg (3)
19-20: LGTM! Fan pin reassignments correctly implemented.The pin changes align with the
board-definition.jsonupdates and match the PR's hardware-tested configuration. The regular fan part cooling pin moved from PB0 to PA10, freeing PB0 for the 4-pin fan connector.
22-23: Verify pin sharing between 2-pin and 4-pin toolhead cooling fans.Both
fan_toolhead_cooling_pin(line 20) and4p_toolhead_cooling_pin(line 23) are assigned to PB15. If this represents the same physical connector with alternative configurations (2-pin vs 4-pin), this is correct. However, if these are meant to be independent connectors, this creates a pin conflict.Please confirm that:
- The toolhead cooling connector can be used in either 2-pin or 4-pin mode (mutually exclusive)
- Users won't attempt to configure both simultaneously
If these are separate physical connectors, the pin assignments need correction.
22-23: Excellent addition of 4-pin fan support with tachometer pins.The 4-pin fan definitions correctly expose PWM and tachometer pins for both part cooling and toolhead cooling fans. The tachometer pin assignments (PA14, PB12) match the
customSectionsinboard-definition.json, and the feature was validated with real hardware testing.
Remove unnecessary 4-pin tachometer pin definitions. Previously, the configurator added tachometer fields even for 2-pin fans, which caused a 0 RPM readout on non-tacho setups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
configuration/extruders/sherpa-micro.cfg(1 hunks)
🔇 Additional comments (2)
configuration/extruders/sherpa-micro.cfg (2)
18-18: Verify the high max_extrude_only_distance value.A max_extrude_only_distance of 1400mm is unusually high. While this might be intentional for loading/unloading operations with long Bowden setups, please confirm this value is correct for the Sherpa Micro's typical use case.
15-15: Rotation_distance value verified as correct.The rotation_distance value of 22.67895 matches the standard Klipper starting value for the Sherpa Micro with 5mm Bondtech-style drive gears and is confirmed by official Annex Engineering and community configurations. For non-standard configurations (different motor pinion tooth counts or alternative drive gears), users should calibrate by marking 120mm, extruding 100mm, and adjusting the value if needed.
|
|
||
| [extruder] | ||
| rotation_distance: 22.67895 | ||
| gear_ratio: 50:8 #for sherpa mini 8t motor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify naming: "Sherpa Micro" vs "sherpa mini".
The metadata at line 8 identifies this as "Annex Engineering Sherpa Micro," but this comment refers to "sherpa mini 8t motor." Please confirm whether these terms are interchangeable or if there's a naming inconsistency that should be corrected.
🏁 Script executed:
#!/bin/bash
# Search for other references to "sherpa" variants in the codebase to check naming conventions
rg -i -C2 'sherpa.*(mini|micro)'Length of output: 1832
---
🌐 Web query:
```web
Annex Engineering Sherpa Micro extruder motor specifications 8t
💡 Result:
Brief specs for the Sherpa Micro "8T" extruder motor (commonly sold for Annex Engineering Sherpa Micro):
- Motor type / model family: NEMA14 stepper — LDO 36STH series (often listed as LDO‑36STH20‑1004AHG / 0354AHG with G8T). [1][2]
- Integrated gear: 8‑tooth pinion (8T) sized to mate with Sherpa Micro drivetrain (gives the Sherpa ~50:8 effective gearing). [2][3]
- Body length / package: 36 mm / round NEMA14 with ~20 mm body length variant commonly sold for Sherpa (20 mm motor length). [3]
- Intended use / features: high‑temperature rated extruder motor for Sherpa Mini/Micro and similar compact extruders. Market listings state improved pushing force for high‑performance extrusion. [1][2]
- Typical seller claims: effective extrusion push up to ~10 kg (vendor spec). Use manufacturer datasheet for exact torque/current. [2]
Sources / vendor pages: Annex Engineering product listing (Sherpa Micro), LDO 36STH 8T product listings. [1][2][3]
If you want exact electrical specs (holding torque, step angle, rated current, inductance) I can fetch the LDO 36STH20‑1004AHG datasheet and paste the precise values.
Update comment on line 16 to reflect correct extruder variant.
The comment refers to "sherpa mini" but this configuration file is for the Sherpa Micro (as specified in the metadata on line 8 and the stepper model LDO-36STH20-1004AHG). The Sherpa Micro does use an 8T motor, so update the comment from #for sherpa mini 8t motor to #for sherpa micro 8t motor.
🤖 Prompt for AI Agents
In configuration/extruders/sherpa-micro.cfg around line 16, the inline comment
incorrectly references "sherpa mini" even though this file is for the Sherpa
Micro; edit the comment text on that line to read "#for sherpa micro 8t motor"
so it correctly matches the file metadata and hardware variant.
This PR fixes incorrect Orbitool O2S fan pin definitions and enables the 4-pin dedicated fan option in the configurator.
Changes:
Changes tested on dev-deployment branch and real hardware:
Summary by CodeRabbit