Fix NetworkCommissioning Cluster issues #32156
Merged
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
This PR addresses several NetworkCommissioning issues which are all inter-related: - Attributes are never reported on change. - ScanNetworks used to modify attributes that should not be modified - LastNetworkStatus was not set where required - Thread network scanning did not report error on SSID field provided - All ConstraintErrors actually were reported as InvalidCommand - Lacking results of directed scanning did not report NetworkNotFound - Fixes project-chip#32024 - Fixes project-chip#32022 - Fixes project-chip#32021 - Fixes project-chip#32019 - Fixes project-chip#32018 - Fixes project-chip#31870 Testing done: - TC-CNET-4.4 pass on ESP32 and Linux - Commissioning works on ESP32 and Linux - Manual testing of all attribute changes validated with chip-repl - Automated test beyond TC-CNET-4.4 coming as a follow-up
tcarmelveilleux
added
commissioning
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple and
kkasperczyk-no
|
PR #32156: Size comparison from a0f446b to 5775cd6 Increases (1 build for stm32)
Decreases (1 build for cc32xx)
Full report (3 builds for cc32xx, stm32)
|
|
PR #32156: Size comparison from a0f446b to b0ba574 Increases (23 builds for bl602, bl702, bl702l, cc13x4_26x4, k32w, mbed, qpg, stm32)
Decreases (1 build for cc32xx)
Full report (26 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, k32w, mbed, qpg, stm32)
|
yunhanw-google
approved these changes
Feb 15, 2024
andy31415
reviewed
Feb 15, 2024
andy31415
reviewed
Feb 15, 2024
andy31415
reviewed
Feb 15, 2024
andy31415
reviewed
Feb 15, 2024
andy31415
reviewed
Feb 15, 2024
src/app/clusters/network-commissioning/network-commissioning.cpp
Outdated
Show resolved
Hide resolved
jmartinez-silabs
approved these changes
Feb 15, 2024
examples/energy-management-app/energy-management-common/energy-management-app.zap
Show resolved
Hide resolved
andy31415
reviewed
Feb 15, 2024
src/app/clusters/network-commissioning/network-commissioning.cpp
Outdated
Show resolved
Hide resolved
|
PR #32156: Size comparison from a0f446b to 42308b6 Increases above 0.2%:
Increases (32 builds for bl602, bl702, bl702l, cc13x4_26x4, cyw30739, k32w, linux, mbed, psoc6, qpg, stm32)
Decreases (5 builds for cc32xx, psoc6)
Full report (36 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, k32w, linux, mbed, psoc6, qpg, stm32)
|
andy31415
reviewed
Feb 15, 2024
src/app/clusters/network-commissioning/network-commissioning.cpp
Outdated
Show resolved
Hide resolved
|
PR #32156: Size comparison from a0f446b to 34d7b9f Increases above 0.2%:
Increases (66 builds for bl602, bl702, bl702l, cc13x4_26x4, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
Decreases (5 builds for cc32xx, psoc6)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
|
This was referenced Feb 16, 2024
andy31415
reviewed
Feb 16, 2024
| CtorDtorCounter(const CtorDtorCounter & o) : m(o.m) { ++created; } | ||
| CtorDtorCounter & operator=(const CtorDtorCounter &) = default; | ||
|
|
||
| CtorDtorCounter(CtorDtorCounter && o) : m(o.m) { ++created; } |
There was a problem hiding this comment.
@tcarmelveilleux I still believe this is a bug ....
There was a problem hiding this comment.
This is a bug that exists in Optional, and is a bigger issue. Can you please raise an issue and we will address holistically.
| @@ -426,16 +477,27 @@ void Instance::HandleScanNetworks(HandlerContext & ctx, const Commands::ScanNetw | |||
| } | |||
| if (ssid.size() > DeviceLayer::Internal::kMaxWiFiSSIDLength) | |||
| { | |||
| ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand); | |||
| // This should not happen, it means it's a broken driver. | |||
There was a problem hiding this comment.
It's a broken client, right? This is not a driver bug; it's a client sending bogus data. The error return is correct, but this comment seems like it should be fixed.
| mCurrentOperationBreadcrumb = req.breadcrumb; | ||
| mAsyncCommandHandle = CommandHandler::Handle(&ctx.mCommandHandler); | ||
| ctx.mCommandHandler.FlushAcksRightAwayOnSlowCommand(); | ||
| mpDriver.Get<WiFiDriver *>()->ScanNetworks(ssid, this); | ||
| } | ||
| else if (mFeatureFlags.Has(Feature::kThreadNetworkInterface)) | ||
| { | ||
| // Not allowed to populate SSID for Thread. | ||
| if (!req.ssid.HasValue()) |
There was a problem hiding this comment.
Isn't this check backwards?
| // Not allowed to populate SSID for Thread. | ||
| if (!req.ssid.HasValue()) | ||
| { | ||
| ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError); |
There was a problem hiding this comment.
So "Invoke Execution" in the core spec says:
If a mandatory data field is missing, or incoming data cannot be mapped to the expected data type for a field, a CommandStatusIB SHALL be generated with an error status of INVALID_COMMAND, even if the cluster defines another type of response.
If a data field violates expected constraints, a CommandStatusIB SHOULD be generated with an error status of CONSTRAINT_ERROR.
There's no provision there for "field is provided that should not have been", but that seems closer to the InvalidCommand case than the "the data does not fit constraints" case, maybe.
I don't feel too strongly about this, but maybe we should fix the spec to cover this case...
There was a problem hiding this comment.
Will move to InvalidCommand
| @@ -497,7 +559,7 @@ void Instance::HandleAddOrUpdateWiFiNetwork(HandlerContext & ctx, const Commands | |||
| return; | |||
| } | |||
| #endif // CHIP_DEVICE_CONFIG_ENABLE_WIFI_PDC | |||
| ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand); | |||
| ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError); | |||
There was a problem hiding this comment.
As above, it's not clear whether this should in fact be ConstraintError or InvalidCommand, and InvalidCommand is arguably better.
| @@ -1110,6 +1201,12 @@ void Instance::OnFailSafeTimerExpired() | |||
| ChipLogDetail(Zcl, "Failsafe timeout, tell platform driver to revert network credentials."); | |||
| mpWirelessDriver->RevertConfiguration(); | |||
| mAsyncCommandHandle.Release(); | |||
|
|
|||
| // Reset state on failsafe expiry. | |||
There was a problem hiding this comment.
I don't see a spec basis for this...
Furthermore, if the failsafe was for something totally unrelated to commissioning or networks (e.g. an UpdateNOC), clearing this state doesn't obviously seem like the right thing to do.
There was a problem hiding this comment.
After Commissioning or reconfiguration ends in failure due to expiry of the Fail-Safe timer, the Node SHALL revert to the network configuration present prior to the Fail-Safe timer being armed.
I will just mark Networks list dirty. The other attributes update over time.
tcarmelveilleux
added a commit
to tcarmelveilleux/connectedhomeip
that referenced
this pull request
Feb 16, 2024
- Found a regression on Thread scanning. - Changed some ConstraintError to InvalidCommand where more applicable. - Removed an update of cluster state on fail safe expiry. Testing done: - Retested on Wi-Fi - Testing on Thread as well
mergify bot
pushed a commit
that referenced
this pull request
Feb 16, 2024
* Fix NetworkCommissioning post-review from #32156 - Found a regression on Thread scanning. - Changed some ConstraintError to InvalidCommand where more applicable. - Removed an update of cluster state on fail safe expiry. Testing done: - Retested on Wi-Fi - Testing on Thread as well * Re-notify errors on empty network at fail-safe expiry * Fix MobileDeviceTest * Fix Cirque tests
github-actions bot
pushed a commit
that referenced
this pull request
Feb 16, 2024
* Fix NetworkCommissioning post-review from #32156 - Found a regression on Thread scanning. - Changed some ConstraintError to InvalidCommand where more applicable. - Removed an update of cluster state on fail safe expiry. Testing done: - Retested on Wi-Fi - Testing on Thread as well * Re-notify errors on empty network at fail-safe expiry * Fix MobileDeviceTest * Fix Cirque tests
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.
This PR addresses several NetworkCommissioning issues which are all inter-related:
All fixed issues:
ssidfield #32019NetworkNotFoundError#31870Testing done: