Add force send parameter to setOnOffValue and send it on matter start #35287
Conversation
This file contains 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
|
Review changes with SemanticDiff. |
|
|
bzbarsky-apple
requested changes
Aug 29, 2024
| @@ -340,8 +340,11 @@ Status OnOffServer::getOnOffValue(chip::EndpointId endpoint, bool * currentOnOff | |||
| * @param endpoint Ver.: always | |||
| * @param command Ver.: always | |||
| * @param initiatedByLevelChange Ver.: always | |||
| * @param forceSend Send value of the On/Off even when it is the same as current On/Off value. | |||
There was a problem hiding this comment.
Send what and where?
At startup, if some hardware state needs to be synced to the state of the On/Off cluster, the right sequence of actions is:
- Start up the Matter stack, which initializes the On/Off cluster instance(s).
- Read the relevant attributes from those cluster instances (which might involve the StartUpOnOff value, or might involve reading the persisted OnOff state from flash, or some combination thereof).
- Sync the hardware state to the desired state as represented by the attributes.
There was a problem hiding this comment.
I think name is a bit incorrect and should be something like force_ember.
What you proposed as syncing of the state will work but it causes duplicate of logic on matter side, I changed it to wait for matter to be started and get this value. I will discard the pr
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I sent this PR based on matter implementation for esp-matter it might work differently in other SDKs.
This change is useful and required for an onoff device controlled via NC/NO relay.
Since matter controls the startup parameter via "onoff write start-up-on-off" parameter. Connectedhomeip is the only place where the startup relay state can be determined. The current implementation's problem is that it only provides information about the startup relay state when it is different from the last known state before restart e.g.
start-up-on-off is set to On when the last state of OnOff parameter is Off CHIP sends information about state change,
start-up-on-off is set to On when the last state of OnOff parameter is On CHIP won't send information about the state of OnOff parameter
I think this makes it inconsistent and for me sending the state of OnOff parameter always at the start is the best way to go about it, so information about the startup state always comes from on/off server