SmarAct Encoded Tip Tilt Embedded Screens #1326
Conversation
…evices into SmarActEncodedTipTilt
There was a problem hiding this comment.
All the duplication is annoying, as you point out. I see a couple of ways to approach this, none of which need be implemented in this PR:
- Do some funky python-y import stuff by calling out the file name specifically
- Move all of these into some shared widget library, maybe
typhositself or possibly revivingpcdswidgets.- You'd then import those widgets from these files, just assuming that pcdswidgets is installed. (we don't list it as a dependency, similar to your use of typhos/pydm here)
All that aside, I think this looks pretty reasonable at a first readthrough. I'll take it for a spin and see how it looks
| # Calibrate first | ||
| self._status = axis.do_calib.put(1) | ||
| # Have to manually sleep :[ | ||
| sleep(5) |
There was a problem hiding this comment.
Can we get an explanation of why you need to sleep? It'd be nice if we could instead wait on some signal to let us know that calibration is done, because as is this is a brittle, hard-coded 5s sleep
There was a problem hiding this comment.
Because calib causes the motor to move, but it does not necessarily update device.motor_is_moving accurately when it is doing the calibration sequence. I tried a few iterations of this, but barring:
while device.motor_is_moving.get():
sleep(1)
I don't really have a way around it. From inherited ophyd signals the device.home has a wait incorporated which lets me do this safely during homing but not calib.
I do agree, the hard coded 5s sleep is my least favorite solution
There was a problem hiding this comment.
You know, what I could do (and is probably smarter) is just add some calibrated signal to the SmarAct closed loop device class that reads the :STATE_RBV pv and checks the bit for "is calibrated" like I do manually on the LEDs.
Caveat is that it breaks on older common IOC versions because it was originally an mbbi record and not mbbidirect.
There was a problem hiding this comment.
Would you time out on a calibration failure?
There was a problem hiding this comment.
I guess I could build one into a func for it?
But this method is actually flawed too because if you attempt to re-calibrate a stage you'll get a false-positive.
Also, example of the motor_is_moving signal not doing its job:
Screen.Recording.2025-03-06.112045.mp4
There was a problem hiding this comment.
Ah, but if I hardcode an initial sleep(1) into wait_on_calib to wait for motor_is_moving to change, then it works.
However the first motor_is_moving for an axis gets missed, apparently. So if it is the first time calling wait_on_calib for that axis, it'll just see motor_is_moving == 0 which is really annoying.
Regardless, I wasn't too far off with my hardcoded estimate. Looks like it takes on average 3.8-4 seconds.
| def set_open_loop(self, axis: str): | ||
| """ | ||
| A wrapper to set the open-loop widget channels. | ||
| Ironically more lines than just hard coding it. |
There was a problem hiding this comment.
But easier to extend! Maybe :)
| float(self.ui.tip_tweak_value.text())) | ||
| except Exception: | ||
| logger.exception('Positive tweak failed on tip') | ||
| return |
There was a problem hiding this comment.
You technically don't need these, if you have no returns by the end of the function, it will return None for you
There was a problem hiding this comment.
Ah, very true. I think I had this in there for what seems like superstitious reasons at this point. I'll clear it
|
Oh, lol, my commits from my other PR snuck in. We should delete those from this PR |
This reverts commit 40c4caf.
aberges-SLAC
changed the title
There was a problem hiding this comment.
Took another look after taking it for a spin, I'm still on board with this once some of the nits are resolved.
Looking back I think they're not enough to really hold this back, so I'll leave my check before I leave.
If you end up vendoring any of the positioner row logic again, we should look into a way to make those utilities more reusable
| sequence_progress_bar: QtWidgets.QProgressBar | ||
| tip_expert_button: TyphosRelatedSuiteButton | ||
| tilt_expert_button: TyphosRelatedSuiteButton | ||
| devices: list |
There was a problem hiding this comment.
a list of ophyd devices? We could probably be more specific here.
On a second look I'm not sure if this ever gets used here. Is this necessary?
There was a problem hiding this comment.
Ah, this is probably left in from a previous iteration of the class. I can probably delete it because that context help is meaningless now.
|
|
||
| Device Features | ||
| --------------- | ||
| - Add the SmarActEncodedTipTilt.embedded screens |
There was a problem hiding this comment.
Nit: should preface with the device(s) that was modified.
There was a problem hiding this comment.
What do you mean?
No devices were technically modified, just added .ui and .py files. Typhos backend will just prioritized these screens if the screen name == device class (i.e. pcdsdevices.epics_motor.SmarActEncodedTipTilt).
There was a problem hiding this comment.
Yeah, it's a really nitty nit. Generally we just put the class(es) related to the PR in front of the description, e.g.
SmarActEncodedTipTilt: Added embedded screens
There was a problem hiding this comment.
OH, no that's a valid nit and I see what you mean. Better readability, will commit
| self._status = axis.do_calib.put(1) | ||
| # Have to manually sleep :[ | ||
| sleep(5) | ||
| progress += 1 |
There was a problem hiding this comment.
The march of progress...
There was a problem hiding this comment.
Right! Much less elegant than what you were doing in BTMS-gui, but it works and I don't have to have separate threads
| if (self.tip_panel.minimumWidth() <= self.tip_panel.original_panel_min_width or | ||
| self.tilt_panel.minimumWidth() <= self.tilt_panel.original_panel_min_width): | ||
| # No change | ||
| self.resize_timer.start() |
There was a problem hiding this comment.
I don't fully understand this resize_timer bit. What's this doing? Just preventing the panel from trying to resize before you're done pushing things around?
There was a problem hiding this comment.
This is something TyphosPositionerRow figured out with the pop-up panel.
Basically, when you first launch the screen and proc detailed screen with the button, not all the signals are guaranteed to be connected and complete. The window will open with a much smaller size over the scroll area (you can see this at the 0:26-0:27 mark of the video). This whole method is forcing the pop-up screen to respect the inner widget's dimensions and resize itself.
You'll see this same block of code in SmarActTipTilt.embedded.py as well
Description
Adds:
New features beyond typical motor motion:
Couple caveats:
Don't love that I'm reusing classes and code from
SmarActTipTiltbut the alternatives for importing it as a module are hairy at best.Motivation and Context
As per issue #1325, we need an embedded screen that successfully bundles the open & closed loop motions for these stages, and I have done just that!
How Has This Been Tested?
I've tested this screen and code out on an STT50.8iv stage in the LRL. See video of the demo below!
If you want to take it for a spin, you can use this script:
/cds/home/a/aberges/bin/test_tip_tiltWhere Has This Been Documented?
Documented in the code, release notes, and this PR!
Screenshots (if appropriate):
smaract_encoded_tip_tilt_demo.mp4
Pre-merge checklist