Delegate implementation for Valve Configuration and control cluster. Tested with watervalve CHEF device. #37088
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
… handleOpenValve, kLevel (0x2) has to be set in feature bitmap for Open command to set the right currentLevel.
|
Review changes with |
|
|
andy31415
reviewed
Jan 16, 2025
...app/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.h
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Jan 16, 2025
...sters/valve-configuration-and-control/chef-valve-configuration-and-control-delegate-impl.cpp
Show resolved
Hide resolved
andy31415
reviewed
Jan 16, 2025
...sters/valve-configuration-and-control/chef-valve-configuration-and-control-delegate-impl.cpp
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Jan 16, 2025
...sters/valve-configuration-and-control/chef-valve-configuration-and-control-delegate-impl.cpp
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Jan 16, 2025
...sters/valve-configuration-and-control/chef-valve-configuration-and-control-delegate-impl.cpp
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Jan 16, 2025
...p/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Jan 16, 2025
...p/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp
Outdated
Show resolved
Hide resolved
bzbarsky-apple
requested changes
Jan 16, 2025
...app/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.h
Outdated
Show resolved
Hide resolved
...p/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp
Outdated
Show resolved
Hide resolved
...p/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp
Outdated
Show resolved
Hide resolved
…urationNull * When reading remaining time, max numeric value for the given size (0xFFF...) is interpreted as NULL.
|
PR #37088: Size comparison from f9c7a8d to 4aa5e8a Full report (7 builds for cc13x4_26x4, cc32xx, stm32)
|
|
PR #37088: Size comparison from 292665e to 65b22c1 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Jan 24, 2025
| @@ -45,6 +45,9 @@ CHIP_ERROR SetValveLevel(chip::EndpointId ep, DataModel::Nullable<Percent> level | |||
| CHIP_ERROR UpdateCurrentLevel(chip::EndpointId ep, chip::Percent currentLevel); | |||
| CHIP_ERROR UpdateCurrentState(chip::EndpointId ep, ValveConfigurationAndControl::ValveStateEnum currentState); | |||
| CHIP_ERROR EmitValveFault(chip::EndpointId ep, chip::BitMask<ValveConfigurationAndControl::ValveFaultBitmap> fault); | |||
| CHIP_ERROR GetRemainingDuration(EndpointId endpoint, DataModel::Nullable<uint32_t> & duration); | |||
| CHIP_ERROR SetRemainingDuration(EndpointId endpoint, DataModel::Nullable<uint32_t> duration); | |||
| CHIP_ERROR SetRemainingDurationNull(EndpointId endpoint); | |||
There was a problem hiding this comment.
The fact that it existed internally does not seem like a good reason to make it public API.
The existing SetRemainingDuration can be used just fine, or we could have a ClearRemainingDuration method, I guess, if we really want a helper for this..
There was a problem hiding this comment.
I needed to access RemainingDurationTable gRemainingDuration[] from external delegate code. Thats why the APIs had to be exposed.
...app/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.h
Outdated
Show resolved
Hide resolved
| if (epIdx < kValveConfigurationAndControlDelegateTableSize) | ||
| { | ||
| gRemainingDuration[epIdx].endpoint = endpoint; | ||
| gRemainingDuration[epIdx].remainingDuration = duration; |
There was a problem hiding this comment.
Pre-existing, but isn't this missing the MatterReportingAttributeChangeCallback that should happen here?
Let's do a followup for this, because this should really be using QuieterReportingAttribute to get the right behavior. Right now this stuff is just totally not spec-compliant...
But that followup absolutely has to happen.
There was a problem hiding this comment.
I invoked MatterReportingAttributeChangeCallback at the caller side. But it makes more sense to call it here, if not onValveConfigurationAndControlTick would not report a matter attr change.
I'm now moving the MatterReportingAttributeChangeCallback from caller to inside this function. Let me know what you think.
...p/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp
Outdated
Show resolved
Hide resolved
|
|
||
| static DelegateImpl * gValveConfigurationAndControlDelegate = nullptr; | ||
|
|
||
| DataModel::Nullable<chip::Percent> DelegateImpl::HandleOpenValve(DataModel::Nullable<chip::Percent> level) |
There was a problem hiding this comment.
Strongly recommend making the argument a const ref, but won't block on that.
| case Attributes::RemainingDuration::Id: { | ||
| CHIP_ERROR err; | ||
| uint32_t * bufUint32 = reinterpret_cast<uint32_t *>(buffer); | ||
| if (NumericAttributeTraits<uint32_t>::IsNullValue(*bufUint32)) // Max value is interpreted as NULL |
There was a problem hiding this comment.
This can lead to unaligned access and crashes, no? I think you need to memcpy the data into an on-stack uint32_t here.
There was a problem hiding this comment.
Yes doing a *bufUint32 can lead to restricted memory access if the allocated buffer size is less than 4 bytes. Here are the reasons for doing it this way -
- PwRPC clients like RPC-console need to explicitly specify attribute type when calling read/write (INT32U in this case). So theres low chance of wrong data size buffer.
- This function will probably be deprecated after we implement a
Writemethod on theValveConfigAndControlAttrAccessclass.DataModel::Providertries to use AAI before ember callbacks. - I'd trust available libraries to take care of endianness.
| if (NumericAttributeTraits<uint32_t>::IsNullValue(*bufUint32)) // Max value is interpreted as NULL | ||
| { | ||
| ChipLogProgress(DeviceLayer, "Setting RemainingDuration to NULL."); | ||
| err = SetRemainingDurationNull(endpointId); |
There was a problem hiding this comment.
This is ... really broken. Why isn't the cluster handling this? This shouldn't need to be handled by the delegate.
And in fact, the cluster does not actually use the attribute store for this stuff at all, so when would this code even get invoked?
There was a problem hiding this comment.
Currently there is no Write implemented on ValveConfigAndControlAttrAccess class thats why we need this callback (DataModel::Provider will end up calling this if it didn't find the corresponding call on AAI).
The right way to go about this is to implement Write on ValveConfigAndControlAttrAccess and get rid of this function. However..., at the time I wrote this function, the Write function in rpc_services/Attributes.h directly called ember instead of doing it through DataModel::Provider. As a result AAI calls would be unused when write calls were made by RPC server. And thats why this ember function needed to be defined. This defect was fixed in PR: 37610.
I plan to remove this callback in a future PR. Ideally chef delegates shouldn't have to write ember callbacks at all. All externally managed attributes should be accessed through AAI, and AAI Read/Writes should be a part of the cluster code.
Since rpc_services didn't use DataModel::Provider before, we had to write the ember callbacks. Anyways now rpc_services deficiencies have been fixed by @andreilitvin .
...sters/valve-configuration-and-control/chef-valve-configuration-and-control-delegate-impl.cpp
Outdated
Show resolved
Hide resolved
|
|
||
| switch (attributeId) | ||
| { | ||
| case Attributes::RemainingDuration::Id: { |
There was a problem hiding this comment.
Was this tested? As far as I can tell, this is dead code that will never be reached, since the AttributeAccessInterface on the cluster will always handle this.
There was a problem hiding this comment.
Yes I tested this and it was never called. Even RPC has migrated to use DataModel::Provider, so RPC clients also don't need this anymore.
...sters/valve-configuration-and-control/chef-valve-configuration-and-control-delegate-impl.cpp
Outdated
Show resolved
Hide resolved
…configuration-and-control-server.h Co-authored-by: Boris Zbarsky <[email protected]>
|
PR #37088: Size comparison from ff2b0e4 to 48bad68 Full report (9 builds for cc13x4_26x4, cc32xx, qpg, stm32)
|
|
PR #37088: Size comparison from ff2b0e4 to 921d505 Full report (56 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
…chef-valve-configuration-and-control-delegate-impl.cpp Co-authored-by: Boris Zbarsky <[email protected]>
|
PR #37088: Size comparison from ff2b0e4 to e0a4669 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #37088: Size comparison from ff2b0e4 to 5f26b9d Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
|
PR #37088: Size comparison from ff2b0e4 to 1f3fb1c Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
@bzbarsky-apple below is the summary of changes since the last comment set -
|
|
PR #37088: Size comparison from ddf6e2f to 57e936d Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
tcarmelveilleux
requested changes
Jan 29, 2025
…to a read-only atribute
|
PR #37088: Size comparison from 95d5de5 to a7d0d39 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #37088: Size comparison from 94a47ad to 7a5ca25 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
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.
Adds a delegate implementation for valve configuration and control cluster intended to be used by chef devices using this cluster. Summary of changes -
RemainingTimeattribute which has only external storage option available.currentStateshould be set tokClosedin server or delegate.kLevelfeature for device.Testing with watervalve