feat(Happy Hare): Adds sync-feedback buffer rendering in the filament status#1758
feat(Happy Hare): Adds sync-feedback buffer rendering in the filament status#1758pedrolamas merged 5 commits intofluidd-core:developfrom
Conversation
Signed-off-by: Pedro Lamas <pedrolamas@gmail.com> # Conflicts: # src/store/printer/types.ts
There was a problem hiding this comment.
Pull request overview
This PR adds visual rendering of a sync-feedback buffer in the MMU filament status component, supporting multiple sensor types: compression-only, tension-only, dual-switch, and proportional sensors.
- Adds new Klipper state property
sync_feedback_bias_modelledto track buffer position - Implements animated piston visualization that moves based on bias values
- Refactors sync feedback display logic to show buffer alongside existing sensor indicators
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/typings/klipper.d.ts | Adds sync_feedback_bias_modelled property to MMU state interface |
| src/mixins/mmu.ts | Adds getter for syncFeedbackBiasModelled to access the new state value |
| src/components/widgets/mmu/MmuFilamentStatus.vue | Adds SVG buffer/piston elements, new computed properties for sensor types, and updates sync feedback rendering logic with animation |
| return ( | ||
| this.syncFeedbackEnabled && (this.hasSensor('filament_compression') || this.hasSensor('filament_tension')) | ||
| ) | ||
| return this.hasFilamentCompressionSensor || this.hasFilamentTensionSensor || this.hasFilamentProportionalSensor |
There was a problem hiding this comment.
The hasSyncFeedback getter no longer checks syncFeedbackEnabled, which was part of the original logic. This means the sync feedback buffer will always be displayed when any of the sensors exist, regardless of whether sync feedback is enabled. Consider whether syncFeedbackEnabled should still be part of this condition or if this change is intentional.
| return this.hasFilamentCompressionSensor || this.hasFilamentTensionSensor || this.hasFilamentProportionalSensor | |
| return this.syncFeedbackEnabled && (this.hasFilamentCompressionSensor || this.hasFilamentTensionSensor || this.hasFilamentProportionalSensor) |
There was a problem hiding this comment.
That is correct behavior because the sync-feedback "buffer" is also used as an endstop for homing the filament to the extruder. The actual enabled flag is simply whether dynamic rotation_distance adjustment is taking place and not about the existence of the sync feedback sensors.
| <text | ||
| x="-22" | ||
| y="4" | ||
| font-size="11px" | ||
| text-anchor="end" | ||
| > | ||
| {{ syncFeedbackPistonText }} | ||
| </text> |
There was a problem hiding this comment.
Vue template interpolation inside SVG defs may not work as expected. The text element with {{ syncFeedbackPistonText }} is defined in defs and referenced via use, which means the interpolation will only evaluate once during definition, not dynamically update when syncFeedbackPistonText changes. Consider moving the text element outside of defs and positioning it directly where needed, or use a different approach for dynamic text in reusable SVG elements.
There was a problem hiding this comment.
I think Copilot is mistaken (or it is with older Vue implementations) because I tested on Chrome, Safari, Firefox (on Mac) and Safari/Chrome on iPhone and it is reactive as expected(?)
I don't understand Vue well enough to know why, but I have validated it works as designed...
|
@moggieuk can you please review the 2 comments above? The rest LGTM. |
2 participants
Description:
This PR added a sync-feedback buffer rendering in the filament status component. Supports:
Compression-only, Tension-only, Dual-switch and Proportional sensors
SIgned off by: Paul Morgan (moggieuk@hotmail.com)