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

Changes to camViewer #94

Merged
merged 32 commits into from
Mar 19, 2022
Merged

Changes to camViewer #94

merged 32 commits into from
Mar 19, 2022

Conversation

spenc333
Copy link
Contributor

@spenc333 spenc333 commented Jan 13, 2022

Description

Changes to camviewer script involved making fully worded enable and disable options as well fixing the -H option that now allows for a camera to be accessed from any hutch. Adding the imgr script to eng_tools instead of its original directory allows for control of an ioc given it's name and hutch . Also needed this script to be called in camviewer

Motivation and Context

Fixes #76

How Has This Been Tested?

Tests involved sourcing the script from my epics-dev directory using various camera names and hutches as well as the other options. Since imgr is in development and needed this command within camviewer, it also had to be sourced from epics-dev directory within the camviewer script.

Where Has This Been Documented?

Both scripts have updated usage functions

@spenc333 spenc333 requested a review from ZLLentz January 13, 2022 07:08
@ZryletTC ZryletTC self-requested a review January 13, 2022 10:09
@spenc333 spenc333 marked this pull request as draft January 13, 2022 11:08
@spenc333 spenc333 marked this pull request as ready for review January 13, 2022 11:40
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a bunch of comments/questions/suggestions

scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/imgr Outdated Show resolved Hide resolved
scripts/imgr Outdated Show resolved Hide resolved
scripts/imgr Outdated Show resolved Hide resolved
scripts/imgr Outdated Show resolved Hide resolved
scripts/imgr Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/imgr Outdated Show resolved Hide resolved
scripts/imgr Outdated Show resolved Hide resolved
scripts/imgr Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
@ZLLentz
Copy link
Member

ZLLentz commented Jan 20, 2022

One more suggestion for any time you're doing a bash script: make sure you run the result through shellcheck, it can often help catch issues.
There's a shellcheck utility in pcds_conda and also a website https://www.shellcheck.net/

@silkenelson
Copy link
Collaborator

It feels wrong to me to add the imgr script to engineering tools as long as it also lives in the iocmanager package - with exactly the same name.

@spenc333
Copy link
Contributor Author

It feels wrong to me to add the imgr script to engineering tools as long as it also lives in the iocmanager package - with exactly the same name.

I agree.

@ZryletTC
Copy link
Contributor

@silkenelson This is pretty much how iocmanager is done as well, do you have a better alternative? At the moment, there is no default way to use imgr than by specifying the full path to the executable. Adding it to engineering_tools adds it to everyone's PATH without extending the PATH variable.
I brought up the question of how to handle this with you and Mike over Slack back in June. He agreed that a wrapper script in engineering_tools was probably best but I never heard your opinion. If there's a better way, I'm all ears.

@silkenelson
Copy link
Collaborator

If I was part of that discussion, I have forgotten - I am unable to retain slack discussions for much longer than a week. Yes indeed: the answer that removing this script from iocmanager was part of this request (or at least where this was going) answers my question.

@ZryletTC
Copy link
Contributor

Once #101 is merged, the imgr addition should be removed from this script, then I'll rereview this PR and we can hopefully merge.

@ZryletTC ZryletTC changed the title Changes to camViewer and adding imgr script Changes to camViewer Feb 16, 2022
@spenc333 spenc333 requested a review from ZryletTC February 25, 2022 20:37
@ZLLentz
Copy link
Member

ZLLentz commented Mar 1, 2022

I'm excited to use this one, I semi-frequently find myself wishing I had the hutch selector here

Copy link
Contributor

@ZryletTC ZryletTC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the code, the style fixes have broken some functionality.

scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
Copy link
Contributor

@ZryletTC ZryletTC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, looks good, all my tests have worked :) (after adding the commas)

scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
scripts/camViewer Outdated Show resolved Hide resolved
@spenc333 spenc333 merged commit 62c2884 into pcdshub:master Mar 19, 2022
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.

ENH: Future improvements for camViewer
4 participants