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.

Sign up for GitHub

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

Jump to bottom

EBD/FEE Flow Switch Readbacks #120

Merged
merged 14 commits into from
Dec 8, 2023
Merged

EBD/FEE Flow Switch Readbacks #120

merged 14 commits into from
Dec 8, 2023

Conversation

nrwslac
Copy link
Contributor

@nrwslac nrwslac commented Oct 31, 2023 •
edited
Loading

Description

  • Implement fbFlowSwitch variables for J120K Flow Switches installed in the EBD and the FEE.
  • Implements the following component flow switches:
  • PC1K0
  • IM1K0
  • SL1K0
  • IM2K0
  • SL2K0
  • PF1K0
  • IM1K4
  • PC1K4
  • ST1K4
  • IM1K3

Motivation and Context

  • Newly installed J120K Flow Switches.

How Has This Been Tested?

  • Switches were turned on and off and high/lo signals observed on the corresponding ECat EL1004 terminals.
  • Software deployed, all variables checked for proper linking and high flow signals.
  • Double checked all links agree with the linked google spread sheet.

Where Has This Been Documented?

https://jira.slac.stanford.edu/browse/ECS-1514
https://jira.slac.stanford.edu/browse/ECS-1482
https://docs.google.com/spreadsheets/d/1UAVYuG5my1ic51wRrpq9xJfvT5X7NX-nF4QzPtAUaPg/edit#gid=1111868035

Screenshots (if appropriate):

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive comments
  • Test suite passes locally
  • Libraries are set to fixed versions and not Always Newest
  • Code committed with pre-commit (alternatively pre-commit run --all-files)

@nrwslac nrwslac requested a review from ZLLentz November 1, 2023 23:36
ZLLentz
ZLLentz approved these changes Nov 2, 2023
View reviewed changes
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.

Just one small IO naming question to pay attention to
I think these are implemented correctly.

As I'm writing the end of this review I've just noticed the google sheet- I'll go through the sheet too to double-check the linking.

...nction 3 (EK1122)/X1 TMO FEE EC1 (EK1100)/TMO FEE EC1 E1 (EK1122)/X2-ST1K4-TEST (EK1100).xti
@@ -19,7 +19,7 @@
<Box File="ST1K4-EL7041-E4.xti" Id="116">
<EtherCAT PortABoxInfo="#x01000073"/>
</Box>
<Box File="FEE-K-PCW-FSW-EL1004.xti" Id="240">
<Box File="ST1K4-EL1004-E5.xti" Id="240">
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that there's already an E5 in the IO tree here. Is this a typo, or was ST1K4-EL9505-E5 always named wrong (should it be E6?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think whats going on here is E5 at install exists turning the following terminals into 5V. Next someone adds an EL5042 calls it E5A before the 9505, next someone adds EL1004 Before E5A calls it correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's currently an 1004-E5, 5042-E5A, followed by 9505-E5

Copy link
Member

@ZLLentz ZLLentz Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense, that explains the confusion here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could quickly fix this.

Copy link
Member

Choose a reason for hiding this comment

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

I would ask you to, but this also needs to involve updating drawings, terminal labels, and maybe even captars, so it might not necessarily be worth your time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is incidentally a device that never had a drawing until I started hooking stuff up to it, so its probably not a big deal to update. Its actually been stuck in purgatory for awhile:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://trello.com/c/HvURG1e1/256-05ops-fee-sc-fsw-st1-im1k4-r-1-e4
not a bad idea to get it pulled from the darkness into a JIRA ticket

plc-kfe-motion/kfe_motion/POUs/IM2K0/FB_XPIM_IM2K0.TcPOU
@@ -85,7 +85,7 @@ VAR
fbLED: FB_XPIM_LED;

{attribute 'pytmc' := '
pv: SFW
pv: FSW
Copy link
Member

Choose a reason for hiding this comment

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

Good catch

ZLLentz
ZLLentz previously approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@ZLLentz ZLLentz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

OK, double-checked that the channels match up with the documentation 👍

Are you worried at all about the names of the switches not matching up with the PVs?

e.g. IM1L0:FSW:1 has PVs named IM1L0:XTES:FSW:FLOW_OK_RBV (I think?)

@nrwslac
Copy link
Contributor Author

nrwslac commented Nov 2, 2023

IM1L0:XTES:FSW:FLOW_OK_RBV

I think the current PV naming makes sense, I'll make the spread sheet align with them and not the other way around.

ZLLentz
ZLLentz previously approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@ZLLentz ZLLentz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM (pending pamm testing)

@nrwslac nrwslac marked this pull request as ready for review December 8, 2023 18:47
joshc-slac
joshc-slac approved these changes Dec 8, 2023
View reviewed changes
Copy link

@joshc-slac joshc-slac left a comment

Choose a reason for hiding this comment

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

LGTM: worked through it with Nick live

@nrwslac nrwslac merged commit eb1d955 into pcdshub:master Dec 8, 2023
9 checks passed
@tangkong tangkong mentioned this pull request Dec 15, 2023
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshc-slac joshc-slac joshc-slac approved these changes

@ZLLentz ZLLentz ZLLentz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nrwslac @ZLLentz @joshc-slac