ChimeraTK / ControlSystemAdapter

An adapter layer which allows to use control applications with different control system software environments.
GNU Lesser General Public License v3.0
3 stars 2 forks source link

getProcessArray() should throw exception if variable does not exist #8

Closed mhier closed 3 years ago

mhier commented 7 years ago

ControlSystemPVManager::getProcessArray() currently returns a nullptr if a non-existent process variable has been requested. In analogy to DeviceAccess an exception should be thrown (preferably the same exception as in DeviceAccess when an unknown register is requested). This also is the much better and much safer interface than returning a nullptr which are possibly not checked for. Other similar functions might require the same change.

This is of course an incompatible interface change, but the impact should not yet be big.

smarsching commented 7 years ago

I agree with this change and can see how it might be useful. My only concern is that we should have a very specific exception for signalling that the process variable has not been found. I can think of cases where one might want to treat such a situation as non-fatal and thus it should be easy to catch the exception without also catching exceptions that are caused by "real" errors.

mhier commented 7 years ago

We have to see. Currently we have too many exceptions in ChimeraTK, especially in DeviceAccess. To properly treat the situation it is enough that the thrown exception is only thrown for this particular error type inside that function (getProcessArray()). I don't think we should introduce a specific exception type for this, because this only leads again to a big mess of exceptions. Moreover, I don't really think this is an exception any application should run into unless something has gone really badly. If the application doesn't know which process variables are there, it should look into the list instead of making guesses. If the application assumes a certain variable is there but it isn't, the application logic has gone wrong already before. Please give an example, if you think there is some actual use for it.

smarsching commented 7 years ago

Yes, the most important thing is that there is a safe way for specifically dealing with such an error and being sure that the same exception is never thrown (by the same method) in case of other problems. In general, I do not mind having many different exceptions as long as they are properly organized in a hierarchy that only has few exceptions (in the best case only or or two) at the top level.

The use case where it might be useful to catch the exception caused by a non-existing process variable is when an application expects certain optional process variables. For example, a certain device might have a naming convention in which a setpoint PV is identified by the suffix "_setpoint" and the corresponding (optional) readback PV is identified by the suffix "_readback". In such a case, the application layer might want to probe for each configured PV whether there is a readback PV in addition to the setpoint PV. Of couse, one could also get the whole list of process variables and check this list to find out whether the process variable exists, but such a check would be O(n) instead of O(1).

On the other hand, we could also provide a hasProcessArray method for explicitly checking whether a certain process variable exists. Such a check would be O(1) because we internally use a hash-map.

mhier commented 7 years ago

I do mind many exceptions for the simple reason that they are part of the interface, but they easily expose too much detail of the underlying implementation. It usually doesn't matter for what reason a certain action failed, especially if there is no way for the application to circumvent the problem.

I would certainly be in favour of introducing something like hasProcessArray(), but we should try to match the interface of this functionality with DeviceAccess. There we have a catalogue of all registers (= process variables) which provides this kind of functionality.

mhier commented 3 years ago

long solved.