[nrf noup] Add AppFactoryResetHandler #569
Conversation
adigie
force-pushed
the
app-factory-reset-handler
branch
from
1e3885c to
75c75dd
Compare
|
|
||
| if (!AppFactoryResetHandler()) | ||
| { | ||
| ChipLogError(DeviceLayer, "Factory reset failed, AppFactoryReset failed"); |
There was a problem hiding this comment.
Can you describe why this is needed? Application is typically the one that triggers the factory reset so it can remove its records before calling Matter factory reset. Or is there a reason you would prefer having this called exactly at this moment?
There was a problem hiding this comment.
The main reason is to cleanup AccessStorage in door lock sample. With CONFIG_CHIP_FACTORY_RESET_ERASE_SETTINGS=n we have to manually clean individual settings during factory reset. Do you think we could do this in some better way?
There was a problem hiding this comment.
It just felt weird to see the callback to the application as it's the application that normally triggers the factory reset (so it seems unnecessary cyclic dependency). Plus weak functions are easy to misuse. But I guess it's easier to reuse existing shell commands for triggering the factory reset the way you did it, so I'm OK with that :)
adigie
force-pushed
the
app-factory-reset-handler
branch
from
75c75dd to
c3c0ebb
Compare
|
|
||
| if (!AppFactoryResetHandler()) | ||
| { | ||
| ChipLogError(DeviceLayer, "Factory reset failed, AppFactoryReset failed"); |
There was a problem hiding this comment.
It just felt weird to see the callback to the application as it's the application that normally triggers the factory reset (so it seems unnecessary cyclic dependency). Plus weak functions are easy to misuse. But I guess it's easier to reuse existing shell commands for triggering the factory reset the way you did it, so I'm OK with that :)
| @@ -58,6 +58,11 @@ | |||
| #include <mpsl/mpsl_lib.h> | |||
| #endif // CONFIG_SOC_FLASH_NRF_RADIO_SYNC_MPSL | |||
|
|
|||
| __weak bool AppFactoryResetHandler() | |||
There was a problem hiding this comment.
Wouldn't it be better to use a callback instead of a weak function?
There was a problem hiding this comment.
It would, I can change this. We should design some interface for that and make changes in the upstream, but that will probably take a while.
There was a problem hiding this comment.
Yes, that would be my first suggestion, but obviously it requires more effort to extend interfaces and resolve reviews in the upstream :)
There was a problem hiding this comment.
Callback is more accurate to the whole upstream's approach. We can easily assign it in the application using the configuration manager instance. I wonder whether we should prepare it as toup and then try to upstream it rather than making this as noup and create a temporary weak function.
There was a problem hiding this comment.
Adding callback would take some time. We should merge this workaround and use it in v2.9.2, then implement proper solution for v3.0.0.
There was a problem hiding this comment.
Ok, so could you please create a bug for it and write here a comment to point out that it is a workaround?
adigie
force-pushed
the
app-factory-reset-handler
branch
from
c3c0ebb to
6ce9516
Compare
Add weak `AppFactoryResetHandler` to handle application specific cleanup. Signed-off-by: Adrian Gielniewski <adrian.gielniewski@nordicsemi.no>
adigie
force-pushed
the
app-factory-reset-handler
branch
from
6ce9516 to
047591c
Compare
Add weak
AppFactoryResetHandlerto handle application specific cleanup.NCS PR: nrfconnect/sdk-nrf/pull/20803