pcdshub / lightpath

LCLS Lightpath Module
https://pcdshub.github.io/lightpath
Other
4 stars 9 forks source link

GUI: More expressive widget icon colors, and more along the way #150

Closed tangkong closed 1 year ago

tangkong commented 1 year ago

Description

We're finally getting into the issues from the jira tickets! Kinda.

Also in a more minor sense

Motivation and Context

closes #144 , and a few jira tickets as detailed above

Thoughts and justifications of my decisions, to make myself feel better - Accumulated transmission logic: Inserted devices can now have non-0 transmission, but should removed devices have the ability to block? - Currently the logic already assumes that removed devices cannot be blocking, unless there is branch mismatch - While it's not absurd to imagine a device that blocks when removed, I chose to punt the issue - Now that the `LightRow` widget relies on the path and the device, are we going to have a race condition? - Two `LightRow` widgets and one `BeamPath` are subscribed to changes in `device.lightpath_summary`. - When `lightpath_summary` changes, is it possible that the `LightRow`'s update before the `BeamPath` finishes, giving us bad state information? - No, callbacks are processed in insertion-order, and the `BeamPath` callbacks are subscribed before the `LightRow` widgets - I should really draw out the subscription structure in a nice way so people don't have to go through the maze repeatedly

How Has This Been Tested?

Interactive testing, and tests have been added to the suite.

Where Has This Been Documented?

This PR, some docstrings, etc

Some example screenshots

From sim mode, sim_ipimb's have an inserted transmission of 0.5, so inserting 4 drops the beam below the minimum of 0.1. Inserted devices are now blue, and device types have been updated

image

An example from XCS

image
tangkong commented 1 year ago

This is probably ready for some eyes. The exact names and colors I used will surely be changed in the future, but hopefully this PR provides us an opportunity to customize such things.

I had thoughts of adding the option to switch between various color maps, but that's maybe for a different pr.

ZLLentz commented 1 year ago

I'll give this a look! Any special instructions for running it myself? Do I need to set up a pcdsdevices dev too?

tangkong commented 1 year ago

I'll give this a look! Any special instructions for running it myself? Do I need to set up a pcdsdevices dev too?

You'll need updates from 3 branches:

Once those three are in your path, you should be able to redirect HAPPI_CFG to the new device_config db and load lightpath. Loading lightpath --sim should work independent of that

ZLLentz commented 1 year ago

It looks to me like I also need to include lightpath as a pip -e install to make the container work right and I need to include the master branch of happi too

ZLLentz commented 1 year ago

I get strange behavior when I run this: lots of error spam and then included device types is surprisingly bare. I suspect maybe I haven't set up my environment correctly but I can't tell. The new colors show up and look good. image

tangkong commented 1 year ago

Yeah, I'm working out the fix to the error spam now. It's been bothering me for quite a while, and I think I made a breakthrough (stay tuned). Edit: Error spam has been fixed in pcdsdevices, pcdshub/pcdsdevices@9e7c57b0

The device types list is bare because we're looking for specific pre-determined device types, and pcdsdevices has become quite... varied in their inheritance. (e.g., at2l0 doesn't inherit from AttBase). Something I wanted to do was populate that table with the existing devices, but couldn't immediately think of a clean way to decide what the "parent" of a device should be, or when to stop grouping devices together (a lot of devices might be reasonably grouped as "positioners", but not all of them?). We could also let the user configure this, but we'd have to build up the machinery for that. I remember thinking about this and eventually also punting it.

If all this looks good I can make an issue

ZLLentz commented 1 year ago

The device types list is bare because we're looking for specific pre-determined device types, and pcdsdevices has become quite... varied in their inheritance.

Maybe this should become its own issue if there is enough unrelated stuff in this PR. I'd expect that I'd be able to make all of the devices disappear by clicking all the filter buttons, but that doesn't seem to be possible (it wasn't possible in the old lightpath either, to be fair)

lightpath --sim doesn't load properly for me which makes me think I've made an error in my dev setup here...

ZLLentz commented 1 year ago

I don't think this closes #144 though. The buttons still don't work fully, there are still valves that remain when clicking the valves button for example.

tangkong commented 1 year ago

I don't think this closes #144 though. The buttons still don't work fully, there are still valves that remain when clicking the valves button for example.

Which valves remain? the SIM_Valve only refers to the mock device, and I think not all valves are GateValves?

ZLLentz commented 1 year ago

Pretty much all valves in the lightpath are gate valves

ZLLentz commented 1 year ago

From this screenshot:

tv1l0_vgc01 should be grouped with the gate valves (class VGC = Valve Gate Controlled) im1l0 should be grouped with the pims (class XPIM = XTES PIM) sl1l0 should be grouped with the slits (class PowerSlits)

image

I think we should sit down and try to put all the remaining beamline objects into one of the existing boxes

ZLLentz commented 1 year ago

Lingering combobox error spam can be sourced to https://github.com/pcdshub/pcdsdevices/pull/1046

tangkong commented 1 year ago

I think I've addressed the concerns here. I spent this morning poking around the gui trying to illicit a "double free" error, but to no avail. Instead I:

ZLLentz commented 1 year ago

Do you want a re-review and/or a re-beta tester?

tangkong commented 1 year ago

Do you want a re-review and/or a re-beta tester?

A re-review would be nice, the main thing that's changed is the device types filtering. Someone else poking around with it would also be appreciated, maybe the sporadic errors/crashes will show up for others...

Once I merge this I have a documentation rewrite in the pipeline, after which maybe we can tag? 👀

ZLLentz commented 1 year ago

Looks good interactively! I'll poke through the source now. There are a few devices that filter on an unintuitive button but I think that means they need happi/pcdsdevices updates rather than anything special here.

ZLLentz commented 1 year ago

I hit a segmentation fault but it wasn't the double free. I'll run again and turn core dumping on while I browse the code.

tangkong commented 1 year ago

If there are no further comments I'll merge this at end of day (8/25/22). The disconnected devices issue will be a bit involved, and deserves a separate PR.

ZLLentz commented 1 year ago

I agree, this is a strong improvement and fixes a lot!