Conversation
mac-kan
left a comment
mac-kan
left a comment
There was a problem hiding this comment.
Please check through my comments and clean up the code before it is reviewed and tested further. Some of the comments are small and a little bit silly, but is there since we work on a common project and have a code style guide.
If you aren't aware, please check the TwinCAT + Git Development Workflow . It specifies how to perform a commit. As tc_mca_std_lib is a common project, commits shall be made in a fashion similar to:
"
Short title in imperative
Body text explaining what was done and why the change was introduced (in present tense).
"
To update a commit message is easy afterwards, just use an interactive rebase. Please ask if you want me to show you! This will help people reviewing and understanding the code in the future.
In the same guide (TwinCAT + Git Development Workflow), it is specified that a branch name shall be reflecting the Jira issue number + a small description. Please update.
| IF (eSlitPairState <> E_SlitPairStates.INIT AND NOT bFunctionInErrorState) AND NOT bEnable THEN | ||
| eSlitPairState := E_SlitPairStates.ERROR; | ||
| END_IF | ||
| //IF eSlitPairState <> E_SlitPairStates.INIT AND NOT bAxesEnabled THEN |
There was a problem hiding this comment.
Code which has been commented should not remain since we have version control allowing the reversion of a change.
| @@ -0,0 +1,16 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
The E_SlitSoftLimits is not reused, and instead of creating a DUT, it could be declared directly in FB_Axis as:
eSoftLimitsUpdate: (START, WRITE_GAP_CENTRE_FWD, WRITE_GAP_CENTRE_BWD, WRITE_GAP_SIZE_FWD) := START;
@federrg Do we have any defined standard regarding this?
| eSlitPairState: E_SlitPairStates := E_SlitPairStates.INIT; //statemachine index | ||
|
|
||
| //Internal statuses for logic | ||
| bEnable: BOOL; //Enable/disable the slit set |
There was a problem hiding this comment.
The bEnable and bReset is placed under //Internal statuses for logic. The definition of each word, as well as the comment after the declaration, mean that they control something. Please revise.
There was a problem hiding this comment.
this is as it is because it is still a question about the general functionality. This can be removed when the entire functionality is tested
POUs/Motion/FB_SlitPair.TcPOU
Outdated
|
|
||
| CASE eSoftLimitsUpdate OF | ||
|
|
||
| E_SlitSoftLimits.START: //START SEQ Software limit center gap fwd (When not busy start) |
There was a problem hiding this comment.
The START state is also used to write to the axis parameters. Since the remaining states are named WRITE_GAP_X, it would be good to also use a similar terminology for this state.
POUs/Motion/FB_SlitPair.TcPOU
Outdated
| - GVL.astAxes[iGapSize].stStatus.fActPosition/2); | ||
| END_IF | ||
|
|
||
| E_SlitSoftLimits.WRITE_GAP_CENTRE_FWD: //WRITE GAP CENTRE FWD SOFT LIMIT DONE. Calculate software limit center gap bwd |
There was a problem hiding this comment.
General comment for the states WRITE_GAP_X:
The state name is e.g. WRITE_GAP_CENTRE_FWD, but the comment says it is done and that we calculate the limit center gap bwd. This is better to align correctly.
There was a problem hiding this comment.
I have re-structured this and I think it looks better
Update soft limits to have a better understanding of the sequence. New step finish added
|
Thanks @mac-kan for the review. I think updating the comment is time that I do not have now. I have done some corrections, maybe I missed something. It would be good to review the functionality asap as I already the code in ODIN and it nees to work well. Cosmetics can be fixed later |
mac-kan
force-pushed
the
rubenm/slits-code-multimaster
branch
5 times, most recently
from
b11d501 to
a2d4284
Compare
Aligns the FB_SlitPair with the coding style guide. Removes unused VAR_INPUTRemoves unused VAR_INPUT. Corrects spelling.
mac-kan
force-pushed
the
rubenm/slits-code-multimaster
branch
from
a2d4284 to
064cdfd
Compare
|
Updates were continued under #43 , it is now finalized and merged, so I am going to close this merge request. |
3 participants
The slits code is ready for review. You can read some description about implementation and operation here:
https://confluence.ess.eu/display/MCAG/SLITS+system
Test cases: