SmarAct UI overhaul by aberges-SLAC · Pull Request #1320 · pcdshub/pcdsdevices

aberges-SLAC · 2025-01-30T03:33:01Z

Description

Adding new SmarAct screens to help with day-to-day ops:

Detailed screen for closed loop SmarActs (includes PicoScale enabled stages, excludes SmarPod).
Open-loop tip-tilt embedded screen with minimal footprint to improve user experience during alignment (have this open as a floating window with camViewer)

I made the bulk of the UI in designer and (mostly) manually labeled the signals 🤮. Noticed in #430 that the discussion for naming signals with human-readable text has come up a few times. I think Ken had a good point that I should probably put the dotted_name signals back in as a tooltip for hutch-python friendliness

Motivation and Context

While fixing #1275 we realized it was time to update the SmarAct screens to improve user readability.

How Has This Been Tested?

I've tested this on some live devices for linear & tip-tilt (open-loop) in the LRL as well as the picoscale devices in the FTL (with permission). Everything appears to work as intended with the live devices.

One small hang-up is that launching the SmarAct.detailed.py screen from typhos using CLI triggers some complaints:

ERROR - Unable to find signal for <PyDMChannel (sig://${name}_motor_egu)> in signal registry.Use typhos.plugins.register_signal()
ERROR - Unable to find signal for <PyDMChannel (sig://${name}_description)> in signal registry.Use typhos.plugins.register_signal()

Since the ui file isn't having the macros expanded before it opens. I have to wait until add_device processes for the macros to be expanded, so none of the other signals appear to trigger it.

Both the EGU and DESC fields read/write regardless of this complaint.

Now I could fix this by emptying out all the channel slots on all the elements in the ui file then just slot the signals in the .py but it would be super annoying and messy, I think. Also, the ui works by itself for any non-picoscale related system, so I'd hate to break it.

Where Has This Been Documented?

This PR

Screenshots (if appropriate):

Some vids showing off the GUI

smaract_embedded_tip_tilt.mp4

smaract_picoscale_screen.mp4

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

…ick behavior from TyphosPositionerRow

…tion

…ude optional picoscale

aberges-SLAC · 2025-01-30T03:33:39Z

I know this is a really large PR due to the size of the pyqt scripts/uis, so feel free to break it up (perhaps I should've submitted 2 different PRs, honestly)

ZLLentz · 2025-01-30T18:44:15Z

+    def __init__(self, parent=None, **kwargs):
+        super().__init__(parent=parent)
+
+        self.ui = typing.cast(_SmarActDetailedUI, uic.loadUi(self.ui_template, self))


I'm a little surprised you'd need to invoke the uic loader manually here. Naively I'd expect PyDM to do this for you in the super call:
https://github.com/slaclab/pydm/blob/ba4125d479a2e6adb033e9ab1a05ab2bab2fdf84/pydm/display.py#L315-L316

Perhaps this is why your macros aren't being expanded?

Definitely why the macro isn't being expanded, but also I can't omit this line or else self.ui never populates. Not sure why the super() call doesn't proc it

Okay figured out why pydm.Display wasn't loading the UI correctly. Got that remedied, but now I can't quite grab my object names. Might need to fenagle a bit more of the code to fix that

This sadly isn't why the macro isn't being expanded. Got the super call to work with:

class SmarActDetailedWidget(Display, utils.TyphosBase): """ Custom widget for managing the SmarAct detailed screen """ ui: _SmarActDetailedUI ui_template = path.join(path.dirname(path.realpath(__file__)), 'SmarAct.detailed.ui') def __init__(self, parent=None, ui_filename=ui_template, **kwargs): super().__init__(parent=parent, ui_filename=ui_filename)

but still get greeted with the complaints when the expert screen opens:

[2025-01-30 16:17:39] - INFO - Loading Tools ... [2025-01-30 16:17:39] - INFO - Adding devices ... [2025-01-30 16:17:42] - INFO - Launching application ... [2025-01-30 16:17:45] - INFO - Loading Tools ... [2025-01-30 16:17:45] - INFO - Adding devices ... [2025-01-30 16:17:45] - ERROR - Unable to find signal for <PyDMChannel (sig://${name}_motor_egu)> in signal registry.Use typhos.plugins.register_signal() [2025-01-30 16:17:45] - ERROR - Unable to find signal for <PyDMChannel (sig://${name}_description)> in signal registry.Use typhos.plugins.register_signal()

:sadge:

slactjohnson

This is pretty cool. Some of this Typhos stuff is a bit beyond me, but reading through this, things generally make sense. I'd be interested to understand if this is a good template to base other, similar screens off of. I imagine there's a lot of nice things that we could do with many of our devices if we put in a little time.

slactjohnson · 2025-01-30T18:08:37Z

+class _SmarActDetailedUI(QtWidgets.QWidget):
+    """Annotations helper for SmarAct.detailed.ui. Do not instantiate."""
+    # Status bar
+    calibrated_bool: pydm.widgets.byte.PyDMByteIndicator


Generally we don't add the type to the signal name; is there a reason you're doing that here?

If you make a stub class with annotations and apply it like this, you get really good autocomplete/api lookup help in e.g. vscode because it knows that self.ui.calibrated_bool is a PyDMByteIndicator

slactjohnson · 2025-01-30T18:13:35Z

+
+    def add_device(self, device):
+        """Typhos hook for adding a new device."""
+        super().add_device(device)


Should this occur after the check for None?

I think that will always kill the screen, because I have to wait for typhos to grab add_device and link the ophyd.device back to the display. Let me try it out at the very least!

I checked a bit and I think the ordering doesn't matter (based on the code for TyphosBase)

slactjohnson · 2025-01-30T18:43:02Z

+
+    def add_device(self, device):
+        """Typhos hook for adding a new device."""
+        super().add_device(device)


Same comment as above.

ZLLentz · 2025-01-30T18:59:03Z

To my quick-ish readthrough of the python file everything looks kosher, I can go again later with a more critical eye if you'd like to make sure there aren't any potential smaller issues. The results look good! I can try to nitpick ui layout stuff too (resize behavior, whitespace) if that's desirable.

aberges-SLAC · 2025-01-30T21:33:00Z

I'd be interested to understand if this is a good template to base other, similar screens off of. I imagine there's a lot of nice things that we could do with many of our devices if we put in a little time.

I think some of the maybe_add_signal variation and metadata shenanigans I did are scalable, if a bit meticulous. I really ought to play with typhos.variety module and see if I can generalize this approach and make it blend. That way we can do a lot of this magic through our pcdsdevices classes and leverage typhos more.

As long as the method hooks the typhos magic and you get the self.device access, you can do most of the pyqt stuff we'd be doing in custom IOC screens. PyDM has honestly improved a lot so I imagine it fits most of our wants now.

…n checks for TipTilt

christina-pino

So far looks good! Albeit that it has been a while since I have had to work with typhos/pydm and never got this far. Two questions/comments.

when released, will the *.ui and *.py "screens" join to be just *.ui like the rest?

what command did you use to open the detailed screen with the pico scale check? I tried running

typhos "pcdsdevices.epics_motor.SmarAct[{'prefix':'LAS:FTL:MCS:01:m1', 'name':'LAS:FTL:MCS:01:m1'}]"
but it never shows the pico scale tab and has the EGU/description error

aberges-SLAC · 2025-02-04T19:46:15Z

when released, will the *.ui and *.py "screens" join to be just *.ui like the rest?

Both the .ui and .py will show up as optional screens due to how typhos backend works, but will prioritize .py scripts over .ui. Due to the dynamic nature of picoscale signals, it necessitates having the .py file to handle the optional PVs and macro expansion

what command did you use to open the detailed screen with the pico scale check? I tried running
typhos "pcdsdevices.epics_motor.SmarAct[{'prefix':'LAS:FTL:MCS:01:m1', 'name':'LAS:FTL:MCS:01:m1'}]"
but it never shows the pico scale tab and has the EGU/description error

That's expected! A pcdsdevices.epics_mot.SmarAct device has no such pico_exists device signal, so the .py script will never populate the tab. You must use pcdsdevices.epics_motor.SmarActPicoscale to get that to work.

The egu and description signal error seems to be tied to how the .py script populates the UI display without expanding the macros. I can't get it to change that behavior, sadly, since pydm.Display doesn't appear to be expanding macros in UI file since it is opened before typhos adds the device, so it can't extend the macro. 🤷

aberges-SLAC · 2025-02-06T02:31:09Z

I might consider refactoring these screens given the ease of pcdshub/typhos#627.

I think I'd still need to do something janky to get the ordering the way I want for the tabs, but it does cut down on a lot of hard coding since I can just add things like:

def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        # Long name shenanigans
        # Motor Record long name overrides
        self.velocity.long_name = 'Velocity'
        self.velocity_base.long_name = 'Velocity Min'
        self.velocity_max.long_name = 'Velocity Max'
        self.acceleration.long_name = 'Acceleration'
        self.motor_stop.long_name = 'Stop Motor'
        self.motor_is_moving.long_name = 'Actively Moving'
        self.dial_position.long_name = 'Dial Position'
        self.direction_of_travel.long_name = 'Direction of Travel'
        self.home_forward.long_name = 'Home Forward'
        self.home_reverse.long_name = 'Home Backward'

To the pcdsdevices.epics_motor.SmarAct class and get a lot of pretty formatting for free from typhos.

ZLLentz · 2025-02-06T18:21:36Z

Both the .ui and .py will show up as optional screens due to how typhos backend works

I wonder if this is worth a typhos patch itself, or if there are cases where one might want to avoid the py file.

… as a byte indicator

…evice

… epics_motor device

aberges-SLAC · 2025-02-12T02:55:13Z

Okay, refactored my pyqt approach based on the changes in typhos for long_name. Also added tooltip support to be consistent with how TyphosSignalPanel would be handling things.

ZLLentz · 2025-02-14T01:25:27Z

These screenshots look really great!
I'm going to review once more from a coding perspective

tangkong · 2025-02-14T01:39:19Z

This does look amazing. I will mention that this seems to have a merge conflict, hopefully that's not too painful

ZLLentz

Mostly nitpicks! Can ignore if you like!
I think this is pretty great.

ZLLentz · 2025-02-14T01:31:57Z

+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        # Long name shenanigans
+        self.step_voltage.long_name = 'Step Voltage'


side note: the long_name ophyd PR churned a little bit recently (rebase?) so these are pretty safe for the future.

ZLLentz · 2025-02-14T01:33:35Z

+    def __init__(self, parent=None, ui_filename=ui_template, **kwargs):
+        super().__init__(parent=parent, ui_filename=ui_filename)


nitpick: this might be redundant now, unless you intend to block the kwargs pass-through

Suggested change

def __init__(self, parent=None, ui_filename=ui_template, **kwargs):

super().__init__(parent=parent, ui_filename=ui_filename)

It breaks if I change it 🤷

I think if I just manually call out the template anyway it's superfluous, but I deleted that and went the super().__init__ route. I remember it only being able to catch my template if I did it this way (still doesn't expand my macros) but let me tinker

Yeah I don't really gain or lose anything by blocking the **kwargs or not, least as far as I can tell. Still need to super().__init__ though. Maintainer's choice on whether or not I should pass the **kwargs 🤷

ZLLentz · 2025-02-14T01:34:30Z

+    """
+    ui: _SmarActDetailedUI
+    # Seems confusing, but really just cleans up the call to pydm for setting self.ui
+    ui_template = path.join(path.dirname(path.realpath(__file__)), 'SmarAct.detailed.ui')


Question: In the past I was able to just give e.g. SmarAct.detailed.ui without any path handling and pydm knew to check the same dir. Does it behave differently here? (same for the embedded widget)

You're absolutely right! This is probably a relic of previous fenagling I was doing. I can update this across the .py files

ZLLentz · 2025-02-14T01:37:01Z

+
+    def add_device(self, device):
+        """Typhos hook for adding a new device."""
+        super().add_device(device)


I checked a bit and I think the ordering doesn't matter (based on the code for TyphosBase)

ZLLentz · 2025-02-14T01:38:47Z

+    def fix_pvs(self):
+        """
+        Fix all the channel access and signal linking to various pydm objects in the screen,
+        since the macros aren't expanded when the UI is initialized.


nit: it'd be nice to figure out and clean the root cause here but it's not strictly required

Yeah the lack of macro expansion is very annoying considering the tedium of adding it in designer in the first place, but c'est la vie. I can't figure out why pydm isn't expanding the ui before opening the .py screen.

A little hacky for me to just handle the egu and desc signals this way, but it got rid of the terminal spam on start-up complaining about how the sig//${name}_egu etc. didn't exist

ZLLentz · 2025-02-14T01:40:11Z

+        self.maybe_add_pico()
+        self.add_tool_tips()
+        # Only start this timer if PicoScale exists
+        if hasattr(self.device, 'pico_exists'):


nitpick: possibly this belongs in maybe_add_pico

Oh! Good point, I'll move this there, not sense in having it out here if the check is already done in maybe_add_pico

Co-authored-by: Zachary Lentz <ZLLentz@users.noreply.github.com>

ZLLentz

Still happy, still on-board

aberges-SLAC and others added 8 commits January 24, 2025 14:27

ENH: Add a SmarAct detailed screen to customize to SmarAct signals

43147ca

ENH: Add a SmarActTipTilt embedded screen for simple open loop control

6fbb854

ENH: add hybrid pyqt script for SmarActTipTilt embedded screen to mim…

a27dff4

…ick behavior from TyphosPositionerRow

MNT: Try to fix expanded panel placement and refactor the resize func…

d93860a

…tion

ENH: Fix object names and formatting for controls tabs

86f5aac

ENH: Add hybrid pyqt script to handle SmarAct closed loop UI and incl…

315cd1f

…ude optional picoscale

Merge branch 'pcdshub:master' into SmarAct_ui_overhaul

a70c0a9

DOC: add release notes

f327052

aberges-SLAC requested review from ZLLentz, christina-pino and slactjohnson January 30, 2025 03:33

ZLLentz reviewed Jan 30, 2025

View reviewed changes

slactjohnson reviewed Jan 30, 2025

View reviewed changes

MNT: fix some ui loading calls for both screens and resizing conditio…

38ed48d

…n checks for TipTilt

aberges-SLAC self-assigned this Feb 4, 2025

christina-pino reviewed Feb 4, 2025

View reviewed changes

aberges-SLAC mentioned this pull request Feb 6, 2025

Add long names pcdshub/typhos#627

Merged

8 tasks

aberges-SLAC added 7 commits February 11, 2025 16:16

ENH: Add long_names for SmarAct

a358576

ENH: Remove ${name} macros from channels, will be handled in .py script

0df115a

ENH: Use long_names to simplify and refactor. Add another pico signal…

2351005

… as a byte indicator

MNT: Rename LSCO and LSCI widgets to be consistent with epics_motor.d…

f8d0b19

…evice

MNT: Fix tab priority

d94b4cf

MNT: make channel and module temp widget object names consistent with…

fc706f9

… epics_motor device

MNT: More naming convention fixes

fcd710d

aberges-SLAC added 3 commits February 11, 2025 18:28

MNT: EVEN MORE naming convention compliance

8e032e7

MNT: Last naming convention fix I swear

24b1d70

ENH: Add tooltip helpers and fix some object names

323d4e3

aberges-SLAC requested review from ZLLentz, christina-pino and slactjohnson February 12, 2025 02:54

DOC: update release notes doc

f30adb7

ZLLentz previously approved these changes Feb 14, 2025

View reviewed changes

Update docs/source/upcoming_release_notes/1275-SmarAct_UI_Overhaul.rst

cdd6188

Co-authored-by: Zachary Lentz <ZLLentz@users.noreply.github.com>

aberges-SLAC dismissed ZLLentz’s stale review via cdd6188 February 14, 2025 18:28

aberges-SLAC and others added 3 commits February 14, 2025 10:58

MNT: Clean up ui_filename call and move pico timer to rightful place

77e7d92

MNT: Clean up ui_filename call

14baba1

Merge branch 'pcdshub:master' into SmarAct_ui_overhaul

f799099

ZLLentz approved these changes Feb 18, 2025

View reviewed changes

aberges-SLAC merged commit 77112c3 into pcdshub:master Feb 19, 2025

aberges-SLAC mentioned this pull request Mar 4, 2025

Add SmarAct TTZV and other new PVs #1275

Closed

		def __init__(self, parent=None, ui_filename=ui_template, **kwargs):
		super().__init__(parent=parent, ui_filename=ui_filename)

Conversation

aberges-SLAC commented Jan 30, 2025

Description

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Screenshots (if appropriate):

Pre-merge checklist

Uh oh!

aberges-SLAC commented Jan 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slactjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZLLentz commented Jan 30, 2025

Uh oh!

aberges-SLAC commented Jan 30, 2025

Uh oh!

christina-pino left a comment

Choose a reason for hiding this comment

Uh oh!

aberges-SLAC commented Feb 4, 2025

Uh oh!

aberges-SLAC commented Feb 6, 2025

Uh oh!

ZLLentz commented Feb 6, 2025

Uh oh!

aberges-SLAC commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZLLentz commented Feb 14, 2025

Uh oh!

tangkong commented Feb 14, 2025

Uh oh!

ZLLentz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aberges-SLAC commented Feb 12, 2025 •

edited

Loading