Restructuring DS402 support. Added MCS2ChanParameters ST and additional FBs to support cleaner and extra operations aside from FB_motionStage & ST_MotionStage#249
Conversation
lcls-twincat-motion/Library/DUTs/DS402/ST_MCS2ChanParameter.TcDUT
Outdated
Show resolved
Hide resolved
…al Splits
- Ensured stage position is set only on the very first homing request for a stage, preventing cumulative motor/stage offset errors without the need for explicit offset values on the motor record.
- Added `pytmc` pragma for MCS2 channelstate raw status.
- Removed unused variable/error checks for improved code clarity.
- Split SDO (channel state status, open-loop parameter configurations) and DS402 MCS2 support into more modular, smaller functional blocks.
- Removed duplicate MCS2 structures and cleaned up legacy mode selection methods.
- Restructured DS402 support logic:
- Introduced `MCS2ChanParameters` structure.
- Added new FBs for modular channel support (in addition to FB_motionStage & ST_MotionStage).
- Improved support for reading safety flags and module parameter updates; refactored SDO operations for clarity and maintainability.
- Added support for calibrations; split and renamed SDO parameter read & update methods for maintainable operation.
NSLentz
left a comment
NSLentz
left a comment
There was a problem hiding this comment.
This is really too long to properly review but looking through I don't notice any red flags. The PV name lengths could be reduced to help with our max pv length restrictions.
lcls-twincat-motion/Library/DUTs/DS402/ST_MCS2ChanParameter.TcDUT
Outdated
Show resolved
Hide resolved
yeah, something I've suggested myself, just removed the PLC prefix on properties. |
…ge, for ease of testing. moved error reporting to a dedicated FB
| <Released>false</Released> | ||
| <Title>lcls-twincat-motion</Title> | ||
| <ProjectVersion>0.0.0</ProjectVersion> | ||
| <ProjectVersion>1.0.0</ProjectVersion> |
There was a problem hiding this comment.
| <ProjectVersion>1.0.0</ProjectVersion> | |
| <ProjectVersion>0.0.0</ProjectVersion> |
There was a problem hiding this comment.
could be. will handle the version in the release stage
| <Compile Include="DUTs\ST_MotionEpicsInterface.TcDUT"> | ||
| <SubType>Code</SubType> | ||
| </Compile> |
There was a problem hiding this comment.
Why is this being removed? I don't see any changes to the file
There was a problem hiding this comment.
Good catch, before 4.6.0, this was not needed anymore as we decided to create a separate lib for motion abstraction.
| Unknown := 0, // Default/uncategorized | ||
| MoveDone, // Absolute move completed | ||
| StepMoveDone, // Step/JOG move completed | ||
| HomeDone, // Homing completed | ||
| CalibrationDone, // Calibration completed | ||
| ResetDone, // Reset command completed | ||
| HaltDone, // Manual halt completed | ||
| AbortedHome, // Home aborted | ||
| AbortedStepMove, // Step move aborted | ||
| AbortedCalibration, // Calibration aborted | ||
| Referenced, // Axis became referenced | ||
| Calibrated // Axis became calibrated |
There was a problem hiding this comment.
Should these be capitalized?
| {attribute 'hide'} | ||
| nChanFErrWinIdx : WORD; |
There was a problem hiding this comment.
Why are these hidden?
| // Temporary workaround: Avoid declaring mc_power externally from FB_MotionStage | ||
| bOverrideDirEnable : BOOL := TRUE; |
There was a problem hiding this comment.
Is this workaround still needed? I don't see it used anywhere
| HOME := 10, | ||
| GEAR := 30 | ||
| GEAR := 30, | ||
| CALIBRATE:=40 |
There was a problem hiding this comment.
Is this used anywhere?
There was a problem hiding this comment.
yeah. check the motion stage
|
Generally speaking, I think it'd be nice if all the MCS2 function blocks were coupled together somehow so you wouldn't need to instantiate and handle every one manually for every axis in a project. |
| {attribute 'hide'} | ||
| ftStepUpdateDone : F_TRIG; | ||
| tonStartUp : TON; | ||
| nJogParamStep : INT := 0; // 0=idle/start, 1=steps, 2=amp, 3=freq, 10=done, -1=error |
There was a problem hiding this comment.
This should be an enum
There was a problem hiding this comment.
yeah. i'll turn it into one
There was a problem hiding this comment.
Can you also do it for the other FB_Write***Parameters?
| <ST><
…eManager.TcPOU fix doc string syntax Co-authored-by: KaushikMalapati <80156796+KaushikMalapati@users.noreply.github.com>
|
@KaushikMalapati are we missing anything for the screens? |
| bReferenced := TRUE; | ||
| eStepState := E_MoveState.DONE; | ||
| ELSE | ||
| bReferenced := FALSE; |
There was a problem hiding this comment.
It would be nice if we only used homed or only referenced instead of using them interchangeably
| E_MoveState.STARTED: | ||
| IF NOT stDS402Drive.stDriveStatus.TargetReached | ||
| AND NOT stDS402Drive.stDriveStatus.Bit13_OpModeSpecific THEN | ||
| eStepState := E_MoveState.IN_PROGRESS; | ||
| END_IF |
There was a problem hiding this comment.
I think we can skip this and just go from INIT to IN_PROGRESS
| 1: // RESET FAULT | ||
| stDS402Drive.nDriveControl := 16#80; | ||
| tonStartupStep(IN := TRUE); | ||
| IF tonStartupStep.Q THEN | ||
| tonStartupStep(IN := FALSE); | ||
| eStartupState := 2; |
There was a problem hiding this comment.
I think you should have made a method to set and wait nDriveControl instead of repeating this pattern
| IF bOperational THEN | ||
| CASE stDS402Drive.nModeOfOperationDisplay OF | ||
| E_DS402OpMode.CSP: | ||
| bCSPModeEnabled := TRUE; | ||
| bHomeModeEnabled := FALSE; | ||
| bStepModeEnabled := FALSE; | ||
| bCalibrationModeEnabled := FALSE; | ||
| E_DS402OpMode.HOME: | ||
| bCSPModeEnabled := FALSE; | ||
| bHomeModeEnabled := TRUE; | ||
| bStepModeEnabled := FALSE; | ||
| bCalibrationModeEnabled := FALSE; | ||
| E_DS402OpMode.MCS2_OL_STEP_MODE: | ||
| bCSPModeEnabled := FALSE; | ||
| bHomeModeEnabled := FALSE; | ||
| bStepModeEnabled := TRUE; | ||
| bCalibrationModeEnabled := FALSE; | ||
| E_DS402OpMode.MCS2_CALIBRATION: | ||
| bCSPModeEnabled := FALSE; | ||
| bHomeModeEnabled := FALSE; | ||
| bStepModeEnabled := FALSE; | ||
| bCalibrationModeEnabled := TRUE; | ||
| ELSE | ||
| bCSPModeEnabled := FALSE; | ||
| bHomeModeEnabled := FALSE; | ||
| bStepModeEnabled := FALSE; | ||
| bCalibrationModeEnabled := FALSE; | ||
| END_CASE | ||
| ELSE | ||
| bCSPModeEnabled := FALSE; | ||
| bHomeModeEnabled := FALSE; | ||
| bStepModeEnabled := FALSE; | ||
| bCalibrationModeEnabled := FALSE; | ||
| END_IF |
There was a problem hiding this comment.
This could be simplified by setting all four booleans false and having a switch where each case just sets a single variable true
| END_IF | ||
|
|
||
| // Mode switch/request evaluator | ||
| IF bUserExec AND ( |
There was a problem hiding this comment.
Shouldn't this be set to false in the case?
I believe so, but install your latest branch of this library onto the dream motion and rebuild/restart the ioc so I can double check? |
| tonStartUp( | ||
| IN := bEnable AND NOT bCalibrationsParamsSet, | ||
| PT := T#4S | ||
| ); |
There was a problem hiding this comment.
Is there something we can poll for instead of waiting for a fixed period of time?
| IF ftFErrWinSetDone.Q OR ftSoftLimMinSetDone.Q OR ftSoftLimMaxSetDone.Q OR | ||
| ftMaxCloseLoopFreqSetDone.Q OR ftSensorPowerModeSetDone.Q OR | ||
| ftLogicalScaleOffsetSetDone.Q OR ftLogicalScaleInvSetDone.Q THEN |
There was a problem hiding this comment.
I think every falling trigger might need its own if statement. It doens't make sense that you would set the error based on fbSetFErrorWin if it's not done but any of the other ones are.
| END_VAR | ||
| ]]></Declaration> | ||
| <Implementation> | ||
| <ST><![CDATA[nScaledFErrWin:=LREAL_TO_DINT(( stMotionStage.stAxisParameters.fCtrlPosDiffMax / MAX( stMotionStage.stAxisParameters.fEncScaleFactorInternal, stMCS2ChanParameter.fScalingFactor))); |
There was a problem hiding this comment.
Would you mind explaining why we take the max here and how these two values could be different? In any case, I would store it in variable instead of recalculating for the soft limits
| END_VAR | ||
|
|
||
| VAR PERSISTENT | ||
| nCommandLocal : INT; |
There was a problem hiding this comment.
Why is nCommandLocal being saved?
| <ST><![CDATA[(*Do not change a moving axis parameter*) | ||
| fbMcWriteParameter( |
There was a problem hiding this comment.
Do you need to check for a null stMotionStage reference here?
| <Method Name="WriteParameterNC" Id="{a04e1bd8-55ae-495c-bca4-be27f11603b4}"> | ||
| <Declaration><![CDATA[METHOD WriteParameterNC | ||
| VAR_INPUT | ||
| Execute:BOOL; |
There was a problem hiding this comment.
Why would you call this method with execute false?
| //AND stMCS2ChanParameter.bOverrideDirEnable; | ||
| stMotionStage.bAllBackwardEnable:= stMotionStage.bLimitBackwardEnable | ||
| AND ( stMotionStage.bGantryBackwardEnable OR NOT stMotionStage.bGantryAxis) | ||
| AND stMotionStage.stEPSBackwardEnable.bEPS_OK; | ||
| //AND stMCS2ChanParameter.bOverrideDirEnable; |
There was a problem hiding this comment.
do the bOverrideDirEnable lines need to be removed or uncommented with OR?
| END_VAR | ||
| ]]></Declaration> | ||
| <Implementation> | ||
| <ST><
Description
https://jira.slac.stanford.edu/browse/ECS-9134
Motivation and Context
DREAM slit and Sample Paddle Support.
How Has This Been Tested?
Tested in the Maker space context with SmartAct MCS2 EtherCAT drive and BSD PLC
Where Has This Been Documented?
Code contains appropriate documentation
Pre-merge checklist
Always Newestversion (Library, *)pre-commitor ranpre-commit run --all-files