IController - Persist and Restore by nxtum · Pull Request #1920 · rdkcentral/Thunder

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)

Source/plugins/IController.h

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...

Source/plugins/IController.h

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 approach inefficient. 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-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.

Source/Thunder/PluginServer.h

Source/plugins/IController.h

…tConfigs

Copilot

Pull request overview

This pull request adds granular control to plugin configuration persistence by allowing users to persist or restore individual plugins rather than all plugins at once. The changes transform the single "persist all" functionality into a flexible system that supports persisting/restoring specific plugins, all plugins, or server-wide configuration.

Changes:

Extended Persist method to accept an optional callsign parameter, allowing selective persistence of individual plugins or all plugins
Added new Restore method to remove persisted configuration overrides and return plugins to their default state
Refactored the storage system from a single override.json file to individual per-plugin JSON files in a directory structure

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
Source/plugins/IController.h	Updated IConfiguration interface with parameterized Persist method and new Restore method
Source/Thunder/PluginServer.h	Major refactoring of Override class: changed from single-file to directory-based storage, implemented Save/Destroy methods with per-plugin file handling, added PluginHost-specific configuration methods
Source/Thunder/PluginServer.cpp	Updated PluginOverrideFile constant to PluginOverrideDirectory, fixed typo in comment
Source/Thunder/Controller.h	Added Restore method declaration and updated Persist signature
Source/Thunder/Controller.cpp	Implemented new Restore method and updated Persist to forward callsign parameter

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Source/plugins/IController.h

Source/Thunder/PluginServer.h

Source/Thunder/PluginServer.cpp

Copilot · 2026-02-15T13:41:46Z

Source/Thunder/PluginServer.h

+                if (callsign.IsSet() == true) {
+                    bool found = false;
+                    const string& raw = callsign.Value();
+                    const string target = raw.empty() ? _T("Controller") : raw;


When an empty string is passed as callsign (callsign.IsSet() is true but callsign.Value() is empty), it gets mapped to "Controller" at line 1828. This special case should be documented in the function or the IController interface, as it's not obvious that an empty string means "Controller".

Copilot · 2026-02-15T13:41:47Z

Source/Thunder/PluginServer.h

+                if (callsign.IsSet() == true) {
+                    bool found = false;
+                    const string& raw = callsign.Value();
+                    const string target = raw.empty() ? _T("Controller") : raw;


When an empty string is passed as callsign (callsign.IsSet() is true but callsign.Value() is empty), it gets mapped to "Controller" at line 1866. This special case should be documented in the function or the IController interface, as it's not obvious that an empty string means "Controller". This duplicates the same logic from the Save method.

Source/Thunder/PluginServer.h

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

Source/plugins/IController.h Outdated Show resolved Hide resolved

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

sebaszm requested changes Dec 9, 2025

View reviewed changes

nxtum added 4 commits January 9, 2026 06:03

Merge branch 'master' into PersistConfigs

6c75e29

Merge remote-tracking branch 'origin' into PersistConfigs

df5189b

variant stuff

2a6d794

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

81b960a

…tConfigs

Copilot AI review requested due to automatic review settings February 15, 2026 13:35

Copilot started reviewing on behalf of nxtum February 15, 2026 13:35 View session

Merge branch 'master' into PersistConfigs

cfac397

Copilot AI reviewed Feb 15, 2026

View reviewed changes

rdkcmf-jenkins reviewed Feb 15, 2026

View reviewed changes

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

rdkcmf-jenkins reviewed Feb 15, 2026

View reviewed changes

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

copilot comments

4383f82

Conversation

nxtum commented Aug 14, 2025

Uh oh!

Uh oh!

Uh oh!

pwielders left a comment

Choose a reason for hiding this comment

Uh oh!

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!

sebaszm Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sebaszm commented Sep 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sebaszm Sep 11, 2025 •

edited

Loading