feat: switch properties to read-only at runtime (#9384)#183
feat: switch properties to read-only at runtime (#9384)#183
Conversation
|
I just pushed a second commit, but I need to check this myself tomorrow more closely... |
This implements the missing case when the switch is a writeable property. Test is by me actually, rest is AI generated (several interations with input from me).
mhier
force-pushed
the
mhier/feat-9384-ro-access-at-runtime
branch
from
93724e5 to
3ae039f
Compare
|
I have checked the AI generated commit, so this is now ready for review. I will squash that commit later into the feat: switch properties to read-only at runtime (#9384) commit, so we have just two commits (that one and the one with the chrome bars). |
| // value from the caller's DOOCS property via the EqData interface when the callback fires. | ||
| assert(!doocsAdapter.writeableVariablesWithMultiplePropertiesIsFinal); | ||
| doocsAdapter.writeableVariablesWithMultipleProperties[sourcePath].emplace_back( | ||
| [this](bool handleLocking, const boost::weak_ptr<PropertyBase>& caller) { |
There was a problem hiding this comment.
I wonder whether it could be a problem that in this callback, you do not have the 'self != caller' check.
Suppose the dynamic property meaning the one using the isWriteableSource is written to, then it becomes the caller for all callbacks. We don't want doocs-get and conversion to bool happen then.
Maybe extend the test to be safe in that case?
There was a problem hiding this comment.
I am not sure if I fully understand the concern. It would be only a problem, if you specify the property as its own is-writeable source, right? Maybe we should just prevent that.
There was a problem hiding this comment.
My concern is that for the dynamic property, a callback via subscribeToSharedPV(isWriteableControl) will also be registered, and since isWriteableControl is the key in the global map of callbacks, it collects both callbacks under the same key, possibly leading to the confusion which one should be called where.
There was a problem hiding this comment.
Ok, got it now. I will check whether a property might subscribe itself and if so prevent it.
There was a problem hiding this comment.
I improved it now, I think the code is now better to understand. But I believe this has now removed a "feature". If you map a PV twice into a writeable property, I think we have no synchronisation between these two any more. Mapping PVs twice writeable triggers a warning already:
**** WARNING: Variable '...' mapped to more than one writeable property. Expect inconsistencies!
Hence I believe this is anyway not a feature intended for production use. The synchronisation is not perfect and may have race conditions (I think). It was used in the test serverTestZeroMQ_noMatchingMPNumber, which I altered now that it no longer relies on this synchronisation (but it still maps the PV twice writeable).
IMO we should throw an exception instead of just printing a warning and change the test such that it does not do this any more.
What do you think? Is this a wanted feature?
2 participants
See: https://redmine.msktools.desy.de/issues/9384