[Closure Dimension] The §5.5.4 association is a TODO, the delegate's status is flattened to FAILURE, and SetTarget/Step validate after commanding motion
Verified on master (99a81bd3298). All in src/app/clusters/closure-dimension-server/ unless stated.
| # | What | Why it matters |
|---|---|---|
| 1 | The §5.5.4 association with Closure Control is a TODO | Every integrator has to rewrite the state gate in application code |
| 2 | The delegate's status is flattened to FAILURE | A delegate cannot get INVALID_IN_STATE or CONSTRAINT_ERROR to the client |
| 3 | SetTarget omits §5.5.8.1.4's "all fields match CurrentState ⇒ no effect" rule |
The closure is commanded to move when it is already where it was asked to be |
| 4 | SetTarget rejects latch-only and speed-only commands with INVALID_IN_STATE |
A speed change on an uncalibrated closure is refused, against §5.5.8.1.3 |
| 5 | Step commands motion and then answers FAILURE |
The closure moves while the client is told the command failed |
| 6 | Resolution == 0 is never rejected |
A misconfigured app divides by zero |
The shape, stated once. Items 3 and 5 are one pattern, the same one reported for Closure Control in https://github.com/project-chip/connectedhomeip/issues/74260: the handler acts before the rule that should have stopped it runs. SetTarget never compares the command against CurrentState, and Step calls the delegate before the Resolution-multiple rule is checked in the setter. The fix worth making is the ordering rule rather than four patches: validate every field, compare against the current state, and only then call the delegate and move.
Item 4 is the opposite failure and is deliberately not folded into that framing. Its check runs before the delegate call and rejects commands the specification says are valid.
1 — The §5.5.4 association with Closure Control is a TODO
ClosureDimensionCluster.cpp:463-464 (SetTarget) and :585-586 (Step) carry TODOs where the MainState gate belongs:
// TODO: If this command is sent while the closure is in a non-compatible internal-state, a status code of …There is no MainState or Closure Control reference anywhere in the cluster, and ClosureDimensionClusterDelegate.h exposes only HandleSetTarget/HandleStep — no hook by which the cluster could ask.
Why: §5.5.4's requirement that Dimension commands work with Closure Control's state is satisfied only by application code. The only implementation lives in the example app, examples/closure-app/linux/ClosureManager.cpp (OnSetTargetCommand, and the SetOverallTargetState / SetCountdownTimeFromDelegate / SetMainState(kMoving) write-back), so every integrator has to rewrite it.
2 — The delegate's status is flattened to FAILURE
:555 and :656:
VerifyOrReturnError(mDelegate.HandleSetTarget(position, latch, speed) == Status::Success, Status::Failure);Why: any status the delegate returns other than SUCCESS becomes FAILURE, so a delegate that correctly answers INVALID_IN_STATE or CONSTRAINT_ERROR cannot get it to the client. The controller loses the reason the command was refused.
3 — SetTarget omits §5.5.8.1.4's "all fields match CurrentState ⇒ no effect" rule
HandleSetTargetCommand spans :457-560 and never compares the command's Position, Latch or Speed against currentState. Control falls through to the delegate at :555 and SetTargetState at :557.
Why: a command that asks for the state the closure is already in still drives the delegate and the target state, where the specification requires no effect.
4 — SetTarget rejects latch-only and speed-only commands with a status the specification does not define for it
:530-537 — VerifyOrReturnError(!currentState.IsNull(), Status::InvalidInState) fires unconditionally, including for commands carrying only Latch or only Speed. §5.5.8.1.4 defines exactly two INVALID_IN_STATE conditions — the MainState-list gate and the latch-blocks-position-change gate — and a null CurrentState is neither.
Why: it also defeats §5.5.8.1.3's "allow speed change on the fly". On an uncalibrated closure a speed-only command is refused rather than applied, so a controller cannot change speed until calibration completes.
The instinct is reasonable, since an uncalibrated closure arguably cannot honour a position target. The narrower gate is to require a non-null CurrentState only for the fields that need it. If the broad gate is preferred, then §5.5.8.1.4 should gain the condition. Either outcome is fine, but one stack enforcing a rejection the specification does not describe is not.
5 — Step commands motion and then answers FAILURE
mDelegate.HandleStep(...) at :656 precedes SetTargetState(stepTarget) at :659, and the Resolution-multiple rule is enforced in the setter. SetCurrentState applies no % resolution check at all, so the currentPosition that Step derives its target from is read unrounded.
Why: the closure is told to move, and then the command reports FAILURE. The controller and the hardware end up in different states.
Related specification gap: the Step formula states clamping but nothing about rounding the result onto the Resolution grid, while TargetState.Position separately SHALL be an integer multiple of Resolution. Filed as https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/13474.
6 — Resolution == 0 is never rejected
Config::WithPositioning (ClosureDimensionCluster.h) validates neither resolution nor stepValue.
Why: a misconfigured app can install Resolution = 0, and every % resolution check downstream then divides by zero and crashes the device.
Found during a Matter 1.6.0 specification-versus-implementation review.
Source: project-chip/connectedhomeip