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

evrStatus Script addition to engineering_tools #67

Merged
merged 2 commits into from
Apr 6, 2021

Conversation

Mbosum
Copy link
Contributor

@Mbosum Mbosum commented Mar 31, 2021

Description

Add an evrStatus script that allows the user to see an over view of the EVR statuses (Link, fiducial, etc) for a user defined hutch. An evrList file is then search for hutch key words and relevant EVR PVs are returned and an edm screen is displayed. For future EVRs in newer hutches (RIX, TXI etc) new PVs can be added to evrList in a new release or maybe dev version.

Motivation and Context

EVRs have been flaky in several hutches and so a need has arisen to quickly look at the status of them as well as compare easily across hutches. Many EVRs were not displayed in home screens before either.

How Has This Been Tested?

I ran this script from my home directory while logged into the hutch control machines as well as the daq and opr consoles.

The script has also been tested with shellcheck.

Where Has This Been Documented?

Documentation will be provided on confluence under EVRs.

@Mbosum Mbosum requested a review from ZryletTC March 31, 2021 20:09
@ZryletTC ZryletTC removed their assignment Mar 31, 2021
@ZLLentz
Copy link
Member

ZLLentz commented Apr 1, 2021

One thing to look at quickly for shell scripts is the shellcheck utility. It's currently available in pcds_conda. Here's the current output of shellcheck evrStatus:

$ shellcheck evrStatus

In evrStatus line 20:
evrs=(
     ^-- SC2054: Use spaces, not commas, to separate array elements.


In evrStatus line 299:
for i in ${command_args_string[@]}; do
         ^-----------------------^ SC2068: Double quote array expansions to avoid re-splitting elements.


In evrStatus line 307:
    for keyword in ${keywords[@]}; do
                   ^------------^ SC2068: Double quote array expansions to avoid re-splitting elements.


In evrStatus line 309:
        keyword_in_evrs=$(echo "${evrs[@]}" | grep -o $keyword | wc -w) # 1 or 0 depending if user input returns a match or not
                                                      ^------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
        keyword_in_evrs=$(echo "${evrs[@]}" | grep -o "$keyword" | wc -w) # 1 or 0 depending if user input returns a match or not

For more information:
  https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
  https://www.shellcheck.net/wiki/SC2054 -- Use spaces, not commas, to separa...
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...

@ZLLentz
Copy link
Member

ZLLentz commented Apr 1, 2021

Also, in general I advise avoiding writing bash scrips longer than 100 lines or so, it becomes really hard to debug and maintain. It is best to either split it into smaller scripts if possible or do it in Python.

@Mbosum
Copy link
Contributor Author

Mbosum commented Apr 1, 2021

Thanks for reviewing Zach. Yeah I did initially use shellcheck but I went over one more time and corrected all suggestions except for one:

`(pcds-4.0.1) cxi-control:scripts bosum123$ shellcheck evrStatus 

In evrStatus line 22:
evrs=(
     ^-- SC2054: Use spaces, not commas, to separate array elements.

For more information:
  https://www.shellcheck.net/wiki/SC2054 -- Use spaces, not commas, to separa...`

Using commas as a delimiter between PVs is the way I worked out to pass them effectively as EDM macros. I don't think this is a major design flaw, maybe just a cosmetic syntax thing. Other than that I went through and incorporated the rest of the suggested changes.

@Mbosum
Copy link
Contributor Author

Mbosum commented Apr 1, 2021

In the future, this script would be significantly cut down in size if the EVR PVs weren't preloaded, aka hard coded in an array. I'm thinking for a future upgrade there could be some way of querying for EVRs based on user defined conditions, via searching all PVs in our network.

@ZLLentz
Copy link
Member

ZLLentz commented Apr 1, 2021

The line count is a rough proxy for script complexity, not the primary metric for judging if something is too complicated to be expressed as a bash script. A static array of PVs is not adding to the complexity here.

The next go at this should make it a Python script, because it's the simplest scripting language with strong support for data structures. Without data structures (even just simple things like dicts), making scripts like this becomes more difficult than it needs to be, and then becomes a maintenance hazard when something changes and it needs to be modified.

As a Python script we could even start pulling in useful bits into our modules and re-use them elsewhere in a more fluid way.

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.

There is still some cleanup that can be done on this but if it will be converted to python soon anyway, I've tested that the script works and I think this is fine to merge for now.

@Mbosum Mbosum merged commit 6587249 into pcdshub:master Apr 6, 2021
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.

None yet

3 participants