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.

Sign up for GitHub

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

Jump to bottom

432 create sample stage device for p99 #458

Merged
merged 11 commits into from
Aug 14, 2024
Merged

432 create sample stage device for p99 #458

merged 11 commits into from
Aug 14, 2024

Conversation

Relm-Arrowny
Copy link
Contributor

@Relm-Arrowny Relm-Arrowny commented Apr 23, 2024 •
edited

Fixes #432 see p99#7 and p99#13 for full detail

Added :

  • Lab xyz-stage and labxyz-stage for p99.
  • The angles pitch roll and theta are not standard motors, they only have set and feedback and it is not stopable. so a SetReadOnlyMotor device is added to capture this.
  • p99 Filter wheel is added as a FilterMotor device with an enum class for the selections.

Instructions to reviewer on how to test:

  1. Unit test should cover all of the devices.
  2. IRL test can be done on p99 using bluesky and this Jupiter doc.

Checks for reviewer

  • Would the PR title make sense to a scientist on a set of release notes
  • If a new device has been added does it follow the standards

@Relm-Arrowny Relm-Arrowny linked an issue Apr 23, 2024 that may be closed by this pull request
Create Sample stage device for p99 #432
Closed
7 tasks
DominicOram
DominicOram requested changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Sorry to jump in here without knowing much background. I've put some minor comments in the code. In general I'm not sure I like that we have to have a SetReadOnlyMotor as I'm not sure this is the right solution in general. Has the possibility of wrapping a SoftMotor around it in IOC been discussed with controls? Then the implementation details get hidden from the ophyd layer and up.

src/dodal/beamlines/p99.py Show resolved Hide resolved
src/dodal/devices/stages.py Outdated Show resolved Hide resolved
src/dodal/devices/epics/setReadOnlyMotor.py Outdated Show resolved Hide resolved
src/dodal/devices/epics/setReadOnlyMotor.py Outdated Show resolved Hide resolved
src/dodal/devices/p99/sample_stage.py Outdated Show resolved Hide resolved
@Relm-Arrowny Relm-Arrowny mentioned this pull request Apr 24, 2024
Closed
2 tasks
DominicOram
DominicOram requested changes Jun 27, 2024
View reviewed changes
Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Sorry, I completely forgot that I started reviewing this. One comment, it also looks like it won't work with the latest main as beamline_utils has been renamed. Otherwise happy to merge.

src/dodal/devices/stages.py Outdated
from ophyd_async.epics.motion.motor import Motor


class ThreeAxisStage(Device):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should: Can we combine this with XYZPositioner

@DominicOram
Copy link
Contributor

Do you still need this @Relm-Arrowny? Would be good to get it merged if useful or dropped otherwise, just so we don't have too many stale PRs

@codecov Codecov
Copy link

codecov bot commented Aug 14, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.49%. Comparing base (ac220bb) to head (6794a2f).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #458      +/-   ##
==========================================
+ Coverage   94.43%   94.49%   +0.05%     
==========================================
  Files         112      114       +2     
  Lines        4531     4579      +48     
==========================================
+ Hits         4279     4327      +48     
  Misses        252      252

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Relm-Arrowny
Copy link
Contributor Author

Sorry p99 is being move and this slipped my mine, it should be good to go.

DominicOram
DominicOram approved these changes Aug 14, 2024
View reviewed changes
Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Great, thank you! I couldn't test that the PVs are right as I couldn't ssh into P99, maybe it's down? So going to trust you on that. Otherwise just one suggestion, which I will fix myself quickly

src/dodal/devices/motors.py Outdated
Comment on lines 23 to 27
xyz_stage = ThreeAxisStage("BLXX-MO-STAGE-XX:")
Or::
with DeviceCollector():
xyz_stage = ThreeAxisStage("BLXX-MO-STAGE-XX:", suffix = [".any",
".there", ".motorPv"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Should: Docs now reference the old device name, I will fix

@DominicOram DominicOram merged commit f8c493e into main Aug 14, 2024
16 checks passed
@DominicOram DominicOram deleted the 432-create-sample-stage-device-for-p99 branch August 14, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DominicOram DominicOram DominicOram approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create Sample stage device for p99
2 participants
@Relm-Arrowny @DominicOram