Open ZLLentz opened 1 year ago
The background here is nicely written and comprehensive - thanks for the refresher.
As to the plan, I think it's good overall and agree with the path forward. As usual, I have my own opinions regarding potential implementation details. Let me see if I can convey what I'm thinking adequately...
I've mentioned before that I think the heavy reliance in PyDM on URIs like calc://a?b...
with really long strings was an unfortunate or even bad design decision which is hostile to users. I think these attributes should contain structured data in a standard format (i.e., JSON) which can be more verbose and clear as to their intent. PyDM should bundle designer dialogs that help in modifying these in a sensible way.
I think a similar approach here could be beneficial (and would like to know what you think); it's an alternative to adding perhaps 5 or more fields to widgets that need to be handled separately in pyqt properties on widgets. In short, add a "device_source" field of type JSON that has a custom designer dialog that lets you easily search happi, select a component or subdevice or ..., or switch to a load string, etc. This should be well-formed JSON that corresponds with a dataclass that can be easily generated from Python as well, in case we want to programmatically generate them.
You're right, including custom designer dialogs is a step up in UX from "well-named fields with tooltips" and lets us work with structured data instead of limiting us to "the user gave a string" as in my proposal and in the pydm-style long URI formulations. When I dive into this, I'll start by exploring what can be done in these custom dialogs.
I'm creating this issue to document and gather feedback before I jump headfirst into a somewhat consequential refactor.
Background: Typhos plugin structure
SignalPlugin
intyphos.plugins.core
uses thesig://
protocol and sends values and metadata from anyophyd
signal, such as the non-epics signals. It is expecting the address to include the signal's name. Signals must be registered with the signal plugin. When a device is loaded intotyphos
via callingadd_device
on a display, all of its signals are registered. Note that this excludes devices sent in through theHappiPlugin
.HappiPlugin
intyphos.plugins.happi
uses thehappi://
protocol and sends a loadedhappi
device and its accompanying metadata to thetx_slot
as defined intyphos.widgets.HappiChannel
. It is expecting the address to either include thehappi
device's name or thehappi
device's attribute path to access a subdevice (or, any attribute of a subdevice). It sends a dictionary with keys"obj"
and"md"
. Widgets need special handling to use theHappiPlugin
, including a customchannel
attribute that knows to use aHappiChannel
. This is implemented in two places: first, ontyphos.widgets.TyphosDesignerMixin
, which is used for alltyphos
widgets except for the alarm widgets. The second place is ontyphos.alarm.TyphosAlarm
, which is used for thetyphos
alarm widgets. The variant ontyphos.alarm.TyphosAlarm
also allows other kinds of channels to be passed to use a normal epics connection or a signal as the object.HappiPlugin
is not used except when the user invokeschannel=happi://
. Internally, when you launch atyphos
device screen, it will load fromhappi
without invoking the plugin, then calladd_device
.SignalPlugin
cannot be used outside of atyphos
template without some setup in code, making it somewhat confusing to apply. On the flip side, it behaves very similar to thepydm
built-in plugins so it's really nice to be able to re-usepydm
widgets but with anophyd
signal. Users can get confused when there are two paths to accomplish the same goal: EPICS control points could be handled with the EPICS plugins instead, and there is no clear cost or benefit to either methodology. On balance, I don't think much needs to change here.HappiPlugin
can be used anywhere, which makes it very convenient for embedding atyphos
widget inside of a normalpydm
display. UX-wise, however, it mimics thepydm
channel interfaces while having completely different behavior, making it confusing to use. I would usually expect thechannel
field to be a source of live control data, but in this case it's a database source. Widgets that expect theHappiPlugin
mostly ONLY accept theHappiPlugin
, so you are required to passhappi://
even though all other options will fail. Users in general need to read the docs to figure out how to fill the widget regardless of their level of familiarity withpydm
. Using it inside of atyphos
template is clunky: you could use the${name}
macro to help access a subdevice, but this feels like a lot of boilerplate for something that should be a built-in.HappiPlugin
widgets are impossible to fill with devices that aren't in happi via designer. I think this needs some adjustments.typhos
display loads,add_device
is called on alltyphos
widgets. This is why you can place aPositionerWidget
and leave the channel empty but still get a result when you open the screen.add_device
is called regardless of existing devices orhappi
channel settings. This would be fixed by #548, but I paused that PR short of test suite updates to make this issue. I realized that in that PR it was awkward to test because there was no way to add a subdevice from a ui file without putting it into happi first.Here is my plan:
SignalPlugin
SignalPlugin
will be extended slightly to ease the requirement of doing custom setup and make it easier to use.happi
and use the given signal attribute as the data source in the widget. This will not require the signal to be registered.HappiPlugin
HappiPlugin
will stay and thechannel
field will also stay, but it will be consolidated into one implementation using the paradigm set in the alarm widgets: any non-happi protocol will create a signal that will be updated from the data source, and the signal will be sent as a "device" to add to the widget, which will need to handle this appropriately.happi_load_name
will be added that will link directly to theHappiPlugin
without considering thechannel
field.subdevice_attr
will be added that will let us specify a subdevice to fill the widget with rather than the main device objectdevice_source
will be added, with options "automatic", "happi", "channel", and "explicit", which defaults to "automatic". This can be set manually or it will update automatically if exactly one of the related fields is filled. The typhos load process will calladd_device
if this is set to "automatic", will load from theHappiPlugin
usinghappi_load_name
if this is set tohappi
, will load from the channel address if this is set to "channel", and will create a new device if this is set to "explicit".explicit_load_string
will be added that you can fill with e.g.ophyd.EpicsMotor("SOME:PREFIX", name="some_name")
to include any device, even ones that aren't inhappi
.The order of these fields will be:
I'd like to nail down the details here before proceeding: good idea? Bad? Misguided?