Fix ClosureControl Calibration/MotionLatching/Speed opt-in leaks - #623
Closed
lboue wants to merge 14 commits into
Closed
Fix ClosureControl Calibration/MotionLatching/Speed opt-in leaks#623lboue wants to merge 14 commits into
lboue wants to merge 14 commits into
Conversation
…ce cluster on the Root endpoint
… when called with `Behavior.Type` or `ClusterType`.
MotionLatching and Speed were unconditionally enabled on every ClosureDimension panel created via Closure.addPanel(), with no way to opt out even though both are optionalConform features in the Matter 1.5 data model (not implied by Positioning). This meant a bridged device with no real latch mechanism and no variable-speed motor still advertised both to Matter controllers, which could let a controller set a target latch state or motion speed that the underlying device silently ignored. Add motionLatching?/speed? to ClosurePanelOptions (default true, matching current behavior) and use them to build the enabled ClosureDimension feature set per panel, mirroring how Closure already makes Calibration, Ventilation and Pedestrian opt-in on ClosureControl. MatterbridgeClosureDimensionServer now only declares Positioning at the class level; access to the MotionLatching-only LatchControlModes attribute is guarded on this.features.motionLatching and cast, the same pattern MatterbridgeFanControlServer uses for its own MultiSpeed-only attributes. The default currentState/ targetState and the Speed field written by setTarget are now conditional on the corresponding feature, since matter.js's runtime conformance validator rejects a Latch/Speed field when the feature is disabled. Fixes Luligu#618 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r review Per Luligu's review on PR Luligu#619: the class itself should always declare the full feature set a server's command handlers need typed access to (as MatterbridgeClosureControlServer already does for Calibration), and per-instance opt-out belongs at the require() call site via a narrower .with(...), not at the class declaration. MatterbridgeClosureDimensionServer now extends ClosureDimensionServer.with(Positioning, MotionLatching, Speed) instead of just Positioning, so this.state.latchControlModes is natively typed and no longer needs the unsafe cast setTarget() used to read it. The this.features.motionLatching guard stays, since that attribute is still absent from the actual cluster at runtime for panels createClosureDimensionClusterServer() requires without the feature. createClosureDimensionClusterServer() already built its per-panel .with(...) feature set from the motionLatching/speed options, so its behavior is unchanged; 46/46 tests across closurePanel.test.ts, closure.test.ts, closure-dimension.test.ts and closure-control.test.ts still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…wer Calibration/Ventilation/Pedestrian .with(...) Closure's ClosureControl setup fell back to the bare MatterbridgeClosureControlServer whenever calibration, ventilation, and pedestrian were all left at their documented false default. That class extends MatterbridgeClosureControlServerBase = ClosureControlServer.with(Positioning, MotionLatching, Speed, Calibration), so the endpoint's actual featureMap still reported calibration: true regardless of the option - confirmed by the core's own forEachAttribute snapshot test, which built a Closure with zero options and asserted calibration: true. Per Luligu's review on PR Luligu#619 (see the commit fixing the equivalent ClosureDimension MotionLatching/Speed case, c389952): the class should always declare the full feature set for typed access, and per-instance opt-out belongs at the require() call site via a narrower .with(...), never by falling back to the bare class. Applied that same pattern here: the enabled feature set is now always restated explicitly at the require() call, dropping the ternary's bare-class branch entirely. Calibration is optionalConform in the Matter 1.5 data model (gated on Positioning only, never mandatory), so defaulting it off is spec-compliant. Updated the locked forEachAttribute snapshot to the corrected calibration: false / acceptedCommandList without the calibrate command id. Fixes Luligu#621 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
MotionLatching and Speed were unconditionally enabled on every Closure's ClosureControl cluster, with no ClosureOptions flag to opt out - unlike Calibration/Ventilation/Pedestrian, which already are opt-in. A plugin bridging a real-world closure with no physical latch and no controllable motion speed still advertised both to Matter controllers, and MatterbridgeClosureControlServer.moveTo() had no way to reject a Latch/Speed field once the corresponding feature was actually disabled. Add motionLatching?/speed? to ClosureOptions (default true, matching current behavior). The Closure constructor now restates the enabled feature set explicitly (mirroring the calibration/ventilation/pedestrian pattern) and only includes latchControlModes and the Latch/Speed fields of the default OverallCurrentState/OverallTargetState when the corresponding feature is enabled - both are mandatoryConform on their own feature per the data model. MatterbridgeClosureControlServer.moveTo()/completeMoveTo() now guard every Latch/Speed read or write on this.features.motionLatching/speed: a Latch/Speed field on a MoveTo request is rejected with CONSTRAINT_ERROR once the feature is disabled, nextTarget/currentState only carry the field when supported, and OverallCurrentState.SecureState is derived from Position alone when MotionLatching is disabled (per §5.4.6.5.4, unchanged from the existing latch-only derivation when MotionLatching stays enabled). setFullyClosed/setFullOpened/setPartiallyOpened read the cluster's own featureMap so they only ever write the fields the enabled features support. Fixes Luligu#622 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
2 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.
Summary
Two related fixes to
Closure'sClosureControlcluster setup inpackages/core/src/devices/closure.ts, both following the same methodology: the exported server class declares the full feature set for typed access, but therequire()call site must always restate the enabled feature set explicitly - never fall back to the bare class, which silently inherits its full declared set regardless of options.1. Calibration was always enabled, contradicting its documented
falsedefault (fixes #621)Closure's ClosureControl setup fell back to the bareMatterbridgeClosureControlServerwhenevercalibration/ventilation/pedestrianwere all left at their defaultfalse. That class extends a base declaringCalibrationat the class level, so the endpoint's actualfeatureMapstill reportedcalibration: trueregardless of the option - confirmed by the core's ownforEachAttributesnapshot test. The enabled feature set is now always restated explicitly at therequire()call, dropping the bare-class fallback entirely.2. MotionLatching/Speed had no opt-out at all (fixes #622)
Unlike Calibration/Ventilation/Pedestrian,
ClosureOptionshad nomotionLatching/speedflags - both features were unconditionally on, andMatterbridgeClosureControlServer.moveTo()had no way to reject a Latch/Speed field once a plugin's real device had neither. AddedmotionLatching?/speed?(defaulttrue, preserving current behavior). The constructor now only includeslatchControlModesand the Latch/Speed fields of the defaultOverallCurrentState/OverallTargetStatewhen the corresponding feature is enabled (both aremandatoryConformon their own feature per the Matter 1.5 data model).moveTo()/completeMoveTo()guard every Latch/Speed read or write onthis.features.motionLatching/speed: a Latch/Speed field on a MoveTo request is rejected withCONSTRAINT_ERRORonce the feature is disabled, andOverallCurrentState.SecureStateis derived from Position alone when MotionLatching is disabled (§5.4.6.5.4), unchanged from the existing latch-only derivation when MotionLatching stays enabled.setFullyClosed/setFullOpened/setPartiallyOpenednow read the cluster's own featureMap so they only ever write fields the enabled features support.Spec check
Per the Matter 1.5 data model (
ClosureControl.xml), Calibration/Ventilation/Pedestrian/Speed are alloptionalConform, gated only on Positioning; Positioning and MotionLatching form a choice group requiring at least one of the two (not both). So both fixes are spec-compliant defaults-preserving changes.Testing
packages/core/vitest/devices/closure.test.ts,closurePanel.test.ts,packages/core/vitest/clusters/closure-control.test.ts,closure-dimension.test.ts- 48 tests pass.packages/core/vitest/devices/full suite - 339 tests pass.matterbridge-somfy-tahomaplugin (which triggered this investigation - itsClosureendpoints never actually gained the opt-outs they configured, due to bug Update dev #1) - 44 tests pass after rebuild.🤖 Generated with Claude Code