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] Add AppFactoryResetHandler #569

Closed
wants to merge 1 commit into from
Closed

[nrf noup] Add AppFactoryResetHandler #569

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions src/platform/Zephyr/ConfigurationManagerImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@
#include <mpsl/mpsl_lib.h>
#endif // CONFIG_SOC_FLASH_NRF_RADIO_SYNC_MPSL

// TODO: Remove this workaround after proper API is added
__weak bool AppFactoryResetHandler()
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to use a callback instead of a weak function?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that would be my first suggestion, but obviously it requires more effort to extend interfaces and resolve reviews in the upstream :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so could you please create a bug for it and write here a comment to point out that it is a workaround?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added KRKNWK-20060.

{
return true;
}

namespace chip {
namespace DeviceLayer {

Expand Down Expand Up @@ -238,8 +244,14 @@ void ConfigurationManagerImpl::DoFactoryReset(intptr_t arg)
{
ChipLogError(DeviceLayer, "Factory reset failed: %" CHIP_ERROR_FORMAT, err.Format());
}

#endif // CONFIG_CHIP_FACTORY_RESET_ERASE_SETTINGS

if (!AppFactoryResetHandler())
{
ChipLogError(DeviceLayer, "Factory reset failed, AppFactoryReset failed");
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

}

PlatformMgr().Shutdown();
}

Expand Down
Loading