[zenker]

In the controls system adapter retrieving all PVs is done before deciding which to skip. After, the persistent store should be added to those PVs that should not be excluded and thus need a persistent data storage.

I don't know if other Control System Adapter also use the persistent data storage. If that is the case I could add a bool to getAllProcessVariables() to decide if the previous behavior should be used and make it the default.

[mhier]

What do you mean by skipped PVs? Are those the PVs handed to ApplicationBase::optimiseUnmappedVariables()? Shouldn't we couple this so that the exclusion happens automatically?

[zenker]

What do you mean by skipped PVs? Are those the PVs handed to ApplicationBase::optimiseUnmappedVariables()? Shouldn't we couple this so that the exclusion happens automatically?

Yes I mean ApplicationBase::optimiseUnmappedVariables(), but at least in the OPC UA adapter the optimisation is called later.

First the persistence is enabled - this happens before ChimeraTK::ApplicationBase::getInstance().initialise(). After I need to get all PVs via getAllProcessVariables() (this is where currently the persistent data storage is added to all PVs). Then I decide which PVs are excluded. So I can call the new function setPersistentDataStorage(). Currently I call this new function right in the constructor of the OPC UA control system adapter, but one could also call it in ApplicationBase::optimiseUnmappedVariables().

However in this PR I just implement the separation of getting all PVs and adding the persistent data storage. Like that I can add the persistent data storage later in the code.

[mhier]

Does enablePersistentDataStorage() have any other effect than calling setPersistentDataStorage() on each PV we return? If not, we do not need the current enablePersistentDataStorage() at all any more, resp. I would keep that name but change the functionality to be identical to your new setPersistentDataStorage() function (maybe add an empty default value for the exclude parameter).

Btw: You have missed that getProcessVariable() also calls setPersistentDataStorage(), so the filtering would not happen for that code path.

[zenker]

Does enablePersistentDataStorage() have any other effect than calling setPersistentDataStorage() on each PV we return? If not, we do not need the current enablePersistentDataStorage() at all any more, resp. I would keep that name but change the functionality to be identical to your new setPersistentDataStorage() function (maybe add an empty default value for the exclude parameter).

Btw: You have missed that getProcessVariable() also calls setPersistentDataStorage(), so the filtering would not happen for that code path.

In enablePersistentDataStorage() the PersistentDataStorage is created. This includes calling readFromFile(). So this reads all data stored in the persist file and thus provides the initial values. In case of the OPC UA control system adapter, after calling enablePersistentDataStorage() the function ChimeraTK::ApplicationBase::getInstance().initialise() is called which applies the initial values.
So yes the function enablePersistentDataStorage() is still needed and needs to be called before ChimeraTK::ApplicationBase::getInstance().initialise(), whereas setPersistentDataStorage() can only be called after ChimeraTK::ApplicationBase::getInstance().initialise().

See also here.

[killenb]

Technically the implementation is OK.

I don't fully like the names of the two steps (enable and set). Can we come up with better names?

[zenker]

I realized that the persistence can be enabled also after calling initialise(). I was biased by the name and thought the initial values are written in initialise(). However this is not true - they are written when calling setPersistentDataStorage on the pv.
Now it is possible to combine both new methods and to get rid of the mutable keyword for the _persistentDataStorage, because it is no longer used in the const methods -> @mhier: Please check that. I'm not so familiar with using mutable.

@killenb I left the name unchanged as it was there already before this PR.