Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Two critical fixes to Eurotherm #58

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

danielballan
Copy link
Contributor

I used this against the caproto.ioc_examples.thermo_sim IOC for the tutorial
and found a couple bugs.

Instead of checking `is_alive()` on each call of the callback, start
the timer outside the callback so it happens exactly once. Using
`is_alive()` raised errors once the timer stopped because it returns
`False` when the thread *is started* but is also dead.
@codecov-io
Copy link

codecov-io commented Apr 15, 2019

Codecov Report

Merging #58 into master will increase coverage by 9.77%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
+ Coverage   42.46%   52.24%   +9.77%     
==========================================
  Files          10       11       +1     
  Lines         584      691     +107     
==========================================
+ Hits          248      361     +113     
+ Misses        336      330       -6
Impacted Files Coverage Δ
nslsii/temperature_controllers.py 93.75% <100%> (+1.91%) ⬆️
nslsii/iocs/tests/test_epstwostate_ioc.py 100% <0%> (ø)
nslsii/_version.py 44.4% <0%> (+1.8%) ⬆️

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 0ff403e...1ad4156. Read the comment docs.

Copy link
Contributor

@awalter-bnl awalter-bnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me

@awalter-bnl
Copy link
Contributor

we should solve the Travis error first

@tacaswell tacaswell closed this May 10, 2019
@tacaswell tacaswell reopened this May 10, 2019
@tacaswell
Copy link
Member

The failure looked like it was due to flake8 issues in code not touched by this PR, power-cycled to re-run against current master to see if that just fixes it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants