Check side effects of #116

[killenb]

Child of ChimeraTK/ApplicationCore#123
Depends on #116

Until now readLatest() and readNonBlocking() the postRead() function has not been called if there was no new data. This is wrong, there might be stuff to do like exception handling.
In #116 postRead() got a new argument hasNewData. This has to be used in each implementation to device if action is required if there is new data.

• Check each postRead() implementation if the action should always take place, or only if there is new data.
• Note in this ticket which places have been changed.

#122 is already dealing with the generation of new version numbers in NumericAddressedLowLevelTransferElement and LNMBackendVariableAccessor.

Examples

• If preRead() is empty, and post read swaps the new data into the user buffer, this must not happen because the old user buffer must stay intact => no action if no new data
• If preRead() swaps out the user buffer such that the transfer can modify it, which it does not do because there was no new data, it has to be swapped back to get the user buffer back into place => action always required, even if no new data

[mhier]

List of all doPostRead() implementations (via grep "void *doPostRead" -R .):

• ./device_backends/LogicalNameMapping/src/LNMMathPlugin.cc -> unconditionally touches user buffer -> changed, only update if hasNewData==true (change has no effect on tests)
• ./device_backends/LogicalNameMapping/src/LNMMultiplierPlugin.cc -> unconditionally touches user buffer -> changed, only update if hasNewData==true (change has no effect on tests)
• ./device_backends/LogicalNameMapping/src/LNMMonostableTriggerPlugin.cc: always throws -> no change required
• ./device_backends/LogicalNameMapping/include/LNMBackendChannelAccessor.h -> unconditionally touches user buffer -> changed, only update if hasNewData==true (change has no effect on tests)
• ./device_backends/LogicalNameMapping/include/LNMBackendBitAccessor.h -> unconditionally touches user buffer -> changed, only update if hasNewData==true (change has no effect on tests). Note: locking-logic flawed (-> LNMBackendBitAccessor lock logic flawed #129)
• ./device_backends/LogicalNameMapping/include/LNMBackendVariableAccessor.h: read transfers always succeed -> no change required
• ./device_backends/include/NDRegisterAccessorDecorator.h -> unconditionally touches user buffer -> changed, only update if hasNewData==true (change has no effect on tests)
• ./device_backends/include/TransferElement.h: empty default implementation -> no change required
• ./device_backends/include/NumericAddressedLowLevelTransferElement.h -> sets new VersionNumber unconditionally -> changed, only update if hasNewData==true (change has no effect on tests)
• ./device_backends/include/NumericAddressedBackendMuxedRegisterAccessor.h -> unconditionally touches user buffer -> changed, only update if hasNewData==true (change has no effect on tests)
• ./device_backends/include/CopyRegisterDecorator.h -> unconditionally touches user buffer -> changed, only update if hasNewData==true (change has no effect on tests)
• ./device_backends/include/NumericAddressedBackendRegisterAccessor.h -> unconditionally touches user buffer -> changed, only update if hasNewData==true (change has no effect on tests)
• ./device_backends/Subdevice/src/SubdeviceBackend.cc: FixedPointConvertingDecorator -> unconditionally touches user buffer -> changed, only update if hasNewData==true (change has no effect on tests)
• ./device_backends/Subdevice/include/SubdeviceRegisterAccessor.h -> unconditionally touches user buffer -> changed, only update if hasNewData==true (change has no effect on tests)
• ./tests/executables_src/testTransferGroup.cpp -> test specific (just count calls), unchanged.
• ./tests/executables_src/testAsyncRead.cpp -> unconditionally touches user buffer -> changed, only update if hasNewData==true (change has no effect on tests)
• ./tests/executables_src/testDataConsistencyGroup.cpp -> empty implementation, unchanged.

[mhier]

I now realise, there are two aspects:

• prevent post-read action in case of an exception
• prevent post-read action in case of successful read transfer but without new data (exists only with AccessMode::wait_for_new_data)

The first aspect might be intrinsically covered if e.g. a delegated postRead() (without do) is called before the action, because that one will throw the exception and hence prevent the action. Still the second aspect need to be covered, unless the implementation never can have AccessMode::wait_for_new_data - but this is usually not the case for decorator-like implementations.

There might be additional aspects, also for write operations: Some actions might be necessary despite an exception is thrown in a delegated call. This is not covered by the ticket description. Is there a separate ticket for this?

[mhier]

need to do this also for write (see my last comment). will do this as part of this ticket.

[mhier]

List of all doPostWrite() implementations:

• ./device_backends/LogicalNameMapping/src/LNMForceReadOnlyPlugin.cc-> only throws logic_error, no change required
• ./device_backends/LogicalNameMapping/src/LNMMathPlugin.cc -> only delegates, no change required
• ./device_backends/LogicalNameMapping/src/LNMMultiplierPlugin.cc -> only delegates, no change required
• ./device_backends/LogicalNameMapping/src/LNMMonostableTriggerPlugin.cc -> empty, no change required
• ./device_backends/LogicalNameMapping/include/LNMBackendBitAccessor.h -> only delegates, no change required (apart from follow up ticket about locking, see above)
• ./device_backends/include/NDRegisterAccessorDecorator.h -> swaps user buffer back -> changed, finally-construct added
• ./device_backends/include/TransferElement.h -> empty default implementation
• ./device_backends/include/NumericAddressedBackendRegisterAccessor.h -> multiple implementations, all are ok
• ./device_backends/Subdevice/src/SubdeviceBackend.cc -> only delegates
• ./device_backends/Subdevice/include/SubdeviceRegisterAccessor.h -> already ok
• ./tests/executables_src/testTransferGroup.cpp -> test specific, no change
• ./tests/executables_src/testAsyncRead.cpp -> empty implementation
• ./tests/executables_src/testDataConsistencyGroup.cpp -> empty implementation

[mhier]

Btw: we can use a "finally" construct to avoid try-catch-blocks and code duplication. I have copied it from https://github.com/microsoft/GSL to cppext (to avoid another dependency). Hence we need a new version of cppext.

[mhier]

write part also checked and fixed.