BrainardLab / OneLightToolbox

Brainard Lab code for talking to our OneLight boxes
MIT License
1 stars 0 forks source link

Maintaining valid connection with hardware, whose responsibility? #15

Open JorisVincent opened 6 years ago

JorisVincent commented 6 years ago

We control our various hardware peripherals using device driver objects (OneLight, PR670dev). One of the advantages of these, is that they can be passed around between functions, i.e., down the callstack. This creates some questions: /cc @DavidBrainard @gkaguirre

Instantiation

Who is responsible for opening a device driver? Is it the Run[YourProtocolNameHere] top level script? The OLApproach_[YourApproachNameHere]_Engine? For the radiometer(s)/LabJack, should it be the OLValidatePrimary or OLCorrectPrimary function?

The higher level instantiation seems beneficial for several reasons: the user has almost direct control over the specific object to instantiate, and the downstream functions don't have to check/worry about certain aspects (see the point below about real vs. simulated). Moreover, only one device driver object has to be instantiated, since the same object gets passed around. This reduces the potential for errors, hopefully?

On the other hand, we want our functions to be more encapsulated and modular. This could argue for opening up the devices at the lower level, so that, e.g., a call to OLValidatePrimary in the base-OneLightToolbox (not /OLApproachSupport) opens up the radiometer for you. Or we could say that it is the responsibility of the User of the lower level functions to provide valid device drivers. The User can be a User on the command line, or in our case the OLApproachSupport/Engine/Protocol.

Error/exception handling

Who is responsible for error/exception handling? Should any function that interacts with hardware do its own exception handling (i.e., contain try/catch blocks)? It's certainly good form for a low-level function (e.g., OLValidatePrimary) to check that it has valid device drivers passed in, but does it have to contain a try/catch when trying to use a device? If it runs into an exception, it gets passed up the callstack anyway, so a higher level function containing a try/catch could do all the necessary handling when the error reaches it. A few further thoughts on this:

  1. What is the good state that we wish to return our devices to? Do we need to set all mirrors off on the OneLight? If we generate an exception while trying to take a PR6XX measurement, should we try and exit remote mode, or should we try and see if we still have a valid connection?

  2. Exception handling should generally be done in cases where the generated exception can be dealt with, i.e., evaluated and circumvented on runtime by executing some further code, or at least try again / try something else. This is not how we're mainly using it. [Another case where exceptions are often used, is to catch some exception, and provide more information about it before passing it up the stack. But that's also not something we often do.] Instead, we do cleanup: try and return the devices to some good state (which isn't guaranteed to work, since the caught exception could be something like no valid OneLight object), and then rethrow the error.

  3. Matlab also provides cleanupObj = onCleanup(cleanupFun): create an object that lives in the local workspace (of whatever function called onCleanup, i.e., very localized). When the calling function is exited, the workspace is cleared, thus cleanupObj is cleared. When cleanupObj is cleared, it executes whatever you passed as cleanupFun. This would allow us to specify one function e.g. OLCleanup function, which returns the OneLight to a good state, if possible. This would create a more centralized cleanup, and cleaner code than a bunch of try/catches. [It still is the question of which functions should then call the cleanup function]. Plus, the cleanup will get executed on any cleanup, even if the function was exited succesfully (other languages tend to have try... catch... finally, where the finally block is always executed). Thus, another advantage of onCleanup is that in Matlab catch does not get executed on ctrl-c user abort, but cleanupFun still is.

Real vs. simulated devices.

The OneLight driver allows for simulated OneLight objects, which is useful for testing. [To my knowledge, the PR670dev does not, I do not know about the other Radiometer driver classes.] Several functions also take in a 'simulate' flag, to simulate some behavior. This seems like double specification, and could lead to inconsistent specification: trying to take measurements of a simulated OneLight, or simulated measurements of a real OneLight. Moreover, it requires a bunch of if simulate ... blocks in all the downstream functions as well.

I'd argue that it would be nicer to have the intermediate level functions such as all the OLApproachSupport, and even OLValidatePrimary to be agnostic as to whether they have a real or simulated object, and thus remove the 'simulate' flag. For a simulated OneLight this should not be a problem, there is no reason to check if the OneLight is simulated or not. For simulated radiometers, it means that at least one lowlevel function carries the responsibility for providing valid simulated output. In all the code that I am working with so far, any code trying to measure a specific set of starts and stops converges onto OLTakeMeasurementOOC, so I nominate that as the function responsible for simulated radiometer output (by calling some yet to be created OLStartsStopsToSPD, or modifying OLTakeMeasurementOOC to take in a vector of primary values rather than starts and stops)

JorisVincent commented 6 years ago

Created OLMeasurePrimaryValues function as the 'core' function to measure a (set of) vector of primary values.

Currently, if radiometer input is left empty, it will simulate. I think it would be cleaner if we can pass in a simulated radiometer (see original comment of this issue). @npcottaris can you add a simulate property to the Radiometer objects in the BrainardLabToolbox? I don't think I have a commit/push authorization to that one... (cc @DavidBrainard, @gkaguirre we talked about this a while back, I just never got around to doing it).

DavidBrainard commented 6 years ago

OK, let's add this.