Add a Eurotherm device

[awalter-bnl]

This PR will add a new Eurotherm device to replace the seperate Eurotherm devices in the PDF and XPD profiles.

Description

This will add a replacement to Eurotherm device for the XPD and PDF beamlines (and any others who will use these devices in the future). It resolves an issue where the IOC indicates to Ophyd that it is 'done moving' when it first reaches the setpoint temp not after it has reached eqilibrium at the setpoint temp. This is doen using the existing tolerance property of the EpicsSignalPositioner, of which the device is a child, and by adding to new properties, equilibrium_time and timeout. The latter is passed into the status object to indicate how long to wait for equilibrium before raising a TimeOutError. equilibrium_time is used to indicate how long the temperature should remain with tolerance of the setpoint before it is considered at equilibrium.

Motivation and Context

The Eurotherm device has the tendency to overshoot the temperature setpoint prior to ataining equilibrium. The IOC indicates that it is done changing temperature when the setpoint is first reached, this means that bluesky is moving onto the next point in a scan when the temperature has not reached equilibrium. The controls group have indicated that this can not be resolved at the IOC, or lower, level therefore I am implementing this solution in the ophyd layer.

How Has This Been Tested?

This has yet to be tested, once it has been reviewed and approved I will test at either XPD or PDF on the device prior to releasing the Do Not Merge label and allowing for merging.

[awalter-bnl]

I am still not 100% clear on how status objects work. In this case I am not 100% sure that:

1. If the if statement on line 75 fails will it return to checking the if statement on line 73 or not.
2. Will the timeout be implemented effectively (i.e. when does the timing of the timeout start).

Feedback on this or any other items is welcomed

[tacaswell]

https://github.com/NSLS-II/caproto/pull/343 <- that may be useful for testing this (modulo fixing some names).

[awalter-bnl]

thanks for the link @tacaswell I will check it out.

[awalter-bnl]

I have made the changes requested by @tacaswell except:

1. changing equilibrium_time to settle_time. @tacaswell are you now convinced that they are different enough to warrant different names?
2. the issue of blocking of libca/caproto event loops, I am still investigating the issue in order to find possible solutions (comments/suggestions on this are welcomed)

[awalter-bnl]

The latest commit is after I have tested against the simulated temp controller @tacaswell wrote in a caproto PR here. My next step is to add a test using the caproto simulated temp controller.

[tacaswell]

Might be worth adding some locking so that if there is already a set in progress, we can not start a second one? Have to be very careful about it getting cleaned up in stop though.

We should also add a stop method that sets it to it's current value. This isn't great but seems like the best way to stop a temperature controller.

[awalter-bnl]

@tacaswell I have made the requested changes, now to work on the test function !-)

[tacaswell]

We probably want to copy / import a fixture like https://github.com/NSLS-II/caproto/blob/956e7870ecc79bf6308f5b23413c55f7bfa91e51/caproto/tests/conftest.py#L234-L249 so that the test suite will start up the test IOC and we can at least smoke test this!

I think it may be worth moving the example IOC into this repo to prevent weird inter-project dependencies.

[awalter-bnl]

@tacaswell thanks for the link it is very helpful, I have a question about:

I think it may be worth moving the example IOC into this repo to prevent weird inter-project dependencies.

What do you mean by 'example IOC'? Do you mean just a copy of the pytest_fixture found in your link above, the pytest_fixture and a copy of run_example_ioc() or the entire simulated thermo IOC from caproto PR # 343.

It is not 100% clear to me which you mean.

[tacaswell]

What do you mean by 'example IOC'? Do you mean just a copy of the pytest_fixture found in your link above, the pytest_fixture and a copy of run_example_ioc() or the entire simulated thermo IOC from caproto PR # 343.

I importing the pytest fixtures and helpers in probably ok (those are likely to not change in breaking ways anymore due to their heavy use in caproto testing already), but we should copy the IOC from https://github.com/NSLS-II/caproto/pull/343 over in entirety (as it is just an example and nothing makes sure we don't change some small detail of the example to make it a better example that would make it badly suited for the tests here).

