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.

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

Test for Removing Thread credentials after last fabric remove #174

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 28 additions & 0 deletions config/nrfconnect/chip-module/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,34 @@ config CHIP_DEBUG_SYMBOLS
config CHIP_MALLOC_SYS_HEAP
default y if !ARCH_POSIX

choice CHIP_LAST_FABRIC_REMOVED_ACTION
prompt "An action to perform after removing the last fabric"
default CHIP_LAST_FABRIC_REMOVED_ERASE_AND_REBOOT

config CHIP_LAST_FABRIC_REMOVED_ERASE_AND_REBOOT
bool "After removing the last fabric erase NVS and reboot"
help
After removing the last fabric the device will perform the factory reset and
then reboot. The current RAM state will be removed and the new commissioning to
the new fabric will use the initial fabric index. This option is the most safe.

config CHIP_LAST_FABRIC_REMOVED_ERASE_AND_PAIRING_START
bool "After removing the last fabric erase NVS and start Bluetooth LE advertising"
help
After removing the last fabric the device will perform the factory reset without
rebooting and start the Bluetooth LE advertisement automatically.
The current RAM state will be saved and the new commissioning to the next
fabric will use the next possible fabric index.

config CHIP_LAST_FABRIC_REMOVED_ERASE_ONLY
bool "After removing the last fabric erase NVS only"
help
After removing the last fabric the device will perform the factory reset only without
rebooting. The current RAM state will be saved and the new commissioning to the next
fabric will use the next possible fabric index.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, what about a case, where user would like to persist Thread Credentials (aka < NCS 2.5.0)? We could introduce a new option like: CHIP_LAST_FABRIC_REMOVED_NONE to keep pre NCS 2.5.0 behavior, but more important to enable customers to implement custom way of handling it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I can add something like that :)

endchoice

config CHIP_FACTORY_DATA
bool "Factory data provider"
select ZCBOR
Expand Down
2 changes: 2 additions & 0 deletions examples/lock-app/nrfconnect/main/AppTask.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "AppTask.h"
#include "AppConfig.h"
#include "BoltLockManager.h"
#include "FabricTableDelegate.h"
#include "LEDUtil.h"
#include "LEDWidget.h"

Expand Down Expand Up @@ -212,6 +213,7 @@ CHIP_ERROR AppTask::Init()
(void) initParams.InitializeStaticResourcesBeforeServerInit();
initParams.testEventTriggerDelegate = &testEventTriggerDelegate;
ReturnErrorOnFailure(chip::Server::GetInstance().Init(initParams));
AppFabricTableDelegate::Init();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe planned.. but we should do it for all the samples.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, of course, this draft points only to the Lock to be clearer for the review :)


gExampleDeviceInfoProvider.SetStorageDelegate(&Server::GetInstance().GetPersistentStorage());
chip::DeviceLayer::SetDeviceInfoProvider(&gExampleDeviceInfoProvider);
Expand Down
3 changes: 2 additions & 1 deletion examples/lock-app/nrfconnect/main/include/AppTask.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ class AppTask
static void IdentifyStartHandler(Identify *);
static void IdentifyStopHandler(Identify *);

static void StartBLEAdvertisementHandler(const AppEvent & event);

private:
CHIP_ERROR Init();

Expand All @@ -67,7 +69,6 @@ class AppTask
static void FunctionHandler(const AppEvent & event);
static void StartBLEAdvertisementAndLockActionEventHandler(const AppEvent & event);
static void LockActionEventHandler(const AppEvent & event);
static void StartBLEAdvertisementHandler(const AppEvent & event);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it would be better to make FabricTableDelegate a friend of AppTask instead of moving this method to public.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh, great point :)

static void UpdateLedStateEventHandler(const AppEvent & event);

static void ChipEventHandler(const chip::DeviceLayer::ChipDeviceEvent * event, intptr_t arg);
Expand Down
65 changes: 65 additions & 0 deletions examples/platform/nrfconnect/util/include/FabricTableDelegate.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
*
* Copyright (c) 2023 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#pragma once

#include "AppTask.h"

#include <app/server/Server.h>
#include <app/util/attribute-storage.h>

namespace chip {

class AppFabricTableDelegate : public FabricTable::Delegate
{
public:
~AppFabricTableDelegate() { Server::GetInstance().GetFabricTable().RemoveFabricDelegate(this); }

static void Init()
{
static AppFabricTableDelegate sAppFabricDelegate;
Server::GetInstance().GetFabricTable().AddFabricDelegate(&sAppFabricDelegate);
}

private:
void OnFabricRemoved(const FabricTable & fabricTable, FabricIndex fabricIndex)
{
if (Server::GetInstance().GetFabricTable().FabricCount() == 0)
{
#ifdef CONFIG_CHIP_LAST_FABRIC_REMOVED_ERASE_AND_REBOOT
Server::GetInstance().ScheduleFactoryReset();
Copy link
Collaborator

@kkasperczyk-no kkasperczyk-no Sep 22, 2023

Choose a reason for hiding this comment

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

IMO the best from the Matter perspective would be to modify the ScheduleFactoryReset to do not call Shutdown always, but only in case configuration requires it. It could be controlled e.g. using some Matter define, then set by us using Kconfig. This way we would avoid duplicating the code here and re-use what currently exist.
Nevertheless such implementation would mean everyone get our fix for free, so I can understand if we do not want to do that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I heard from Eve that calling FactoryReset immediately does not work. That 3 seconds delay is necessary. We don't see such an issue?

Copy link
Owner Author

Choose a reason for hiding this comment

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

We don't call factory reset immediately, but schedule it to call within the CHIP thread. If I called it immediately in this method, that was true, and the device couldn't send ack to the chip controller. Postponing it to the CHIP thread seems to resolve that problem.

#elif defined(CONFIG_CHIP_LAST_FABRIC_REMOVED_ERASE_ONLY) || defined(CONFIG_CHIP_LAST_FABRIC_REMOVED_ERASE_AND_PAIRING_START)
DeviceLayer::PlatformMgr().ScheduleWork([](intptr_t) {
// Delete all fabrics and emit Leave event.
Server::GetInstance().GetFabricTable().DeleteAllFabrics();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it delete all fabrics from RAM? Does it make sense assuming we already have FabricCount() == 0?

/* Erase Matter data */
DeviceLayer::PersistedStorage::KeyValueStoreMgrImpl().DoFactoryReset();
/* Erase Thread credentials and disconnect */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thread... and Wi-Fi right?

DeviceLayer::ConnectivityMgr().ErasePersistentInfo();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we call ClearWiFiStationProvision() for WiFi devices? Currently this method only clears the cached network configuration but I suppose we can extend it so that the station is disconnected from AP.

#ifdef CONFIG_CHIP_LAST_FABRIC_REMOVED_ERASE_AND_PAIRING_START
/* Start the New BLE advertising */
AppEvent event;
event.Handler = AppTask::StartBLEAdvertisementHandler;
AppTask::Instance().PostEvent(event);
#endif
});
#endif
}
}
};
} // namespace chip
Loading