Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nrf noup] fix various issues with Network Commissioning cluster on Wi-Fi #404

Merged
merged 3 commits into from
Mar 8, 2024
Merged

[nrf noup] fix various issues with Network Commissioning cluster on Wi-Fi #404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7598bd5
[nrf noup] fix handling of LastNetworkID in Wi-Fi driver
LuDuda Mar 7, 2024
c95c08e
[nrf noup] Do not clear Last Network ID and Connection Error after scan
LuDuda Mar 7, 2024
5b97b18
[nrf noup] Fix various Wi-Fi issues with error code handling
LuDuda Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/app/clusters/network-commissioning/network-commissioning.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,8 +519,6 @@ void Instance::OnFinished(Status status, CharSpan debugText, ThreadScanResponseI
}

mLastNetworkingStatusValue.SetNonNull(status);
mLastConnectErrorValue.SetNull();
mLastNetworkIDLen = 0;

TLV::TLVWriter * writer;
TLV::TLVType listContainerType;
Expand Down Expand Up @@ -633,8 +631,6 @@ void Instance::OnFinished(Status status, CharSpan debugText, WiFiScanResponseIte
}

mLastNetworkingStatusValue.SetNonNull(status);
mLastConnectErrorValue.SetNull();
mLastNetworkIDLen = 0;

TLV::TLVWriter * writer;
TLV::TLVType listContainerType;
Expand Down
28 changes: 17 additions & 11 deletions src/platform/nrfconnect/wifi/NrfWiFiDriver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,7 @@ CHIP_ERROR NrfWiFiDriver::Init(NetworkStatusChangeCallback * networkStatusChange

if (mStagingNetwork.IsConfigured())
{
WiFiManager::ConnectionHandling handling{ [] { Instance().OnNetworkStatusChanged(Status::kSuccess); },
[] { Instance().OnNetworkStatusChanged(Status::kUnknownError); },
WiFiManager::ConnectionHandling handling{ [](int connStatus) { Instance().OnNetworkStatusChanged(connStatus); },
System::Clock::Seconds32{ kWiFiConnectNetworkTimeoutSeconds } };
ReturnErrorOnFailure(
WiFiManager::Instance().Connect(mStagingNetwork.GetSsidSpan(), mStagingNetwork.GetPassSpan(), handling));
Expand All @@ -113,16 +112,25 @@ CHIP_ERROR NrfWiFiDriver::Init(NetworkStatusChangeCallback * networkStatusChange
return CHIP_NO_ERROR;
}

void NrfWiFiDriver::OnNetworkStatusChanged(Status status)
void NrfWiFiDriver::OnNetworkStatusChanged(int connStatus)
{
Status status = connStatus ? Status::kUnknownError : Status::kSuccess;

if (status == Status::kSuccess)
{
ConnectivityMgr().SetWiFiStationMode(ConnectivityManager::kWiFiStationMode_Enabled);
}

if (mpNetworkStatusChangeCallback)
{
mpNetworkStatusChangeCallback->OnNetworkingStatusChange(status, NullOptional, NullOptional);
WiFiManager::WiFiInfo wifiInfo;

if (CHIP_NO_ERROR == WiFiManager::Instance().GetWiFiInfo(wifiInfo))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we fallback to NullOptional in case the GetWiFiInfo() fails for whatever reason?

Copy link
Collaborator Author

@LuDuda LuDuda Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be okay, however I'm not sure. I see that in all 15 platforms, some NetworkID is used, or OnNetworkingStatusChange is skipped.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetWiFiInfo only succeeds when the connection attempt succeeded. Shouldn't we rather add a getter for mWantedNetwork to obtain the network ID used in the last failed attempt as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But don't we support only one network anyway? If the last connection failed then we can't interact with the device anyway, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but the connection may also fail during commissioning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed f2f, will be fixed later as it's not vital for now.

{
mpNetworkStatusChangeCallback->OnNetworkingStatusChange(status,
MakeOptional(ByteSpan(wifiInfo.mSsid, wifiInfo.mSsidLen)),
connStatus ? MakeOptional(connStatus) : NullOptional);
}
}

if (mpConnectCallback)
Expand Down Expand Up @@ -164,8 +172,7 @@ CHIP_ERROR NrfWiFiDriver::RevertConfiguration()

if (mStagingNetwork.IsConfigured())
{
WiFiManager::ConnectionHandling handling{ [] { Instance().OnNetworkStatusChanged(Status::kSuccess); },
[] { Instance().OnNetworkStatusChanged(Status::kUnknownError); },
WiFiManager::ConnectionHandling handling{ [](int connStatus) { Instance().OnNetworkStatusChanged(connStatus); },
System::Clock::Seconds32{ kWiFiConnectNetworkTimeoutSeconds } };
ReturnErrorOnFailure(
WiFiManager::Instance().Connect(mStagingNetwork.GetSsidSpan(), mStagingNetwork.GetPassSpan(), handling));
Expand Down Expand Up @@ -218,8 +225,7 @@ Status NrfWiFiDriver::ReorderNetwork(ByteSpan networkId, uint8_t index, MutableC
void NrfWiFiDriver::ConnectNetwork(ByteSpan networkId, ConnectCallback * callback)
{
Status status = Status::kSuccess;
WiFiManager::ConnectionHandling handling{ [] { Instance().OnNetworkStatusChanged(Status::kSuccess); },
[] { Instance().OnNetworkStatusChanged(Status::kUnknownError); },
WiFiManager::ConnectionHandling handling{ [](int connStatus) { Instance().OnNetworkStatusChanged(connStatus); },
System::Clock::Seconds32{ kWiFiConnectNetworkTimeoutSeconds } };

VerifyOrExit(mpConnectCallback == nullptr, status = Status::kUnknownError);
Expand Down Expand Up @@ -249,10 +255,10 @@ void NrfWiFiDriver::LoadFromStorage()
mStagingNetwork = network;
}

void NrfWiFiDriver::OnScanWiFiNetworkDone(WiFiManager::WiFiRequestStatus status)
void NrfWiFiDriver::OnScanWiFiNetworkDone(const wifi_status & status)
{
VerifyOrReturn(mScanCallback != nullptr);
mScanCallback->OnFinished(status == WiFiManager::WiFiRequestStatus::SUCCESS ? Status::kSuccess : Status::kUnknownError,
mScanCallback->OnFinished(status.status ? Status::kUnknownError : Status::kSuccess,
CharSpan(), &mScanResponseIterator);
mScanCallback = nullptr;
}
Expand All @@ -267,7 +273,7 @@ void NrfWiFiDriver::ScanNetworks(ByteSpan ssid, WiFiDriver::ScanCallback * callb
mScanCallback = callback;
CHIP_ERROR error = WiFiManager::Instance().Scan(
ssid, [](const WiFiScanResponse & response) { Instance().OnScanWiFiNetworkResult(response); },
[](WiFiManager::WiFiRequestStatus status) { Instance().OnScanWiFiNetworkDone(status); });
[](const wifi_status & status) { Instance().OnScanWiFiNetworkDone(status); });

if (error != CHIP_NO_ERROR)
{
Expand Down
4 changes: 2 additions & 2 deletions src/platform/nrfconnect/wifi/NrfWiFiDriver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,9 @@ class NrfWiFiDriver final : public WiFiDriver
return sInstance;
}

void OnNetworkStatusChanged(Status status);
void OnNetworkStatusChanged(int status);
void OnScanWiFiNetworkResult(const WiFiScanResponse & result);
void OnScanWiFiNetworkDone(WiFiManager::WiFiRequestStatus status);
void OnScanWiFiNetworkDone(const wifi_status & status);

private:
void LoadFromStorage();
Expand Down
33 changes: 15 additions & 18 deletions src/platform/nrfconnect/wifi/WiFiManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,8 @@ CHIP_ERROR WiFiManager::Connect(const ByteSpan & ssid, const ByteSpan & credenti
{
ChipLogDetail(DeviceLayer, "Connecting to WiFi network: %*s", ssid.size(), ssid.data());

mHandling.mOnConnectionSuccess = handling.mOnConnectionSuccess;
mHandling.mOnConnectionFailed = handling.mOnConnectionFailed;
mHandling.mConnectionTimeout = handling.mConnectionTimeout;
mHandling.mOnConnectionDone = handling.mOnConnectionDone;
mHandling.mConnectionTimeout = handling.mConnectionTimeout;

mWiFiState = WIFI_STATE_ASSOCIATING;

Expand Down Expand Up @@ -343,11 +342,10 @@ void WiFiManager::ScanDoneHandler(Platform::UniquePtr<uint8_t> data)
{
CHIP_ERROR err = SystemLayer().ScheduleLambda([capturedData = data.get()] {
Platform::UniquePtr<uint8_t> safePtr(capturedData);
uint8_t * rawData = safePtr.get();
const wifi_status * status = reinterpret_cast<const wifi_status *>(rawData);
WiFiRequestStatus requestStatus = static_cast<WiFiRequestStatus>(status->status);
uint8_t * rawData = safePtr.get();
const wifi_status * status = reinterpret_cast<const wifi_status *>(rawData);

if (requestStatus == WiFiRequestStatus::FAILURE)
if (status->status)
{
ChipLogError(DeviceLayer, "Wi-Fi scan finalization failure (%d)", status->status);
}
Expand All @@ -358,7 +356,7 @@ void WiFiManager::ScanDoneHandler(Platform::UniquePtr<uint8_t> data)

if (Instance().mScanDoneCallback && !Instance().mInternalScan)
{
Instance().mScanDoneCallback(requestStatus);
Instance().mScanDoneCallback(*status);
// restore the connection state from before the scan request was issued
Instance().mWiFiState = Instance().mCachedWiFiState;
return;
Expand All @@ -379,13 +377,13 @@ void WiFiManager::ScanDoneHandler(Platform::UniquePtr<uint8_t> data)

Instance().mWiFiState = WIFI_STATE_ASSOCIATING;

if (net_mgmt(NET_REQUEST_WIFI_CONNECT, Instance().mNetIf, &(Instance().mWiFiParams.mParams),
sizeof(wifi_connect_req_params)))
if (int connStatus = net_mgmt(NET_REQUEST_WIFI_CONNECT, Instance().mNetIf, &(Instance().mWiFiParams.mParams),
sizeof(wifi_connect_req_params)))
{
ChipLogError(DeviceLayer, "Connection request failed");
if (Instance().mHandling.mOnConnectionFailed)
if (Instance().mHandling.mOnConnectionDone)
{
Instance().mHandling.mOnConnectionFailed();
Instance().mHandling.mOnConnectionDone(connStatus);
}
Instance().mWiFiState = WIFI_STATE_DISCONNECTED;
return;
Expand Down Expand Up @@ -424,15 +422,14 @@ void WiFiManager::ConnectHandler(Platform::UniquePtr<uint8_t> data)
Platform::UniquePtr<uint8_t> safePtr(capturedData);
uint8_t * rawData = safePtr.get();
const wifi_status * status = reinterpret_cast<const wifi_status *>(rawData);
WiFiRequestStatus requestStatus = static_cast<WiFiRequestStatus>(status->status);

if (requestStatus == WiFiRequestStatus::FAILURE || requestStatus == WiFiRequestStatus::TERMINATED)
if (status->status)
{
ChipLogProgress(DeviceLayer, "Connection to WiFi network failed or was terminated by another request");
Instance().mWiFiState = WIFI_STATE_DISCONNECTED;
if (Instance().mHandling.mOnConnectionFailed)
if (Instance().mHandling.mOnConnectionDone)
{
Instance().mHandling.mOnConnectionFailed();
Instance().mHandling.mOnConnectionDone(status->status);
}
}
else // The connection has been established successfully.
Expand All @@ -444,9 +441,9 @@ void WiFiManager::ConnectHandler(Platform::UniquePtr<uint8_t> data)

ChipLogProgress(DeviceLayer, "Connected to WiFi network");
Instance().mWiFiState = WIFI_STATE_COMPLETED;
if (Instance().mHandling.mOnConnectionSuccess)
if (Instance().mHandling.mOnConnectionDone)
{
Instance().mHandling.mOnConnectionSuccess();
Instance().mHandling.mOnConnectionDone(status->status);
}
Instance().PostConnectivityStatusChange(kConnectivity_Established);

Expand Down
14 changes: 3 additions & 11 deletions src/platform/nrfconnect/wifi/WiFiManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,16 +88,9 @@ class Map
class WiFiManager
{
public:
enum WiFiRequestStatus : int
{
SUCCESS = 0,
FAILURE = 1,
TERMINATED = 2
};

using ScanResultCallback = void (*)(const NetworkCommissioning::WiFiScanResponse &);
using ScanDoneCallback = void (*)(WiFiRequestStatus);
using ConnectionCallback = void (*)();
using ScanDoneCallback = void (*)(const wifi_status &);
using ConnectionCallback = void (*)(int);

enum class StationStatus : uint8_t
{
Expand All @@ -120,8 +113,7 @@ class WiFiManager

struct ConnectionHandling
{
ConnectionCallback mOnConnectionSuccess{};
ConnectionCallback mOnConnectionFailed{};
ConnectionCallback mOnConnectionDone{};
System::Clock::Seconds32 mConnectionTimeout{};
};

Expand Down
Loading