[NWPROV] Fix spec compliance issues #17038
Conversation
pullapprove
bot
requested review from
andy31415,
anush-apple,
austinh0,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
hawk248,
holbrookt,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs and
jmartinez-silabs
pullapprove
bot
requested review from
sagar-apple,
saurabhst,
selissia,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yufengwangca
| using ClusterObject = NetworkCommissioning::WiFiSecurity; | ||
| using PlatformInterface = NetworkCommissioning::WiFiSecurity; |
There was a problem hiding this comment.
These are the same type, so the asserts below are trivially going to pass.
What I would suggests is that we get rid of DeviceLayer::NetworkCommissioning::WiFiSecurity and include cluster-enums.h in the relevant places so we can use Clusters::NetworkCommissioning::WiFiSecurity directly.
There was a problem hiding this comment.
This is discuessed before, and we want to avoid using cluster-enums in platform code.
There was a problem hiding this comment.
cluster-enums was specifically created so it can be used in platform code. Not to be confused with non-ABI-stable cluster-objects.
There was a problem hiding this comment.
+1 cluster-enums.h is usable in platform layer and was introduced to avoid this
| if (!nullableSSID.IsNull()) | ||
| { | ||
| ssid = nullableSSID.Value(); | ||
| } |
There was a problem hiding this comment.
This should enforce the "1 to 32" length constraint from the spec.
| mAsyncCommandHandle = CommandHandler::Handle(&ctx.mCommandHandler); | ||
| mpDriver.Get<WiFiDriver *>()->ScanNetworks(req.ssid, this); | ||
| mpDriver.Get<WiFiDriver *>()->ScanNetworks(ssid, this); |
There was a problem hiding this comment.
Unfortunately, the driver interface is not documented very well, so the implementations of it differ in how they handle the SSID. For example, src/platform/Ameba/NetworkCommissioningWiFiDriver.cpp checks ssid.data() to decide whether we're doing a single-ssid scan or general scan, while src/platform/EFR32/NetworkCommissioningWiFiDriver.cpp checks ssid.size(). If we checked the size here the difference would at least not be observable, but as it is they would reply differently to the same protocol message.
My recommendation would be to change the driver API to have separate functions for "scan for networks with the given SSID" and "scan for all networks" so the only place where we have to make that decision is here.
There was a problem hiding this comment.
#17060 filed on this
| memcpy(mLastNetworkID, mConnectingNetworkID, mLastNetworkIDLen); | ||
| mLastNetworkingStatusValue.SetNonNull(ToClusterObjectEnum(commissioningError)); | ||
|
|
||
| if (debugText.size() != 0) |
There was a problem hiding this comment.
Should be using empty(), really.
|
|
||
| SuccessOrExit(err = commandHandle->PrepareCommand( | ||
| ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id))); | ||
| VerifyOrExit((writer = commandHandle->GetCommandDataIBTLVWriter()) != nullptr, err = CHIP_ERROR_INCORRECT_STATE); | ||
|
|
||
| SuccessOrExit(err = writer->Put(TLV::ContextTag(to_underlying(Commands::ScanNetworksResponse::Fields::kNetworkingStatus)), | ||
| ToClusterObjectEnum(status))); | ||
| SuccessOrExit(err = DataModel::Encode( | ||
| *writer, TLV::ContextTag(to_underlying(Commands::ScanNetworksResponse::Fields::kDebugText)), debugText)); | ||
| if (debugText.size() != 0) |
| @@ -436,28 +468,33 @@ void Instance::OnFinished(Status status, CharSpan debugText, ThreadScanResponseI | |||
| TLV::TLVType listContainerType; | |||
| ThreadScanResponse scanResponse; | |||
| size_t networksEncoded = 0; | |||
| uint8_t extendedAddressBuffer[8]; | |||
There was a problem hiding this comment.
That 8 should be a constant defined somewhere. I would be very surprised if we do not have a constant for this already.
* Fix spec compliance of network commissioning cluster * Update existing platform code * Run Codegen
* Fix spec compliance of network commissioning cluster * Update existing platform code * Run Codegen
* [nwprov] Resolve comments in #17038 add more checks * Remove enums in include/platform/NetworkCommissioning.h use ssid.empty() instead of ssid.size() * Fix template * Regen
* Fix spec compliance of network commissioning cluster * Update existing platform code * Run Codegen
…ect-chip#17197) * [nwprov] Resolve comments in project-chip#17038 add more checks * Remove enums in include/platform/NetworkCommissioning.h use ssid.empty() instead of ssid.size() * Fix template * Regen
Problem
Fixes #17010
Some definitions in network commissioning cluster does not match the spec
Change overview
Testing
Build should pass.