Valve cluster: refactor to make unit-testable #35631
Draft
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
Sepearate delegates for level control and non-level control valves, add unit tests and handlers.
|
PR #35631: Size comparison from 85b2fd3 to 83d5b79 Full report (82 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #35631: Size comparison from 85b2fd3 to 100a967 Increases above 0.2%:
Full report (82 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Sep 26, 2024
|
|
||
| struct ClusterInitParameters | ||
| { | ||
| DataModel::Nullable<ValveStateEnum> currentState = DataModel::NullNullable; |
There was a problem hiding this comment.
Are these = needed? Nullable should default to null/missing I believe.
| DataModel::Nullable<Percent> currentLevel = DataModel::NullNullable; | ||
| BitMask<ValveFaultBitmap> valveFault = 0u; | ||
| uint8_t levelStep = 1u; | ||
| }; |
There was a problem hiding this comment.
It may be nice to make this fluent initializable, so I can write:
Init(ClusterInitParameters().withValveFault(10).withLevelStep(100))
| public: | ||
| explicit ClusterStateAttributes(MatterContext & matterContext) : mMatterContext(matterContext) {}; | ||
| void Init(ClusterInitParameters initialState); | ||
| const ClusterState & GetState() { return mState; } |
There was a problem hiding this comment.
Suggested change
| const ClusterState & GetState() { return mState; } | |
| const ClusterState & GetState() const { return mState; } |
| CHIP_ERROR SetValveFault(const BitMask<ValveFaultBitmap> & valveFault); | ||
| CHIP_ERROR SetLevelStep(const uint8_t levelStep); | ||
|
|
||
| System::Clock::Milliseconds64 GetNextReportTimeForRemainingDuration(); |
There was a problem hiding this comment.
Needs a comment explaining use: it seems not immediately obvious what this time represents and its usage.
| class ClusterStateAttributes | ||
| { | ||
| public: | ||
| explicit ClusterStateAttributes(MatterContext & matterContext) : mMatterContext(matterContext) {}; |
There was a problem hiding this comment.
Could we describe the lifetime requirements of the MatterContext? It seems we take a reference, so we have to ensure this context is valid for the duration of this object existance.
| { | ||
| mState.defaultOpenDuration.SetNonNull(defaultOpenDuration); | ||
| } | ||
| mMatterContext.GetDefaultOpenLevel(mState.defaultOpenLevel); |
There was a problem hiding this comment.
This seems to also return CHIP_ERROR that needs to be handled
| if (mMatterContext.GetDefaultOpenDuration(defaultOpenDuration) == CHIP_NO_ERROR) | ||
| { | ||
| mState.defaultOpenDuration.SetNonNull(defaultOpenDuration); | ||
| } |
There was a problem hiding this comment.
in case of error, the error seems to be thrown away. I think we should log instead of silently ignoring the error.
Comment on lines
+69
to
+75
| bool dirty = openDuration != mState.openDuration; | ||
| mState.openDuration = openDuration; | ||
| if (dirty) | ||
| { | ||
| mMatterContext.MarkDirty(Attributes::OpenDuration::Id); | ||
| } | ||
| return CHIP_NO_ERROR; |
There was a problem hiding this comment.
Suggested change
| bool dirty = openDuration != mState.openDuration; | |
| mState.openDuration = openDuration; | |
| if (dirty) | |
| { | |
| mMatterContext.MarkDirty(Attributes::OpenDuration::Id); | |
| } | |
| return CHIP_NO_ERROR; | |
| ReturnErrorCodeIf(openDuration == mState.openDuration, CHIP_NO_ERROR); | |
| mState.openDuration = openDuration; | |
| mMatterContext.MarkDirty(Attributes::OpenDuration::Id); | |
| return CHIP_NO_ERROR; |
I think this pattern could be applied everywher: it seems if we get passed in a NOOP we can early-return.
There was a problem hiding this comment.
+1 on that pattern.
Also, would be great to have a AttributeValue<T> class that can has a SetValue(const T& val, TheInterfaceThatHasMarkDirty *) which then we could have:
return mOpenDuration.SetValue(openDuration, mMatterContext)
| { | ||
| VerifyOrReturnError(mInitialized, CHIP_ERROR_INCORRECT_STATE); | ||
| VerifyOrReturnError(mConformance.supportsDefaultOpenLevel, CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE); | ||
| if (defaultOpenLevel < 1 || defaultOpenLevel > 100) |
There was a problem hiding this comment.
is level always a percentage? Would it make sense to name things Percent as a suffix then?
Unit tests included, also tested on an example app with mfg tags
If the valve can be opened before the command is complete or before the next read is done, the test will fail because it expects the transitioning phase to happen. In the spec: When the movement is complete, the device SHALL set the CurrentState attribute to the Open value. ^ this can happen before the end of the command.
This way we can ensure the open duration is writeable. Also add a cleanup step.
Self-select the valve tests off the device information rather than the PICS. This makes it easier to run the tests since you can just run them all unconditionally and they will run if required.
github-actions
bot
added
the
matter-1.4-te2-script-change
|
PR #35631: Size comparison from f509f67 to eb2dcbf Full report (22 builds for cc13x4_26x4, cc32xx, nrfconnect, nxp, qpg, stm32, tizen)
|
|
PR #35631: Size comparison from f509f67 to 3e30568 Full report (20 builds for cc13x4_26x4, cc32xx, nrfconnect, nxp, qpg, stm32, tizen)
|
examples/valve/controller/README.md
Outdated
| await devCtrl.CommissionOnNetwork(nodeId=1, setupPinCode=20202021, filterType=ChipDeviceCtrl.DiscoveryFilterType.LONG_DISCRIMINATOR, filter=3840) | ||
| ``` | ||
|
|
||
| ### Interacting with teh valve app |
There was a problem hiding this comment.
Suggested change
| ### Interacting with teh valve app | |
| ### Interacting with the valve app |
| class NonLevelValveEndpoint | ||
| { | ||
| public: | ||
| NonLevelValveEndpoint(EndpointId endpoint) : |
There was a problem hiding this comment.
should we inject the kvsStorage here
| } | ||
|
|
||
| private: | ||
| const ClusterConformance kConformance = { |
There was a problem hiding this comment.
this ignores generated code
There was a problem hiding this comment.
yes, but that's not an oversight, it's hoping to be forward looking. ie, the hope is that we can move away from statically generated conformances for clusters.
Generally though, the question of where to set the conformance is a valid one - @andy31415 - with your work on the IM layer, where would you prefer to set the conformance at the cluster, or have this come from the IM layer?
I think the clusters still need to understand their conformance, even if it is not static, because they need to be able to call into the delegates correctly and understand the responses. But should the cluster logic be responsible for this type of error handling, or should the cluster logic assume that if the IM layer is asking to ex. read or write an arrtibute, that it is allowed?
| { | ||
| inline bool HasFeature(Feature feature) const { return featureMap & to_underlying(feature); } | ||
| uint32_t featureMap; | ||
| bool supportsDefaultOpenLevel; |
There was a problem hiding this comment.
feature map and all following bool supports should be a bitfield
maybe something like this
union {
struct {
bool supportsDefaultOpenLevel: 1;
bool supportsValveFault: 1;
bool supportsLevelStep: 1;
};
uint32_t featureMap;
};
I'm not sure that it is portable tho. Maybe Using bitflag and enum type would be best
tehampson
reviewed
Oct 15, 2024
| /** @brief | ||
| * Interface to allow interaction with interaction model and ember layers. Can be faked for unit testing. | ||
| */ | ||
| class MatterContext |
There was a problem hiding this comment.
Just an idea that there could be a Base MatterContext that is common to all clusters (like MarkDirty) That would mean what you have here would need to be a little more generic to accept EndpointId and ClusterId, but it could later allow for things like that to funnel into one central command MatterContext that is shared everywhere to reduce boiler plate
tehampson
reviewed
Oct 15, 2024
| ~MockedMatterContext() { gSystemLayerAndClock.Clear(); } | ||
|
|
||
| private: | ||
| // Won't handle double-marking an attribute, so don't do that in tests |
There was a problem hiding this comment.
nit, not sure I follow this comment. This does supposed double-marking an attribute you would see it come up twice in this vector
Comment on lines
+44
to
+48
| CHIP_ERROR TranslateErrorToIMStatus(CHIP_ERROR err) | ||
| { | ||
| if (err == CHIP_NO_ERROR) | ||
| { | ||
| return err; |
There was a problem hiding this comment.
This doesn't feel like it belongs in this cluster.
There was a problem hiding this comment.
We may want to eventually use https://github.com/project-chip/connectedhomeip/blob/master/src/app/data-model-provider/ActionReturnStatus.h#L44
Beyond that though, translating a CHIP_ERROR to another CHIP_ERROR feels odd.
Comment on lines
+68
to
+79
| CHIP_ERROR EncodeRead(AttributeValueEncoder & aEncoder, const F & getter) | ||
| { | ||
| T ret; | ||
| CHIP_ERROR err = getter(ret); | ||
| if (err == CHIP_NO_ERROR) | ||
| { | ||
| err = aEncoder.Encode(ret); | ||
| } | ||
|
|
||
| // TODO: Should the logic return these directly? I didn't want to mix the IM layer into there, but this is annoying. | ||
| return TranslateErrorToIMStatus(err); | ||
| } |
There was a problem hiding this comment.
This really feels like the translate to IM status could be combined, adn also all the getters could return values rather than CHIP_ERROR. Overall, the presence of the attribute should be determinable before we get here.
3 tasks
Testing: TC_TestArrtAvail.py
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.
Refactor of Valve to enable unit testing of the cluster logic per the proposal in https://project-chip.github.io/connectedhomeip-doc/cluster_and_device_type_dev/unit_testing_clusters.html.
Unit tests are in TestValveConfigurationAndControl
General outline
Server implementation receives TLV, calls into the cluster logic for all read/write/command operations. Server is only responsible for encode / decode, error translation and installing itself into the registry. Uses AttributeAccessInterface and CommandHandlerInterface
Cluster logic
Matter context handles attribute reporting (and will handle event logging once I implement it). This can be mocked for testing.
Timers and clocks are injected by the test at the system layer (not the matter context) because there is a single clock for the entire system.
App owns all the resources - delegate, matter context, logic and server and initializes them as part of the app startup
one of each is instantiated once for each endpoint
Open questions on the general design:
Open questions for the valve:
Remaining:
event generation
AutoCloseTime handling
handling of async opens / closes
set functions for things like valve fault
command handling for > EP1 seems to be broken, still need to fix that.