Closed PTKu closed 3 years ago
@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
In my opinion properties should be defined as follows "IsEnabled". The name itself should already indicate what type it is.
I like the return value of the method as boolean. I find more complex datatypes inappropriate, because they have to be instantiated outside or returned by reference.
The inheritance of each base class of "fbComponent" is necessary for using Inxton or tc.prober?
When it comes to type naming, I personally am a bit radical and generally leave out prefixes. Except for interfaces, references and pointers. e.g.
Type Naming
Block type | Notation | Prefix | Example |
---|---|---|---|
FB/CLASS name | PascalCase | No | Cyclinder |
ENUM type name | PascalCase | No | MachineState.Start |
INTERFACE name | PascalCase | I | ICyclinder |
|
FUNCTION name | PascalCase | No | Add() |
STRUCT name | PascalCase | No | Data |
UNION name | PascalCase | No | Control |
@philippleidig
- In my opinion properties should be defined as follows "IsEnabled". The name itself should already indicate what type it is.
Fully agree.
- I like the return value of the method as boolean. I find more complex datatypes inappropriate, because they have to be instantiated outside or returned by reference.
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)
- The inheritance of each base class of "fbComponent" is necessary for using Inxton or tc.prober?
No. there is not specific requirement for that nor Inxton nor tc.prober. We use it in this way. ComponentBase
is an abstract class that has some public contract (Manual Method, etc), but it can implement some common features for components. I am not a great fan of inheritance (I prefer composition), but in this case I would like to have some option opened for the future.
In inxton if you want to collect all components in a collection you can do that when something is copmonent
there is mechanism for that.
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.
- When it comes to type naming, I personally am a bit radical and generally leave out prefixes. Except for interfaces, references and pointers. e.g.
Type Naming
Block type Notation Prefix Example FB/CLASS name PascalCase No
Cyclinder
ENUM type name PascalCase NoMachineState.Start
INTERFACE name PascalCaseI
ICyclinder
FUNCTION name PascalCase NoAdd()
STRUCT name PascalCase NoData
UNION name PascalCase NoControl
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
I don't see a benefit in using prefix. It doesn't help me in any way.
Class (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
@jozefchmelar
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 Naming
I don't see a benefit in using prefix. It doesn't help me in any way. 👍👍
Member Variables
Class (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
trigger
(variable name) and Trigger
(property name) would conflict. We will need to prefix it with _
as proposed I guessFully agree 👍
There is also the attribute "conditionalshow". But this can only be used in conjunction with a compiled library. https://infosys.beckhoff.com/english.php?content=../content/1033/tc3_plc_intro/8095402123.html&id=7685156034373049758 Since I assume that we will provide an open library, this would only make limited sense.
_
as prefix for member variables is neccesary as @PTKu said.
I generally like to stick to the naming conventions and name selection of C#.
I like the return value of the method as boolean. I find more complex datatypes inappropriate, because they have to be instantiated outside or returned by reference.
@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. CalculcateArea
would return an REAL
`LREAL`.
Fully agree on the suggested variable naming!
I like the return value of the method as boolean. I find more complex datatypes inappropriate, because they have to be instantiated outside or returned by reference.
@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.
CalculcateArea
would return anREAL``LREAL
.
@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.
Fully agree on the suggested variable naming!
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 while _array : ARRAY[1..10] OF BOOL; // does not trans-pile
Any comment on this?
Same for TwinCAT HMI (TE2000)
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 :).
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 while _array : ARRAY[1..10] OF BOOL; // does not trans-pile
Any comment on this?
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 like the return value of the method as boolean. I find more complex datatypes inappropriate, because they have to be instantiated outside or returned by reference.
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
@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 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;
atEnd := axis.GoToEnd();
atBeginning := axis.GoToBeginning();
@philippleidig not familiar with TwinCAT HMI. How are non 0 based arrays handled there?
@mark-lazarides
I think methods should return what ever is reasonable for the method. The name of the method should help you understand that. 👍
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
atEnd := axis.GoToEnd();
atBeginning := axis.GoToBeginning();
The executing methods
returning true
when done allow for clean declarative use.
What I have in mind is somethig like this:
VAR
_state : INT;
END_VAR
====================================
CASE _state OF
0:
IF(axis.MoveAbsolute(Position: 100.0)) THEN
_state := 1;
END_IF;
1:
IF(axis.MoveRelative(Position: 100.0)) THEN
_state := 2;
END_IF;
2:
IF(axis.MoveAbsolute(Position: 300.0)) THEN
_state := 3;
END_IF;
3:
_state := 0;
END_CASE
This could be reduced to
VAR
_state : INT;
END_VAR
====================================
CASE _state OF
0:
Await(axis.MoveAbsolute(Position: 100.0),1);
1:
Await(axis.MoveRelative(Position: 100.0),2);
2:
Await(axis.MoveAbsolute(Position: 300.0),3);
3:
Await(true,0);
END_CASE
===================================
===================================
METHOD Await
VAR_INPUT
done : BOOL
nextState : INT;
END_VAR
IF (done) THEN
_state := nextState;
END_IF;
=====================================
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 res := axis.MoveTo(Position:=100);
before axis.Enable()
wouldn't work), but the consuming code should be given enough information at all points to understand where the issue it.
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 execute methods
(let's call them that way) should not implement cyclic logic. I did assume what we have been discussing previously that we ensure that what needs to be executed in a cyclical manner would be placed either in the body of FB or in a Cyclic
method; that should be called in some appropriate place in consumer's program.
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).
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 Should be in ALLCAPS so they are easily identifiable
Acceptable Name Length Minimum of 4 characters maximum of 24 characters?
A couple random suggestions taken from the PLCopen documents.
Thanks for the link
Constants Should be in ALLCAPS so they are easily identifiable
👍
Acceptable Name Length Minimum of 4 characters maximum of 24 characters?
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
I realy dont like your Conventions but i think your Project is quite interesting! personaly i prefer a more classic way FB_ fb function Block M_Add() Method P_Parameter Prop
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: https://github.com/TcOpenGroup/TcOpen/discussions/11
Conventions document here
Please contribute to discussion bellow
Let's keep the discussions here for tracking.
Quick chats here:![TcOpen Slack](https://img.shields.io/badge/Slack-channel-ff69b4.svg)
[ ] Naming conventions for variables (VAR, VAR_INPUT, VAR_OUTPUT, VAR_IN_OUT, VAR_INST, TEMP)
[ ] Naming convention for methods
[ ] Naming convention for properties
[ ] Naming convention for blocks (FB, FC, PRG etc.)