IController - Persist and Restore #1920

nxtum · 2025-08-14T16:27:51Z

Previously there was one Persist function that would save the configs for all plugins without the ability to undo.
Now, you can specify which plugin to persist, as well as remove any/all configs (restore to default)

nxtum · 2025-08-14T16:37:33Z

Source/plugins/IController.h

        enum { ID = RPC::ID_CONTROLLER_CONFIGURATION };

+        // @omit
+        Core::hresult Persist() { return Persist(Core::OptionalType<string>());}


This could be removed, but will need to create 2 PR to update:
LocationSync.cpp https://github.com/WebPlatformForEmbedded/ThunderNanoServicesRDK/blob/master/LocationSync/LocationSync.cpp#L126

ConfigUpdateExample: https://github.com/rdkcentral/ThunderNanoServices/blob/master/examples/ConfigUpdateExample/ConfigUpdateExample.cpp#L63

Please mark it as DEPRECATED so people using it will get a warning!

Source/Thunder/PluginServer.h

…tConfigs

pwielders

An overall remark. I guess in the past, because all plugins where in 1 file, we added an empty config ('{}') for plugins that had no override or where destroyed. However now the configs are single entries per callsign. Whenever you "add" a file it probably takes up a sector on flash storage per file (much more than two byes to signal {} in the one file approach). I guess it is worth the effort to not store an override file if there are no changes to to config. So suggest to add this optimization in the code.
No changes on the plugin, no override file in the thunder/plugins directpry o persitent storage space to safe flash sectors...

pwielders · 2025-08-15T15:01:56Z

Source/plugins/IController.h

        enum { ID = RPC::ID_CONTROLLER_CONFIGURATION };

+        // @omit
+        Core::hresult Persist() { return Persist(Core::OptionalType<string>());}


Please mark it as DEPRECATED so people using it will get a warning!

pwielders · 2025-08-21T15:28:55Z

Source/Thunder/PluginServer.cpp

 #endif

-    /* static */ const TCHAR* Server::PluginOverrideFile = _T("PluginHost/override.json");
+    /* static */ const TCHAR* Server::PluginOverrideDirectory = _T("PluginHost/");


Might be good to move the directory now to Thunder/plugins and store the configs there, it is in the same line as the fixed configs are. Maybe you could even swap this line than with the nex line and use the default of the plugins config from there so place this:

/* static */ const TCHAR* Server::PluginOverrideDirectory = _T("Thunder/") + Server::PluginConfigDirectory;

on the line "after" the Server::PluginConfigDirectory declaration

pwielders · 2025-08-21T15:34:43Z

Source/Thunder/PluginServer.h

+                    // Create individual plugin configuration files
+                    string filePath;
+                    filePath.reserve(persistentFolder.size() + name.size() + sizeof("Override.json") - 1);
+                    filePath.append(persistentFolder).append(name).append("Override.json");


If we put them all in a folder (e.g. previous remark Thunder/plugins. I guess we could than use the same sematics as we do in thunder an only append the name with json, so:

filePath.append(persistentFolder).append(name).append(".json");

than it is consistent with the configs we find in /etc/Thunder/plugins as the startin point. Consistency is key ;-)

…tConfigs

pwielders

Questions/Remark:

Why is in the save, the way to create an override a lamda i.s.o. a normal private method?
Comparing if the configs are different might be a bit more tricky. Cause officially:

{ "fieldA": 1, "fieldB" : 2 } === { "fieldB": 2, "fieldA": 1}

This is now detected as different configs.
And probbaly even worse:

{     "field":   1 } === {"field":1}

Is already detected as a different config :-)

got 2 suggestions to avoid this (or make it more clear):

Use a flag on the service that if the "config" is set through a method, flag it as dirty and consider the config to be changed (even if it was not changed :-)
enhance the Core::JSON::VariantContainer class with an bool operator ==(const VariantContainer& ) const and load each string into the VariantContainer class and compare those (field availability and there corresponding values)

I have a preference for the second option enhance the JSON class :-) might be usefull in more places and the VariantContainer is already used anway, somewhere in the PluginServer code but aI am also interested in @sebaszm opinion here...

sebaszm · 2025-09-11T11:13:40Z

Source/Thunder/PluginServer.cpp

-
-    /* static */ const TCHAR* Server::PluginOverrideFile = _T("PluginHost/override.json");
    /* static */ const TCHAR* Server::PluginConfigDirectory = _T("plugins/");
+    /* static */ const TCHAR* Server::PluginOverrideDirectory = _T("Thunder/plugins/");


Use EXPAND_AND_QUOTE(NAMESPACE) instead of using framework name directly.

sebaszm · 2025-09-11T11:14:10Z

Source/Thunder/PluginServer.cpp


        // See if the persitent path for our-selves exist, if not we will create it :-)
-        Core::File persistentPath(_config.PersistentPath() + _T("PluginHost"));
+        Core::File persistentPath(_config.PersistentPath() + _T("Thunder/plugins"));


Use the constant instead of repeating the path name.

sebaszm · 2025-09-11T11:21:25Z

Source/Thunder/PluginServer.h

+                    // Create individual plugin configuration files
+                    string filePath;
+                    filePath.reserve(persistentFolder.size() + name.size() + sizeof(".json") - 1);
+                    filePath.append(persistentFolder).append(name).append(".json");
+
+                    _pluginOverrideFiles.emplace(
+                        std::piecewise_construct,
+                        std::forward_as_tuple(name),
+                        std::forward_as_tuple(std::move(filePath))
+                    );
+


Do we really need to remember the filenames for each callsign? if we only stored the base folder having a callsing we can every time find the right path. I don't think configs are being overiden so frequent to justify this flamboyance with memory.

sebaszm · 2025-09-11T11:23:43Z

Source/Thunder/PluginServer.h

+                        // Clear all currently set values, they might be from the previous run.
+                        Clear();

-                        Callsigns::const_iterator current(_callsigns.find(index->Callsign()));
+                        // Read the file and parse it into this object.
+                        IElement::FromFile(storage);
+                        _serverconfig.SetPrefix(Prefix.Value());
+                        _serverconfig.SetIdleTime(IdleTime.Value());


This part shouldn't be repeated for each plugin config. This clears and looks for prefix and idletime in each of the plugins' config while this is Thunder property.

sebaszm · 2025-09-11T11:26:20Z

Source/Thunder/PluginServer.h

                uint32_t result = Core::ERROR_NONE;
+
+                auto CreateOverride = [this, &result](ServiceMap::Iterator& indexService) {
+                    const string currentCallsign = indexService->Callsign();


sebaszm · 2025-09-11T11:46:08Z

Source/Thunder/PluginServer.h

+                        }
+
+                        Core::File storage(indexFile->second);
+                        if (storage.Create() == true) {


Let's not write the override config at all if it's not changed! Let the absence of the override file mean there is no override.

sebaszm · 2025-09-11T11:51:25Z

Source/Thunder/PluginServer.h

+            Override infoBlob(_config, _services, Configuration().PersistentPath() + PluginOverrideDirectory);
+            return (infoBlob.Save(callsign));
+        }

-            return (infoBlob.Save());
+        uint32_t Restore(const Core::OptionalType<string>& callsign)
+        {
+            Override infoBlob(_config, _services, Configuration().PersistentPath() + PluginOverrideDirectory);
+            return (infoBlob.Destroy(callsign));
        }
+
        uint32_t Load()
        {
-            Override infoBlob(_config, _services, Configuration().PersistentPath() + PluginOverrideFile);
-
+            Override infoBlob(_config, _services, Configuration().PersistentPath() + PluginOverrideDirectory);


I find this inefficient approach. Even if we are working with a single callsign we still create the Override object and prepare for handling all plugins (ie. saving callsiings, configs, filenames.... etc).

Source/plugins/IController.h

sebaszm · 2025-09-11T11:54:49Z

Source/plugins/IController.h

    struct EXTERNAL IConfiguration : virtual public Core::IUnknown {
        enum { ID = RPC::ID_CONTROLLER_CONFIGURATION };

+        // @omit @deprecated


omit and deprecated tags are not needed as this is not a virtual method and will not be considered for code generation at any level.

Source/plugins/IController.h

sebaszm · 2025-09-11T14:21:33Z

Use a flag on the service that if the "config" is set through a method, flag it as dirty and consider the config to be changed (even if it was not changed :-)

enhance the Core::JSON::VariantContainer class with an bool operator ==(const VariantContainer& ) const and load each string into the VariantContainer class and compare those (field availability and there corresponding values)

I have a preference for the second option enhance the JSON class :-) might be usefull in more places and the VariantContainer is already used anway, somewhere in the PluginServer code but aI am also interested in @sebaszm opinion here...

does not solve the problem much if Store() is used with no callsign then all plugins will be marked dirty even if not changed.

Agree, to do this properly we would need to normalize the JSON first. JSON::Variant would be a perfect starting point to achieve this (it is though behind COMPLEMENTARY_CODE_SET flag). We however will encounter nuance like order of elements in array (is [a,b] same as [b,a]? depends), or explicitly setting a value that is same as a default. I think too that it would be a useful feature, but perhaps we should split this into another PR.

nxtum added 3 commits August 14, 2025 08:17

directory not being created locally...

8532490

comments

5ca8414

documentation changes

0cf95f8

nxtum commented Aug 14, 2025

View reviewed changes

rdkcmf-jenkins reviewed Aug 14, 2025

View reviewed changes

Source/Thunder/PluginServer.h Outdated Show resolved Hide resolved

nxtum requested review from pwielders and sebaszm August 15, 2025 11:12

MFransen69 and others added 5 commits August 19, 2025 18:52

Merge branch 'master' into PersistConfigs

37975ae

fix concat

57a56e1

Merge branch 'PersistConfigs' of github.com:nxtum/Thunder into Persis…

fed1996

…tConfigs

Merge branch 'master' into PersistConfigs

5720420

Merge branch 'master' into PersistConfigs

59199cd

pwielders requested changes Aug 21, 2025

View reviewed changes

nxtum and others added 5 commits August 28, 2025 08:44

Check if config differs

e2c6116

Merge branch 'PersistConfigs' of github.com:nxtum/Thunder into Persis…

4b29d94

…tConfigs

Merge branch 'master' into PersistConfigs

dae711a

add deprecated

8a59b72

Merge branch 'master' into PersistConfigs

782b5da

pwielders self-requested a review September 6, 2025 11:04

pwielders reviewed Sep 6, 2025

View reviewed changes

sebaszm requested changes Sep 11, 2025

View reviewed changes

pwielders and others added 4 commits September 19, 2025 09:24

Merge branch 'master' into PersistConfigs

0a5fc2a

changes no store files

a46b4fd

Merge branch 'master' into PersistConfigs

4a99d86

Merge branch 'master' into PersistConfigs

96e446c

sebaszm self-requested a review October 1, 2025 13:22

sebaszm and others added 2 commits October 17, 2025 12:42

Merge branch 'master' into PersistConfigs

237c40a

Merge branch 'master' into PersistConfigs

dfe6b0f

IController - Persist and Restore #1920

Are you sure you want to change the base?

IController - Persist and Restore #1920

Conversation

nxtum commented Aug 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pwielders left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pwielders left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sebaszm commented Sep 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants