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

SR-570 race condition #917

Merged
merged 2 commits into from
Apr 4, 2024
Merged

Conversation

canismarko
Copy link
Collaborator

Fixes a race condition where the SR570 pre-amp gets the sensitivity_value set before the sensitivity_unit, ending up at the wrong gain.

If the gain is set to 1 mA/V in the IOC, and you try to set it to e.g. 200 µA/V, you need to set the unit first then the value since "1" is the only valid "mA/V" value:

✓ 1 mA/V -> 1 µA/V -> 200 µA/V
✗ 1 mA/V -> 200 mA/V (corrects to 1 mA/V) -> 1 µA/V

Solved this problem by using put_complete=True for the sensitivity_unit signal.

@canismarko
Copy link
Collaborator Author

Want to check this one last time at the beamline, then it will be ready for review.

@canismarko canismarko added the bug label Jan 25, 2024
@canismarko canismarko changed the title Srs570 race condition SR-570 race condition Jan 25, 2024
@prjemian
Copy link
Contributor

Tests ran with errors here.

@canismarko
Copy link
Collaborator Author

Tests ran with errors here.

You mean locally? The CI looks like it's just that weird decrease in test coverage.

@prjemian
Copy link
Contributor

Tests ran with errors here.

Meant to say: "Tests ran with no new errors here."

@canismarko canismarko marked this pull request as ready for review March 22, 2024 21:25
@canismarko
Copy link
Collaborator Author

Finally had a chance to test this at the beamline. Totally didn't forget about it and just noticed it was still in draft while I was looking for something else. Anyway, I think it works, best as I can tell.

I swapped the gain back and forth a hundred or so times in a while loop, and they always got set in the right order.

@canismarko
Copy link
Collaborator Author

Any thoughts about merging this PR?

Copy link
Contributor

@prjemian prjemian left a comment

Choose a reason for hiding this comment

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

LGTM

@prjemian
Copy link
Contributor

prjemian commented Apr 4, 2024

@canismarko - You can merge now

@prjemian prjemian added this to the 1.6.19 milestone Apr 4, 2024
@canismarko canismarko merged commit 047daf4 into BCDA-APS:main Apr 4, 2024
11 of 13 checks passed
@canismarko canismarko deleted the srs570_race_condition branch April 4, 2024 13:29
prjemian added a commit that referenced this pull request Apr 5, 2024
MDecarabas pushed a commit that referenced this pull request Apr 12, 2024
MDecarabas added a commit that referenced this pull request Apr 12, 2024
* Added edge_align plan for beam edge alignment

* Need to find a better way to check signal quality.

* Formatted using ruff

* small formatting changes

* removed unused stats import

* removed another unused import

* Added a check function that yields correct results

* intermediate commit saving progress

* finished try/except when bad signal is detected

* Fixed formatting

* Added scipy to environment.yaml & requirements.txt

* CI #909 switch to 'ruff'

* DOC #909 update release notes

* MNT #909 revise per 'ruff check'

* CI #909 update

* Bump davidslusser/actions_python_ruff from 1.0.0 to 1.0.1

Bumps [davidslusser/actions_python_ruff](https://github.com/davidslusser/actions_python_ruff) from 1.0.0 to 1.0.1.
- [Release notes](https://github.com/davidslusser/actions_python_ruff/releases)
- [Commits](davidslusser/actions_python_ruff@v1.0.0...v1.0.1)

---
updated-dependencies:
- dependency-name: davidslusser/actions_python_ruff
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* MNT #872 hoist from MPE devices

* TST #872 adjust for new hoist

* MNT #872

* MNT #872 refactor

* MNT #872 hoist utils

* DOC #912

* ENV #938 bump the minimum requirement

* DOC #932 update relase notes

* DOC #938 contribution from @sveseli

* MNT #914 setup.py --> pyproject.toml

* DOC #921 copyright year

* MNT #914

* Added edge_align function for beam edge alignment

* added scipy to pyproject.toml

* CI #909 switch to 'ruff'

* DOC badge

* DOC badge

* DOC #940 example scans with sscan record

* DOC #940 it's only a word ...

* DOC #940

* DOC #940 it's a demo, actually

* DOC #940

* DOC #940 convert the blocking call to st.wait()

* DOC #940 per review

* DOC #940 per review

* DOC #940 proofreading

* DOC #940 per review, remove the section with the polling loop

* DOC #940 new title

* DOC #940 more proofing

* DOC #940 clarify the default sscan record array size of 1,000

* DOC #940 refactor to polling loop. Show the recommended style in a details section.

* DOC #940

* Added put_complete=True to sensitivity value and unit for the SRS-570 Pre-amp.

* Added a test for the sensitivity_unit put_complete, and removed the put_complete from sensitivity_value.

* CI #955 move requirements file

* MNT #955 sphinx config

* DOC #955 re-arrange

* DOC #955 apply to lower-level pages

* DOC #955 add docs upload

* CI #955

* CI #955

* CI #955 wip

* CI #955

* CI #955

* DOC #917 release note

* DOC #957 merge overview into home page

* DOC #955

* DOC #955

* DOC #955

* DOC #955

* Added edge_align function for beam edge alignment

* Added edge_align function for beam edge alignment

* Added edge_align function for beam edge alignment

* DOC #909 update release notes

* DOC #932 update relase notes

* Added edge_align function for beam edge alignment

* CI #909 switch to 'ruff'

* DOC #955

* Fixed merge issue

* fixed unused import

* change for tests to pass

* Formatting changes

* Fixed repeated section

* Small formatting changes

* Small syntax change

* Added toolz & scipy to conf.py

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Codrea <ecodrea@nefarian.xray.aps.anl.gov>
Co-authored-by: Pete R Jemian <prjemian@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Pete R Jemian <prjemian@users.noreply.github.com>
Co-authored-by: Mark Wolfman <canismarko@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants