Skip to content
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

Added configurable rate for open_viewer #1309

Closed
wants to merge 2 commits into from

Conversation

KaushikMalapati
Copy link
Contributor

@KaushikMalapati KaushikMalapati commented Nov 20, 2024

Description

Added a rate signal that appears as a typhos slider on the screen which is passed as an argument to run_viewer which opens camviewer in open_viewer. Default is five hertz, which is also the default used by run_viewer.

Motivation and Context

#1308

How Has This Been Tested?

Interactively
Left side is at one hertz, right is at twenty. You can see the left camviewer update about once a second and right update much faster, although maybe not quite at twenty because of lag.
https://github.com/user-attachments/assets/c3537c01-8e48-4b55-8583-9d82793fa1ff

Where Has This Been Documented?

upcoming-release notes

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

@KaushikMalapati
Copy link
Contributor Author

KaushikMalapati commented Nov 20, 2024

Gonna leave this as a draft for now because I am having a problem with the TyphosSlider where dragging it never changes the value by more/less than one. @tangkong, do you have any idea why this is happening?

Screenshare.-.2024-11-19.7_10_43.PM.mp4

@KaushikMalapati
Copy link
Contributor Author

@aberges-SLAC, I ended up trying this on las_lhn_bay1_cam_nf when I saw there was nothing scheduled for the laser hall. I mainly wanted to confirm that changing the rate signal actually changed framerate in camviewer.

the changes are in PCDSAreaDetectorTyphos which the basler classes inherit from. Feel free to try this for yourself and see what you think. Let me know what you think of the way you pick the rate or anything else you want changed/added.

@tangkong
Copy link
Contributor

Strange, I'm not sure. I haven't used the typhos slider much, and would have to take a deeper look. I'm not immediately sure if the other typhos screens that use this slider have a similar problem.

I tried changing the signal to a caproto-backed PV, on the off chance that there was some repetitive caput loop that kept snapping the slider. (no change was seen)

@ZLLentz
Copy link
Member

ZLLentz commented Nov 20, 2024

This might just be related to https://github.com/slaclab/pydm/releases/tag/v1.25.0 since typhos relies on the pydm internals quite a bit

@tangkong
Copy link
Contributor

tangkong commented Nov 20, 2024

I'd put money on this just needing a pydm update. How we had a slider for all these years that wasn't draggable? That's a whole different problem.

EDIT: I just updated pydm in my dev and the slider slides 💀

@KaushikMalapati
Copy link
Contributor Author

I tried this with dev_conda, which says it's using v1.25.1, but the slider still has the bug. When I use a pydm repo checked out to v1.25.1, the slider works. Am I missing something? pcds_conda uses v1.24.0, so it makes sense that the slider wouldn't work, but dev_conda looks like it's using the most recent tag for pydm which should have fixed the slider.

(pcds-5.9.1) [kaushikm@psbuild-rhel7-02 ~]$ dev_conda
python
i(pcds-5.9.1) [kaushikm@psbuild-rhel7-02 ~]$ python
mPython 3.9.19 | packaged by conda-forge | (main, Mar 20 2024, 12:50:21)
[GCC 12.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pydm
pydm.__ve>>> pydm.__version__
'v1.25.1'

image

@tangkong
Copy link
Contributor

How are you invoking typhos/opening the screen? perhaps that's stuck looking at pcds_conda? (It looks like you're setting yourself up correctly)

@KaushikMalapati
Copy link
Contributor Author

How are you invoking typhos/opening the screen? perhaps that's stuck looking at pcds_conda? (It looks like you're setting yourself up correctly)

I was doing export PYTHONPATH=/cds/home/k/kaushikm/pcdsdevices to look at my pcdsdevices branch. I usually use pcds_conda to test which doesn't use PYTHONPATH which is why I don't do export PYTHONPATH=/cds/home/k/kaushikm/pcdsdevices:$PYTHONPATH but dev_conda does use it
(/cds/group/pcds/pyps/apps/dev/pythonpath), so I was inadvertently removing the directory with the more recent pydm version and defaulting to the same version pcds_conda uses. Since the slider does work with dev_conda when I do not break it, I think this is ready for review. Thank you for your help @tangkong and @ZLLentz.

@KaushikMalapati KaushikMalapati marked this pull request as ready for review November 21, 2024 01:08
@KaushikMalapati
Copy link
Contributor Author

@aberges-SLAC, do you have an opinion on what range of rates should be available? I have it from 0 to 100 right now, but the lower limit should definitely be one since camviewer doesn't work with a rate of 0 (big surprise). Do you think twenty or 25 would be enough for the high limit, or should it be something else?

@tangkong
Copy link
Contributor

I know we went through all the slider trouble, but is this possibly better as a qspinbox with intelligent bounds? So the user can just type exactly what the rate should be?

@KaushikMalapati
Copy link
Contributor Author

Actually, I was thinking about this more - is there any reason why this shouldn't be an option in the camviewer gui? This would let people dynamically change the rate without having to restart camviewer and seems like a better solution that adding rate selection everywhere we call run_viewer.

If for some reason that's not a good idea, I agree that a spinbox would be better than the slider and I will make that change.

@aberges-SLAC
Copy link
Contributor

Just adding a +1 to having a rate that is just an int being typed by the user. I think the sliders will drive the users in Lasers mad.

We probably should use some smart bounds since acquisition rates vary wildly by camera. I don't remember if there is already a record in the gigecam ioc that describes that, but you can absolutely read that property from the genicam tool on a per-model basis (at least for aravis flavors)

@ZLLentz
Copy link
Member

ZLLentz commented Nov 21, 2024

Actually, I was thinking about this more - is there any reason why this shouldn't be an option in the camviewer gui?

I wrote the code for this a few hours ago, with the intention of making this part of the next camviewer release.
Sneak preview very WIP (the cli argument will choose the starting value for the setting):

image

@KaushikMalapati
Copy link
Contributor Author

If this is being added to camviewer, I think this pr is redundant. @aberges-SLAC, can you think of any reason why we would want to set the rate in typhos before opening in camviewer as opposed to dynamically selecting the rate in camviewer? If not, I will close this. I think a small benefit of the latter is to prevent people from repeatedly using a high framerate for lucid screens that are open for long periods of time.

@aberges-SLAC
Copy link
Contributor

If this is being added to camviewer, I think this pr is redundant. @aberges-SLAC, can you think of any reason why we would want to set the rate in typhos before opening in camviewer as opposed to dynamically selecting the rate in camviewer? If not, I will close this. I think a small benefit of the latter is to prevent people from repeatedly using a high framerate for lucid screens that are open for long periods of time.

I think that even if we opted for the latter, the users that would take advantage of this feature would have the cams open for very long times. I think keeping the acquisition rate inside the camviewer screen is fine. If the users really want it in the typhos screen afterwards, we can reopen the issue.

Thank you for the prompt response on all of this btw!

@ZLLentz
Copy link
Member

ZLLentz commented Dec 2, 2024

(if you want to speed up the cam viewer rate options PR, I'm looking for beta tester opinions in https://jira.slac.stanford.edu/browse/ECS-5976)

@KaushikMalapati KaushikMalapati deleted the typhosrate branch December 4, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants