-
Notifications
You must be signed in to change notification settings - Fork 48
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
Conventions #4
Comments
@mark-lazarides @Roald87 @philippleidig @jozefchmelar let's keep the discussion about conventions here... slack for quick chat; discussions here to keep track of the activity |
|
Fully agree.
We use it like described with our components. It is useful when controlling state of a sequence. In vast majority of cases bool suffice. Sometimes would be nice to have more information about the state of the method... but this would need wider discussion (maybe fluent-syntax like somethig)
No. there is not specific requirement for that nor Inxton nor tc.prober. We use it in this way. In inxton if you want to collect all components in a collection you can do that when There is also another reason for that. We are working these day on open-sourcing our base library, that has some requirements in regards. I hope will be able to come up with something next week. To give you more details.
Not a fan of prefixes either. The table resembles the system of prefixes we use... but again should we decide to get rid of the it would make me just happy. |
In most cases I don't see a benefit in using prefixes. I agree with @philippleidig's proposal. I'd say that pointer and reference are an exception here. I proposed my conventions in PR #5 Member naming & Type NamingI don't see a benefit in using prefix. It doesn't help me in any way. Member VariablesClass (FB) member variables should be hidden and begin with small name VAR
{attribute 'hide'}
trigger : BOOL;
{attribute 'hide'}
counter : INT;
{attribute 'hide'}
analogStatus : AnalogStatus;
END_VAR |
|
Fully agree 👍 There is also the attribute "conditionalshow". But this can only be used in conjunction with a compiled library.
I generally like to stick to the naming conventions and name selection of C#. |
@philippleidig what do you mean with return values? In this case they are used as error checks? Usually the return value depends on the method. Fully agree on the suggested variable naming! |
@Roald87 the idea would be that method of a component that performs an action would return 'true' when the action is completed (MoveToHome() when home sensor/position reached). This does not prevent other return types when necessary.
|
I would have a suggestion about arrays. There is a formal requirement for inxton compiler that trans-piles only arrays that are 0 based. The reason is to prevent the confusion when used in C#. _array : ARRAY[0..10] OF BOOL; // trans-piles Any comment on this? |
Same for TwinCAT HMI (TE2000) |
Would be very convenient to have the arrays in sync with the HMI indeed! |
@philippleidig || @Roald87 would any one of you PR conventions about arrays then pls... I just like seeing more contributors in the repo :). |
Because of the way structured text looping works, I prefer to keep PLC arrays dimensioned 1..X. The code is easier to read everywhere in the PLC as a result. I think we should be writing code that is attractive and maintainable on the PLC always. If we need shims to make it work better on third party bits of code, we can manage that separately. // Declaration
NUMBER_OF_DRIVES : INT := 10;
drives : ARRAY[1..NUMBER_OF_DRIVES] OF I_Drive;
// now in the code
FOR i := 1 to NUMBER_OF_DRIVES DO
drives[i].SomethingCool();
END_FOR
// Compared to
// Declaration
drives : ARRAY[0..(NUMBER_OF_DRIVES -1) ] OF I_Drive;
// Code
FOR i := 0 to (NUMBER_OF_DRIVES -1) DO
drives[i].SomethingCool();
END_FOR
|
I think methods should return what ever is reasonable for the method. The name of the method should help you understand that. i.e. IF NOT piece.PassesValidation() THEN
LogError('Piece does not pass validation');
END_IF
// OR
IF sequence.Finished THEN
axis.Disable(); // No return type necessary.
state := WaitForAxisDisabled;
END_IF
|
I really don't like the approach of repeatedly calling a public method, where that method is executing the funcationality repeatedly until it returns correct. There is a single case I think it;s accetpable (if there is an "Execute" type method on an interface, but there are also better ways for that to work too, I believe). The problems with it;
|
@philippleidig not familiar with TwinCAT HMI. How are non 0 based arrays handled there? |
Concurrency and race conditions are all reasonable concerns. IMHO These problems should be addressed as much as possible at the level of the components, but more importantly at the level of coordination when consuming the components. The component methods should be called from within properly implemented state-controller-like primitives (be it simple CASE, IF, ELSIF, or more complex sequencer/selector/iterator) that would prevent concurrent calls of conflicting methods of the same instance of a component. Something like this should be prevented in the component's consumer code
The What I have in mind is somethig like this:
This could be reduced to
edit: I assume that the component is used in single plc task |
It places constraints on the consuming the code. Our components should not be liable to errors if the consumer uses them in an incorrect order. They should respond well to all interaction. They won't necesasrily "work" (i.e. calling It still doesn't read well to me either. You can't read axis.MoveAbsolute(syx) and understand it needs to be called cyclically. You would only understand that if you knew that our idiomatic style required that of you and at that point I think we have failed in creating something very usable. I would also say, that regardless of whether errors can be factored out, they are still more likely. With the cyclic method call approach, you could create a method that checks whether the state of the object is ready to be called, then call the cyclic method, then monitor the other state of the object to ensure nothing has happened in the interim. Or you make the request, which tells you whether it is successful or not, then monitor status for completion. You can register a callback for that as part of the request if you want, which is a decent OO approach to this and reduces more calling / interrogating cyclically. |
@mark-lazarides correct! I |
OK. So why are we calling the method cyclically? I still think the original arguments stand. The method should do one job. Either start something (and therefore report the success of that) or get something (and return it). |
No objections Mark, we do not need to call execute methods cyclically, but it should not be an issue if we do. I do not see why a method could not return the result of the operation. I think we should come up with some more complex components and to prototype these ideas (pneumatic piston is not complex enough for this discussion) I think we could start with drive/axis. |
Agreed, Peter. Due to the thread name, I thought we were aiming for this as a convention! Apologies if I have miss-understood. Chris has a basic axis block in as a PR at the moment - I've commented on it, but it needs more eyes on it. |
@mark-lazarides no apologies needed Mark, we are here to discuss freely, I will have a look at PR tomorrow... |
@philippleidig @Roald87 @dhullett08 Discussion about component design also here |
A couple random suggestions taken from the PLCopen documents. plcopen_coding_guidelines_version_1.0.pdf Constants Acceptable Name Length |
Thanks for the link
👍
Longer names should not be a problem until they express the intent, 24 characters should suffice, however I would not put limit on max. characters. Too short names are indeed suspicious they should be more than 4 chars. |
@Seversonic your remarks added to the document... |
discussion cont. here #11 |
Plc cleaning up boot dir and loading testing projects
I realy dont like your Conventions but i think your Project is quite interesting! |
Hi, @PeterZerlauth and thanks. It is a bit laborious to agree on the conventions as we are halfway between PLC and classical software engineering. Here is the poll from the early beginning of the discussions: In addition to that, there was a discussion here and in the Slack Channel, there might be also something in @dhullett08 TcOpen repo. There was a general feeling (or at least I interpret it that way) that we should abandon prefixes if they do not deliver useful information or the modern IDE provides the information that we did convey with the prefixes in the past. I understand this is about personal preferences and there is really no right or wrong way of doing it. Just we have to agree on something. |
Closing here the discussion continues here: #11 |
Conventions document here
Please contribute to discussion bellow
The text was updated successfully, but these errors were encountered: