Adding deltas to ATMs #72

jyotiphy · 2024-10-08T21:33:46Z

Description

Adding delta for TM1K2 and TM2K2 states setpoints.

Motivation and Context

More targets are added to the ATMs and scientists need more flexibility to move through states during commissioning experiments.

How Has This Been Tested?

Didn't test yet

Where Has This Been Documented?

Nope

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)

ZLLentz · 2024-10-08T22:24:22Z

plc-kfe-rix-motion/plc-kfe-rix-motion.tsproj

@@ -10,7 +10,7 @@
 					<TargetSelect TargetId="2">{66689887-CCBD-452C-AC9A-039D997C6E66}</TargetSelect>
 					<TargetSelect TargetId="2">{3EBB9639-5FF3-42B6-8847-35C70DC013C8}</TargetSelect>
 					<TargetSelect TargetId="2">{520DE751-9DB6-47CB-8240-BD5C466E7E64}</TargetSelect>
-					<LicenseDevice DongleHardwareId="2" DongleDevice="#x03020002" DongleLevel="50" DongleSystemId="{CBBB2953-6251-050D-F24B-D23248A8B21C}"/>
+					<LicenseDevice DongleHardwareId="2" DongleDevice="#x03020002" DongleLevel="90"/>


Did something strange happen to your license config?
The level changed the system id vanished

ZLLentz · 2024-10-08T22:29:34Z

plc-kfe-rix-motion/kfe_rix_motion/POUs/PRG_TM1K2_ATM.TcPOU

+fbStateSetup(stPositionState:=fbTM1K2.stOut, fPosition:=-16.00, fDelta:=10, sPmpsState:='TM1K2:ATM-OUT');
+fbStateSetup(stPositionState:=fbTM1K2.stTarget1, fPosition:=-39.124, fDelta:=10, sPmpsState:='TM1K2:ATM-TARGET1');
+fbStateSetup(stPositionState:=fbTM1K2.stTarget2, fPosition:=-53.5, fDelta:=10, sPmpsState:='TM1K2:ATM-TARGET2');
+fbStateSetup(stPositionState:=fbTM1K2.stTarget3, fPosition:=-67.874, fDelta:=10, sPmpsState:='TM1K2:ATM-TARGET3');
+fbStateSetup(stPositionState:=fbTM1K2.stTarget4, fPosition:=-82.25, fDelta:=10, sPmpsState:='TM1K2:ATM-TARGET4');
+fbStateSetup(stPositionState:=fbTM1K2.stTarget5, fPosition:=-96.623, fDelta:=10, sPmpsState:='TM1K2:ATM-TARGET5');


This is implemented correctly.

All of the ranges overlap here. It doesn't necessarily need to be fixed, the scientists might prefer this. It should be documented.
I ordered them in reverse because it made the overlaps easier for me to spot.

t5: (-106.623, -86.623)
t4: (-92.125, -72.25)
t3: (-77.874, -57.874)
t2: (-63.5, -43.5)
t1: (-49.124, -29.124)
out: (-26, -6)

Yes, I know about the overlap. How do you think it will react in terms of faults in case of overlap between states ?

I think the code will "pick one state" in the overlap region.
I do not know how the faults will react, maybe nothing bad will happen at all?

Maybe!? I will actually try to fix it and try to avoid overlaps by tuning delta.

I'll approve now and I'll approve again if you decide to tune the deltas

ZLLentz · 2024-10-08T22:33:17Z

plc-kfe-rix-motion/kfe_rix_motion/POUs/PRG_TM2K2_ATM.TcPOU

+fbStateSetup(stPositionState:=fbTM2K2.stOut, fPosition:=5.3, fDelta:=5, sPmpsState:='TM2K2:ATM-OUT');
+fbStateSetup(stPositionState:=fbTM2K2.stTarget1, fPosition:=-15.5, fDelta:=10, sPmpsState:='TM2K2:ATM-TARGET1');
+fbStateSetup(stPositionState:=fbTM2K2.stTarget2, fPosition:=-36.5, fDelta:=10, sPmpsState:='TM2K2:ATM-TARGET2');
+fbStateSetup(stPositionState:=fbTM2K2.stTarget3, fPosition:=-52.0, fDelta:=7, sPmpsState:='TM2K2:ATM-TARGET3');
+fbStateSetup(stPositionState:=fbTM2K2.stTarget4, fPosition:=-60.5, fDelta:=10, sPmpsState:='TM2K2:ATM-TARGET4');
+fbStateSetup(stPositionState:=fbTM2K2.stTarget5, fPosition:=-77.0, fDelta:=10, sPmpsState:='TM2K2:ATM-TARGET5');
+fbStateSetup(stPositionState:=fbTM2K2.stTarget6, fPosition:=-96.6, fDelta:=10, sPmpsState:='TM2K2:ATM-TARGET6');


Implemented correctly again, some overlaps again

t6: (-106.6, -86.6)
t5: (-87.0, -67.0)
t4: (-70.5, -50.5)
t3: (-59.0, -45.0)
t2: (-46.5, -26.5)
t1: (-25.5, -5.5)
out: (0.3, 10.3)

ZLLentz · 2024-10-09T20:06:32Z

Do you need help resolving the tmc merge conflict?

jyotiphy · 2024-10-09T20:14:20Z

Do you need help resolving the tmc merge conflict?

@ZLLentz yes please if you have time, can you please help resolve the conflicts and merge the PR ? Thank you!!

ZLLentz · 2024-10-09T20:19:01Z

Ok I will try to push a merge resolution commit to your branch

ZLLentz · 2024-10-09T20:25:54Z

Today I learned that VSCode has a really useful git merge conflict resolution editor
I was planning to handle this manually but the IDE recommended it to me and it made this much easier than it usually is

jyotiphy · 2024-10-09T21:46:34Z

Thanks @ZLLentz I would like to know about this VSCode merge conflict tool as well :)

ZLLentz · 2024-10-09T23:04:14Z

Link to docs: https://code.visualstudio.com/docs/sourcecontrol/overview#_3way-merge-editor

There's probably a way to do this all in VSCode but here's what I did instead:

I cloned the repo at latest pcdshub master
I added your fork as a remote and checked out your branch
Locally: git merge --no-ff master (while on your branch)
Merge conflict!
Open VSCode
Open the file with the conflict
Bottom right there was a thing to click "Resolve in merge editor"
The gui from the link popped up and showed me the single conflict
I clicked "accept incoming" on the left side because it was more up-to-date
It said there were no conflicts remaining
I clicked "complete merge" and it guided me to make a merge commit
I pushed the commit back to github on your branch for this PR

jyotiphy · 2024-10-10T00:57:51Z

Something wrong with this 'merge' or code, PLC is crashing when I deploy master...

ZLLentz · 2024-10-10T01:42:21Z

This PR's effective diff is just setting the fDelta, I can't imagine why this would itself crash the PLC
The merge conflict was in the tmc file which gets regenerated anyway when you deploy to the PLC

jyotiphy · 2024-10-10T04:15:58Z

Could it due to ppm updates @KaushikMalapati ? Both these PRs are being deployed together. I had tested my PR on PLC and it was fine. @KaushikMalapati did you test you code on PLC ?

jyotiphy · 2024-10-10T04:45:04Z

I'm able to run my branch (before 'master' merge from Zach which had new changes from Kaushik) fine on the PLC. [(https://github.com/jyotiphy/lcls-plc-kfe-rix-motion/commit/1a1944cdad28d93d34c58a0118c808aebeeb74bf)]
So there might be some issue with the ppm updates or the merge itself...

KaushikMalapati · 2024-10-10T04:53:00Z

I didn't test the if defined and updated IM4K2 function blocks, I only checked that it built without error. I could try running https://github.com/KaushikMalapati/lcls-plc-kfe-rix-motion/tree/responsive and see if it crashes @jyotiphy. If it does, it would mean the ppm code is the problem. If not then it might be something to do with the merge

jyotiphy · 2024-10-10T05:03:11Z

@KaushikMalapati okay, go ahead test it on PLC and let me know how it goes. I just tested my branch and it seems to be working fine.

KaushikMalapati · 2024-10-10T05:18:56Z

It ran for several seconds and then crashed. This error stands out to me

jyotiphy · 2024-10-10T05:27:03Z

Oh page fault... that's why PLC is crashing. Check your code and any calculations. Divide by 0 error anywhere ?

KaushikMalapati · 2024-10-10T06:27:37Z

I think it's the mod calculations like at https://github.com/pcdshub/lcls-twincat-common-components/blob/696a827d7f7836cf073dc747b06876632e4c9ae2/lcls-twincat-common-components/Library/Devices/PPM/FB_PPM_PowerMeter.TcPOU#L236. Since the array is one-indexed, whenever udBackgroundVoltageBufferIndex is a multiple of one hundred it tries to index into the array at zero, causing a page fault. This would explain why it takes some time for the crash to happen since udBackgroundVoltageBufferIndex is incremented once a second. I think a solution would be to index into the array like this
((udBackgroundVoltageBufferIndex-1) MOD 100) + 1. This ensures that the index is always between 1 and a hundred, but I would want someone to double check this. I have this running and it's not crashing so I can open a PR for it tomorrow

ZLLentz · 2024-10-10T16:54:15Z

I agree with that adjustment, sorry for missing that on review (I guess my brain is used to always-0-indexed arrays from other languages)

jyotiphy · 2024-10-10T17:53:59Z

@ZLLentz I was testing the "delta" implementation last night and it seems like after setpoint, motor is moving only about 2mm, both directions (and not what I had defined on the code). Is there any default set somewhere for Delta which is not being overwritten with this implementation or I'm missing something here ?

I think, currently PLC is running Kaushik's branch, so Delta update is not on PLC. Once Kauhsik's PR is finalized, merged and deployed, maybe you can give it a quick try as well and see if you can re-produce this issue ?

ZLLentz · 2024-10-10T18:10:56Z

Maybe you're getting trolled by autosave... let me check
edit: no, it's not this
I'm happy to check and report back after #73

jyotiphy added 3 commits October 8, 2024 14:28

Adding deltas to ATMs

d8be313

Some cleaning

6871a9f

Ran pre-commit

a77a73c

jyotiphy requested a review from ZLLentz October 8, 2024 21:39

ZLLentz reviewed Oct 8, 2024

View reviewed changes

something got messed up with licensen config

6d7dc31

ZLLentz previously approved these changes Oct 8, 2024

View reviewed changes

Ran pre-commit

4b0f407

jyotiphy dismissed ZLLentz’s stale review via 4b0f407 October 9, 2024 00:43

ZLLentz previously approved these changes Oct 9, 2024

View reviewed changes

Decreased delta to eliminate/dec the overlap between two setpoints

1a1944c

jyotiphy dismissed ZLLentz’s stale review via 1a1944c October 9, 2024 19:53

ZLLentz previously approved these changes Oct 9, 2024

View reviewed changes

Merge branch 'master' into cc_targets

30f884c

ZLLentz dismissed their stale review via 30f884c October 9, 2024 20:24

ZLLentz approved these changes Oct 9, 2024

View reviewed changes

ZLLentz merged commit 4c0dd65 into pcdshub:master Oct 9, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding deltas to ATMs #72

Adding deltas to ATMs #72

jyotiphy commented Oct 8, 2024 •

edited

Loading

ZLLentz Oct 8, 2024

ZLLentz Oct 8, 2024

jyotiphy Oct 8, 2024

ZLLentz Oct 8, 2024

jyotiphy Oct 8, 2024

ZLLentz Oct 8, 2024

ZLLentz Oct 8, 2024

ZLLentz commented Oct 9, 2024

jyotiphy commented Oct 9, 2024 •

edited

Loading

ZLLentz commented Oct 9, 2024

ZLLentz commented Oct 9, 2024

jyotiphy commented Oct 9, 2024

ZLLentz commented Oct 9, 2024

jyotiphy commented Oct 10, 2024

ZLLentz commented Oct 10, 2024 •

edited

Loading

jyotiphy commented Oct 10, 2024 •

edited

Loading

jyotiphy commented Oct 10, 2024

KaushikMalapati commented Oct 10, 2024 •

edited

Loading

jyotiphy commented Oct 10, 2024

KaushikMalapati commented Oct 10, 2024

jyotiphy commented Oct 10, 2024 •

edited

Loading

KaushikMalapati commented Oct 10, 2024

ZLLentz commented Oct 10, 2024

jyotiphy commented Oct 10, 2024

ZLLentz commented Oct 10, 2024 •

edited

Loading

Adding deltas to ATMs #72

Adding deltas to ATMs #72

Conversation

jyotiphy commented Oct 8, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Screenshots (if appropriate):

Pre-merge checklist

ZLLentz Oct 8, 2024

Choose a reason for hiding this comment

ZLLentz Oct 8, 2024

Choose a reason for hiding this comment

jyotiphy Oct 8, 2024

Choose a reason for hiding this comment

ZLLentz Oct 8, 2024

Choose a reason for hiding this comment

jyotiphy Oct 8, 2024

Choose a reason for hiding this comment

ZLLentz Oct 8, 2024

Choose a reason for hiding this comment

ZLLentz Oct 8, 2024

Choose a reason for hiding this comment

ZLLentz commented Oct 9, 2024

jyotiphy commented Oct 9, 2024 • edited Loading

ZLLentz commented Oct 9, 2024

ZLLentz commented Oct 9, 2024

jyotiphy commented Oct 9, 2024

ZLLentz commented Oct 9, 2024

jyotiphy commented Oct 10, 2024

ZLLentz commented Oct 10, 2024 • edited Loading

jyotiphy commented Oct 10, 2024 • edited Loading

jyotiphy commented Oct 10, 2024

KaushikMalapati commented Oct 10, 2024 • edited Loading

jyotiphy commented Oct 10, 2024

KaushikMalapati commented Oct 10, 2024

jyotiphy commented Oct 10, 2024 • edited Loading

KaushikMalapati commented Oct 10, 2024

ZLLentz commented Oct 10, 2024

jyotiphy commented Oct 10, 2024

ZLLentz commented Oct 10, 2024 • edited Loading

jyotiphy commented Oct 8, 2024 •

edited

Loading

jyotiphy commented Oct 9, 2024 •

edited

Loading

ZLLentz commented Oct 10, 2024 •

edited

Loading

jyotiphy commented Oct 10, 2024 •

edited

Loading

KaushikMalapati commented Oct 10, 2024 •

edited

Loading

jyotiphy commented Oct 10, 2024 •

edited

Loading

ZLLentz commented Oct 10, 2024 •

edited

Loading