-
Notifications
You must be signed in to change notification settings - Fork 20
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
Implemented Changes to Expose Axis Parameters as READ-ONLY to EPICS #216
Conversation
…ues of the ST_AxisParameterSet structure to the new structure. Used to expose select values to the EPICS layer via pytmc pragmas.
…properly. Ran all unit tests and all tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me- I tried to find something to change but I think it's good as-is. I left comments about the things I checked.
io: i | ||
field: DESC Maximum commandable speed of the axis in EU/s. | ||
'} | ||
fVeloMaximum : LREAL; // Maximum commandable speed of the axis in EU/s. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought these needed to be above the parameter but I was wrong, these associate properly in the IDE as-is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test is pretty good, catches all the cases for now
It feels a bit reflexive as we discussed yesterday but it will catch errors if someone edits the FB later and mixes up some existing parameters
'} | ||
fCtrlPosDiffMax : LREAL; // Maximum magnitude of position lag in EU. | ||
{attribute 'pytmc' := ' | ||
pv: PosLagTime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the longest new PV name and it's still shorter than the longest existing PV so we're good on PV lengths here 👍
:PLC:AxisPara:PosLagTime
vs
:PLC:bGantryBackwardEnable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for future: here's how I found the longest as-deployed PV name, with a big assist from stackoverflow for use of awk:
(pcds-5.8.2)zlentz@psbuild-rhel7-01:~$ grep_pv :PLC: | grep MMS | awk ' { if ( length > x ) { x = length; y = $0 } }END{ print y }'
SP1L0:KMONO:MMS:DIODE_HORIZ:PLC:bGantryBackwardEnable_RBV, "bi"
stMotionStage.stAxisParametersExposed.fDecelerationMax := stMotionStage.stAxisParameters.fDecelerationMax; | ||
stMotionStage.stAxisParametersExposed.fEncSoftEndMax := stMotionStage.stAxisParameters.fEncSoftEndMax; | ||
stMotionStage.stAxisParametersExposed.fEncSoftEndMin := stMotionStage.stAxisParameters.fEncSoftEndMin; | ||
stMotionStage.stAxisParametersExposed.fVeloMaximum := stMotionStage.stAxisParameters.fVeloMaximum; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an appropriate selection
Maybe in the future people would be interested in more but these cover the essentials as relating to EPS
I need to check, I don't think it exists, and if it doesn't already, can we add encoder scaling and offset? @NSLentz |
Added encoder scaling and offset to NC configuration. Requesting re-review to merge. Testing steps were repeated with an updated test case for the added parameters. |
Description
Created a new DUT called ST_AxisParameterSetExposed.
Added the new DUT as a member of ST_MotionStage.
Added logic to FB_MotionStage to copy the exposed parameters into the new DUT whenever the stAxisParameters structure is updated.
Added pytmc pragmas to the ST_AxisParameterSetExposed to populate the new PVs when an IOC is built using lcls-twincat-motion.
Motivation and Context
This new feature came up as a result of difficulties with scientists and other staff in understanding why certain motions cause errors or are not allowed to be completed. With these parameters exposed, they will have greater transparency with understanding the limits of the system that have been programmed into the PLC.
How Has This Been Tested?
A new unit test was added to test that the values are being copied properly.
All the previous unit tests and the new unit test were run and they all pass.
The library was temporarily installed as version 0.0.0.
The 0.0.0 library version was selected in a test PLC project, and used to define a motion stage.
A test ioc was built on tst-console and a test screen was created to query the 10 PVs that were added.
Each PV was changed within the NC configuration and the test screen was watched to make sure the value updated to the correct new value within a reasonable amount of time (about 1 second).
Where Has This Been Documented?
The code has been commented thoroughly to explain the intended function of each added PV. In addition, the changes to FB_MotionStage have a code comment to describe the intended function of the new copying of parameters.
Pre-merge checklist
Always Newest
version (Library, *
)pre-commit
or ranpre-commit run --all-files