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

add smarpod as a device #1116

Merged
merged 7 commits into from
Feb 28, 2024
Merged

Conversation

patoppermann
Copy link

@patoppermann patoppermann commented Feb 13, 2023

Description

Add a new device class for smarpod and detailed UI.

Motivation and Context

We have a new device from SmarAct, this time a SmarPod.

How Has This Been Tested?

IOC connection was tested.

Where Has This Been Documented?

Doc in the Code

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@tangkong
Copy link
Contributor

I think this looks mostly good aside from styling issues. Try running pre-commit checks locally: Confluence page

(dev) $ pre-commit install  # run this only once
(dev) $ git add pcdsdevices/my_device.py   # add new things to the staging area
(dev) $ pre-commit run  # manually run pre-commit




class SmarPod(BaseInterface, Device):
Copy link
Contributor

Choose a reason for hiding this comment

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

This class will not play nicely with bluesky for the purposes of scanning as you've implemented it as a generic ophyd.Device.

There needs to be a Positioner somewhere for the smarpod.

More details here: https://confluence.slac.stanford.edu/display/PCDS/How+to+Create+an+Ophyd+Device#HowtoCreateanOphydDevice-PVPositioner

Consider the upstream source of this SmarPod IOC implementation and its corresponding ophyd device: https://github.com/NSLS-II-HXN/profile_collection/blob/d724c469b8c4ac1784ca755b40b1992b3e5d0640/startup/15-zp.py#L15-L38 (and further down where it's being used as a sub-device)

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, it looks like NSLS-II splits this kind of device into many different 1D positioners

Copy link
Author

Choose a reason for hiding this comment

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

PVPositioner was chosen as the class.
from ophyd import PVPositioner

@patoppermann patoppermann requested a review from klauer March 8, 2023 21:32
from ophyd.signal import EpicsSignal, EpicsSignalRO


class SmarPod(PVPositioner):
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you given this a try, @patoppermann? Like running it in Python and instantiating a SmarPod instance?

I ask because PVPositioner requires some components to be defined, which I don't see here: https://confluence.slac.stanford.edu/display/PCDS/How+to+Create+an+Ophyd+Device#HowtoCreateanOphydDevice-PVPositioner

PVPositioner should raise an exception if those are not set: see here

I'm not sure if a single PVPositioner will work for the SmarPod, since it has 6 DOF (x/y/z/rx/ry/rz) to check. Normally PVPositioner works with scalars, but maybe...

ver_firmware = Cpt(EpicsSignalRO, ':VER:FIRMWARE', kind='omitted')
# Status
cmd_read_status = Cpt(EpicsSignal, ':CMD:READ_STATUS', kind='normal')
status_temp = Cpt(EpicsSignalRO, ':STATUS:TEMP', kind='omitted')
Copy link
Contributor

Choose a reason for hiding this comment

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

All of this is still really jumbled together. Consider that it might make more sense to shove status-related things into a SmarPodStatus device, which could then be instantiated with a Cpt(SmarPodStatus, ":STATUS") much like how I was talking about the poses.

execute = Cpt(EpicsSignal, ':EXECUTE', kind='normal', doc='Enacts Pose Positions')


class Smarpod(Device):
Copy link
Contributor

Choose a reason for hiding this comment

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

There's SmarPod above and Smarpod here - two different classes.
The above one has everything but the poses. I think these should be merged somehow?

@klauer
Copy link
Contributor

klauer commented Mar 10, 2023

By the way, we have a page here: https://confluence.slac.stanford.edu/display/PCDS/How+to+add+a+device+to+pcdsdevices#Howtoaddadevicetopcdsdevices-Testing

Which goes through at least a bit of the process of instantiating your device and trying it out

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

Hi, I have two minor suggestions.

It won't be possible to use this in a normal scan, but it's good that you've accumulated all the PVs. Perhaps in a follow-up we can add scanning behavior.

Copy link
Member

Choose a reason for hiding this comment

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

I think you should remove this file unless you're planning to make a screen

@ZLLentz
Copy link
Member

ZLLentz commented Feb 28, 2024

This should be good to go
There may be more adjustments later for various hutch needs as they arise but there is no reason to let it sit for another year

@ZLLentz ZLLentz merged commit 3e5005d into pcdshub:master Feb 28, 2024
9 checks passed
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