[awalter-bnl]

I have removed the DO NOT MERGE tag, as it was discussed this morning that an version update to nslsii is imminent and wanted this included. I will add a test function via a furture PR (the current code has been tested manually using the new simulated temp controller).

[awalter-bnl]

I have added a test for the new device, this should be reviewed before merging.

[awalter-bnl]

closed and re-opened to start a Travis CI build

[codecov-io]

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@67833bf). Click here to learn what that means. The diff coverage is 94.44%.

@@            Coverage Diff            @@
##             master      #41   +/-   ##
=========================================
  Coverage          ?   40.53%           
=========================================
  Files             ?        5           
  Lines             ?      449           
  Branches          ?        0           
=========================================
  Hits              ?      182           
  Misses            ?      267           
  Partials          ?        0

Impacted Files	Coverage Δ
nslsii/tests/temperature_controllers_test.py	100% <100%> (ø)
nslsii/temperature_controllers.py	91.17% <91.17%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 67833bf...0362505. Read the comment docs.

[awalter-bnl]

FYI I am having some issues getting the test to run in Travis. It runs locally without issue, I am trying to work through the problem now.

[awalter-bnl]

OK so I have now managed to get the test to pass, So this is ready for review. (FYI the issue was that I didn't have caproto as a requirement.)

[awalter-bnl]

thanks @mrakitin for letting me know about the requirements list ordering. That is now fixed

[awalter-bnl]

@mrakitin I have made the requested changes, but there is one outstanding comment to be resolved above.

[awalter-bnl]

I do not see a show-stopping bug here, @danielballan can you elaborate?

[danielballan]

Elaborated in thread above.

[tacaswell]

The timer class @danielballan is talking about is https://docs.python.org/3.6/library/threading.html#timer-objects

The logic would be to if it is in-band and there is no timer arm it if there is a timer move on, if we ever go out of band cancel it and reset.

The callback on the timer would flip the status object and pulls the callback off the readback.

I am 70/30 on which way is better. Dan is right in principle we may never see the updated value, but in practice this IOC runs with the SCAN value set to some period.

[tacaswell]

To be clear, the 70 is towards Dan's suggestion.

[awalter-bnl]

Ok so just to be 100% clear, this would only work if I remove the timeout kwarg here.

As this is essentially placing the timeout logic into status_indicator? am I correct.

The only issue I sort of see with this is that it 'restarts' the timer everytime it first goes in-band, then we are constantly moving the start time for the timeout. So in principle it could take much longer than timeout before raising an error. This is a subtle change that won't be immediately apparent, and could cause some confusion.

perhaps a better logic is to run the timer at the start of status_indicator if one doesn't exist, and not tie it to being in-band, as then it keeps the more usual logic of timeout but still ensures that the callback is pulled of the reader after timing out? what do others think?

[tacaswell]

This is orthogonal to the timeout on the status object its self (which is set up at https://github.com/NSLS-II/ophyd/blob/b907a8c967a8eb4353666b8dd632d87bd1716e2c/ophyd/status.py#L66-L70 ). That logic is "if after the time out time we have not otherwise completed, unconditionally clean up and fail". This as separate time to succeed.

[awalter-bnl]

I am confused by that statement @tacaswell, to me the result of setting timeout=sum_value in status here is equivalent to setting it as None there and adding a timer object at the beginning of status_indicator with the interval being sum_value and the function being:

def f(): ``try: ``self._handle_failure() ``finally: ``status._finished(success=False)

Am I missing something about this?

My suggestion is to just add the following line above the try: line in f above so that it ensures that the callback is pulled from self.readback:

self.readback.clear_sub(status_indicator)

[mrakitin]

Agreed with @awalter-bnl to merge it as is. The discussed fixes will come in the next PR.