-
Notifications
You must be signed in to change notification settings - Fork 62
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
LasBasler User Presets #1328
base: master
Are you sure you want to change the base?
LasBasler User Presets #1328
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me, but I have some questions/comments.
|
||
Device Features | ||
--------------- | ||
- Adds long_names to signals in LasBasler detector class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: these should be prefaced with the name of the device(s) that was modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I did? LasBasler
is the device class that is edited, the other BaslerNF
and BaslerFF
classes subclass it.
@@ -502,6 +502,64 @@ def _auto_configure(self): | |||
def _auto_configure(self, value): | |||
self.configure(self._conf_d) | |||
|
|||
# Handle UserPresets configuration | |||
default_preset = Cpt(EpicsSignal, 'UserSetDefaultSe_RBV', write_pv='UserSetDefaultSe', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did they really leave off the 't' in 'Set', or is this a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property name is actually UserSetDefaultSelector
but the common ioc truncates it 🤷
kind='config', | ||
doc='Default Preset to use on startup. See UserSetSelector' | ||
' for more options') | ||
user_preset = Cpt(EpicsSignal, 'UserSetSelector_RBV', write_pv='UserSetSelector', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumb question: if these are called "User Sets", why are we calling them presets here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly because I hear folks call them "presets" vs. "user sets". I think the official vendor documentation calls them "User Sets".
I felt that something like camera.user_set
would be an unclear signal name and imply being a setter
and not an actual config option.
|
||
def __init__(self, *args, **kwargs): | ||
super().__init__(*args, **kwargs) | ||
# Add some long_names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all of these long name updates just a ride-along update for readability, or are they related to the user set stuff somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both. Ride-along for better general readability and also for clarity on the newly added signals.
…devices into LasBasler_user_presets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mile-high nitpick: There are already "presets" in pcdsdevices (https://pcdshub.github.io/pcdsdevices/v8.7.0/presets.html), so this is a sort of feature naming collision.
These two features exist in two different sub-types of device, but it did disorient me briefly (I thought you implemented our pcdsdevices-style-presets for the Baslers)
Throwing it out there: Is "settings" an inappropriate name here? (user_settings
, save_settings
, load_settings
)?
Personally I like "settings" better |
Agreed and done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you merge my comment through the UI it won't dismiss my review 🤔
docs/source/upcoming_release_notes/1327-LasBasler_User_Presets.rst
Outdated
Show resolved
Hide resolved
….rst Co-authored-by: Robert Tang-Kong <[email protected]>
Nope 😿 |
Now I'm slightly more confident that the merge commit won't dismiss the review... |
Description
Added UserPresetSelector PVs for the default selector and current selector. This is a feature exposed in the gigecam IOC for Basler cameras that allows the user to save hardware settings to a preset for a given stream. There are also several factory settings available as well (high gain, color raw, etc.).
Changing
UserPresetDefaultSe
will load that preset uponARAVIS_RESET
commands.Motivation and Context
Try to fix day-to-day operations pains for Laser folks as it will make restoring giges after planned/unplanned power cycling of cams much easier.
Also closes #1327
How Has This Been Tested?
Tested on pretty much every chemRIXS Basler in their MODS, and a few in TMO IP1.
Where Has This Been Documented?
In this PR and the code! See also https://jira.slac.stanford.edu/browse/ECS-4805 for quirks about how camera firmware messes with the behavior of these pvs due to hashtable nonsense.
Screenshots (if appropriate):
Demo below
basler_ff_user_preset_example.mp4
Pre-merge checklist