Test for Removing Thread credentials after last fabric remove #174
Conversation
ArekBalysNordic
requested review from
LuDuda,
kkasperczyk-no and
markaj-nordic
ArekBalysNordic
force-pushed
the
remove_last_fabric_test
branch
from
774a18a
to
070a774
Compare
| @@ -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); | |||
There was a problem hiding this comment.
Maybe it would be better to make FabricTableDelegate a friend of AppTask instead of moving this method to public.
There was a problem hiding this comment.
Oh, great point :)
| if (Server::GetInstance().GetFabricTable().FabricCount() == 0) | ||
| { | ||
| #ifdef CONFIG_CHIP_LAST_FABRIC_REMOVED_ERASE_AND_REBOOT | ||
| Server::GetInstance().ScheduleFactoryReset(); |
There was a problem hiding this comment.
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.
| #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(); |
There was a problem hiding this comment.
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 */ | ||
| DeviceLayer::ConnectivityMgr().ErasePersistentInfo(); |
There was a problem hiding this comment.
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.
| 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. | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I can add something like that :)
| Server::GetInstance().GetFabricTable().DeleteAllFabrics(); | ||
| /* Erase Matter data */ | ||
| DeviceLayer::PersistedStorage::KeyValueStoreMgrImpl().DoFactoryReset(); | ||
| /* Erase Thread credentials and disconnect */ |
There was a problem hiding this comment.
Thread... and Wi-Fi right?
| @@ -212,6 +213,7 @@ CHIP_ERROR AppTask::Init() | |||
| (void) initParams.InitializeStaticResourcesBeforeServerInit(); | |||
| initParams.testEventTriggerDelegate = &testEventTriggerDelegate; | |||
| ReturnErrorOnFailure(chip::Server::GetInstance().Init(initParams)); | |||
| AppFabricTableDelegate::Init(); | |||
There was a problem hiding this comment.
Maybe planned.. but we should do it for all the samples.
There was a problem hiding this comment.
Yes, of course, this draft points only to the Lock to be clearer for the review :)
| if (Server::GetInstance().GetFabricTable().FabricCount() == 0) | ||
| { | ||
| #ifdef CONFIG_CHIP_LAST_FABRIC_REMOVED_ERASE_AND_REBOOT | ||
| Server::GetInstance().ScheduleFactoryReset(); |
There was a problem hiding this comment.
I heard from Eve that calling FactoryReset immediately does not work. That 3 seconds delay is necessary. We don't see such an issue?
There was a problem hiding this comment.
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.
chiptool_ncs operationalcredentials remove-fabric 2 1 0