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

Automatically open the shutters when executing plans #338

Merged
merged 16 commits into from
Jan 12, 2025
Merged

Conversation

canismarko
Copy link
Contributor

Things to do before merging:

  • add tests
  • write docs
  • update iconfig_testing.toml
  • flake8, black, and isort
  • test with queueserver
  • test with Firefly

@canismarko canismarko added documentation Improvements or additions to documentation enhancement New feature or request device support Issues that allow support for certain kinds of hardware labels Jan 4, 2025
@canismarko
Copy link
Contributor Author

canismarko commented Jan 4, 2025

Tests failing in test_energy_xafs_scan.py::test_exafs_k_range, seems unrelated to the changes in this PR. I'll look closer.

@canismarko canismarko changed the title Auto shutter Automatically open the shutters when executing plans Jan 5, 2025
@canismarko canismarko requested a review from yannachen January 7, 2025 19:56
Copy link
Collaborator

@yannachen yannachen left a comment

Choose a reason for hiding this comment

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

The configBits for XIA filters/shutters seem convenient for the code logic. However, multiple filters did not work simultaneously. Is it theoretically possible to update this defect in Bluesky or Epics?

from haven.devices import ShutterState

shutter = filter_bank.shutters[0]
await shutter.set(ShutterState.CLOSED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

When we set it to "CLOSED," do filters 2 and 3 close simultaneously or one by one? As Debora and I discussed, the shutter should close simultaneously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, both filters should move together. The steps are:

  1. Read the current config bits (e.g. "0001")
  2. Figure out the new config bits by setting only bits 2 and 3 (.e.g "0010")
  3. Write the new value to the PV

This way, they should move together.

@@ -85,29 +85,54 @@ def load(
beamline = HavenInstrument(
{
# Ophyd-async devices
"ion_chamber": IonChamber,
"aerotech_stage": AerotechStage,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this "aerotech_satege" using old electronics or Automation1? Or does it work for anyone similar to Automation1, even if we update some years later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the old one. I'll get rid of it. We can add new support when Automation1 is ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I looked and it should work for both. The fly-scan support is commented-out, so as long as the Automation1 support is just two motors, it doesn't matter. Plus it's already done in ophyd-async so we can just add the fly-scan support when we get there.

return decorator


all_decorators = chain(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the "decorators" slow the plan operation speed, making scan time longer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think mostly no, with the exception of the open shutters decorator.

The baseline decorator adds a bunch of reads, but I think they all happen in parallel so whichever the slowest will be added to the scan time.

Shutter suspend decorator just listens for changes in the shutter, so not really.

Open shutters decorator depends on the state of the shutters when the scan starts. 1) If all shutters are open, then the extra time is however long it takes to check if they're open (10 ms?). 2) If a slow shutter is closed, it will add the time needed to open the shutter at the start of the scan and close it at the end (~5s). 3) If a fast shutter is closed, it will add the time needed to open and close the shutter to every point. I measured, and #3 adds about 1s to every point.

We should measure, though.

@canismarko
Copy link
Contributor Author

The configBits for XIA filters/shutters seem convenient for the code logic. However, multiple filters did not work simultaneously. Is it theoretically possible to update this defect in Bluesky or Epics?

This one bugged me too, but I'm not sure the right way to fix it. From the comment, I'm thinking you tried something like bps.mv(filter0, FilterPosition.CLOSED, filter1, FilterPosition.CLOSED)?

Right now they use the individual filter open/close signals. These don't work if you set more than one simultaneously, even if you just set the PVs directly so I think it's an IOC question. We could try the config-bits approach for filters too. There's a race condition, though. Let's say the filters have an initial state of "0000" (all open) and we want to close filters 0 and 1. Here are the steps:

  1. Filter0 reads the current config-bits
  2. Filter0 flips it's associated bit
  3. Filter0 writes the new config-bits
  4. Filter1 reads the current config-bits
  5. Filter1 flips it's associated bit
  6. Filter1 writes the new config-bits

Whether this works depends on what filter1 gets in step 4. If step 3 has happened already, then everything is fine, but if step 3 hasn't happened, filter1 gets the old state, updates it and writes the now out of date state back to the device.

@canismarko
Copy link
Contributor Author

Thanks for the review @yannachen. Merging now.

@canismarko canismarko merged commit a879a9a into main Jan 12, 2025
1 check passed
@canismarko canismarko deleted the auto_shutter branch January 12, 2025 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
device support Issues that allow support for certain kinds of hardware documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants