Skip to content
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

create library and refactor code and change it to OOP... #1

Open
runtimevic opened this issue Apr 29, 2023 · 22 comments
Open

create library and refactor code and change it to OOP... #1

runtimevic opened this issue Apr 29, 2023 · 22 comments

Comments

@runtimevic
Copy link
Collaborator

Hello @fisothemes,
congratulations for spreading your work, I think it is the right way to improve.
I liked your idea implementing timers...
Would you like me to create a single FB that groups them all..., refactoring the code and passing it to the OOP paradigm and also create a timer library...
Tell me if you are interested and we can do it collaborating or I do it and see if you like it...
I wait your answer...

@fisothemes
Copy link
Owner

Hello @runtimevic,
That sounds nice, I would be happy to collaborate or see what you come up with.
Thanks for the taking an interest and for contributing,

@fisothemes
Copy link
Owner

Hi @runtimevic
Thanks again, I though it would be helpful to share some background and my thought process on the matter.

Timer Library:
I previously created a library the first version is tagged v0.1.0.0.

Bundling timers into a single FB and bringing it into line with OOP:
When I first started, I thought about doing this. The reason why I didn't group them all into a single FB was because I wanted make the use of the timers familiar by basing them on those found in the Tc2_Standard library. It also made refactoring old code easy.
Bundling them into a single OOP FB would be nice. I'm having a think as to how it would can be done in a way that is friendly for devs new and old alike (Similar to FB_Timer were parameters are descriptive and it works in similar way to general consumer hardware timers or timers you have on your mobile device).
I'm interested in what you have so far. Nothing has been set stone so I am open to anything.

Refactoring:
I did think about coming back and refactoring the code as this made on my early journey to learning PLC programming, but I had my hands full with other projects specifically a TwinCAT ADS client for LabVIEW.

@runtimevic
Copy link
Collaborator Author

runtimevic commented Apr 30, 2023

Hello @fisothemes
I think that it is best to reach consensual agreements between the 2:

  • The first issue that I would like to raise is that there is no normalization of the names of the input variables, outputs...
VAR_INPUT
	IN 	: BOOL; // Starts timer with falling edge, resets timer with rising edge
        PAUSE: BOOL
	PT 	: TIME; // Time to pass, before Q is set
END_VAR
VAR_INPUT
        bStart 		: BOOL; // Start (TRUE) or Stop (FALSE) Timer 
	bPause 		: BOOL; // Pause Timer (TRUE), Only Pauses if IN := TRUE
        tSet		: TIME; // Set Time
END_VAR
VAR_OUTPUT
	Q 	: BOOL; // is FALSE, PT seconds after IN had a falling edge
	ET 	: TIME; // Elapsed time
END_VAR
VAR_OUTPUT
    bElapsed 	: BOOL;	// Becomes TRUE after Set Time has passed
    tElapsed 	: TIME; // Time Elapsed
END_VAR

I think we should unify the names of the variables, tell me which of the 2 forms do you like the most? for us to change it...

@fisothemes
Copy link
Owner

I think we should stick to the PLC programming convention used by Beckhoff, they already have pre-built Static Analysis configuration file, which saves us the headache of producing our own and makes reviews easier. The downloads for the config files are found in the link I gave.

VAR_INPUT
        bStart 	: BOOL; // Start (TRUE) or Stop (FALSE) Timer 
	bPause 	: BOOL; // Pause Timer (TRUE), Only pauses if bStart := TRUE
        tSet 	: TIME; // Set Time
END_VAR

@runtimevic
Copy link
Collaborator Author

I think we should stick to the PLC programming convention used by Beckhoff, they already have pre-built Static Analysis configuration file, which saves us the headache of producing our own and makes reviews easier. The downloads for the config files are found in the link I gave.

VAR_INPUT
        bStart 	: BOOL; // Start (TRUE) or Stop (FALSE) Timer 
	bPause 	: BOOL; // Pause Timer (TRUE), Only pauses if bStart := TRUE
        tSet 	: TIME; // Set Time
END_VAR
  • so what do you think is better?
    bStart or IN?

@fisothemes
Copy link
Owner

bStart

@runtimevic
Copy link
Collaborator Author

runtimevic commented Apr 30, 2023

bStart

  • I think we should do without the Hungarian notation==🤮 do you agree?
    let me give these symbol hints:
    Start, Pause, PreSet, Done, Elapsed.
    What do you think?

@fisothemes
Copy link
Owner

fisothemes commented Apr 30, 2023

Yh I personally don't like it either, but in terms of PLC programming Hungarian notation is pretty much a thing to overcome the pitfalls of the language and it's implementations such as being case insensitive, difficult to read in plain text (metadata everywhere on the file) and potential clashes with reserved keyword, local enums and Tc2 stuff.

@runtimevic
Copy link
Collaborator Author

  • We have to decide with or without Hungarian notation? bStart or Start?

@fisothemes
Copy link
Owner

With. So bStart unfortunately

@runtimevic
Copy link
Collaborator Author

With. So bStart unfortunately

  • So we stay with these symbols to unify everything the same?
    bStart, bPause, tPreSet, bDone, tElapsed.
    Or
    biStart,biPause,tiPreSet,boDone,toElapsed.

@fisothemes
Copy link
Owner

bStart, bPause, tPreSet, bDone, tElapsed.

We'll stick with Beckhoff's conventions, https://infosys.beckhoff.com/english.php?content=../content/1033/tc3_plc_intro/12073947403.html&id=3338831657965116106

@runtimevic
Copy link
Collaborator Author

runtimevic commented May 1, 2023

https://alltwincat.com/2019/02/11/plc-naming-conventions/

  • so that you can also read it, there are also people who make a difference in the symbolic if they are inputs/outputs and even the scope of the signal if it is gvl, local, but it already seems like too many letters...

bStart, bPause, tPreSet, bDone, tElapsed.

  • Very well, we'll stay like this, I'll change it tomorrow or do you want to change it yourself?

  • In order to pass it to OOP, the programming that is in the main body of each FB I need to pass it to a method, for example it can be called M_Run. This method has to come from an interface I_Run and/or from an abstract FB FB_Timer_Abstract, is that okay?

  • I would also like to add all the properties "attributes" of the variables that we need to be able to read or write them.

  • I would also like to put structures grouping the variables.

  • Decide how to have the different types of timers in a single FB, there are still too many things to do everything at the same time, I can do it all at the same time without problems, but I prefer that you see the process as soon as we go one to one...

@fisothemes
Copy link
Owner

Adding i/o prefixes:
Yh that's a bit much, i can also be easily mistaken for int. Adding stuff like li, di, ui, etc. will hamper refactoring efforts on number types, ui is a bad idea for obvious reasons.

Passing as OOP:
I can do the refactoring myself this evening if you haven't already. Or you can put in what you have now and later I'll scan through it and refactor as well as do the documentation.

I'm pretty much happy with everything else.

@runtimevic
Copy link
Collaborator Author

Give me a few days and I'll send you the changes so you can see what you think...

@runtimevic
Copy link
Collaborator Author

runtimevic commented May 2, 2023

I have sent you a version, tell me what you think?,
but I think it can be improved by using POINTER and NEW for the list of timers, in this way you would not have the entire list of timers...

https://twincontrols.com/community/twincat-knowledgebase/factory-method-design-pattern/#post-470

Tomorrow if I have time I'll try it and if it works I'll send you a new version...

@fisothemes
Copy link
Owner

I'm gonna run some tests them give me a moment.

@fisothemes
Copy link
Owner

I have sent you a version, tell me what you think?, but I think it can be improved by using POINTER and NEW for the list of timers, in this way you would not have the entire list of timers...

https://twincontrols.com/community/twincat-knowledgebase/factory-method-design-pattern/#post-470

Tomorrow if I have time I'll try it and if it works I'll send you a new version...

I love the idea of a polymorphic timer. I can see this coming in handy in situations when you have to create dynamic timers. One suggestion would be to add a type property to the base Timer class. It will allow for some introspection which will be be handy for situation like these:

fbTimerCollection
	.Insert(fbRStopwatch)
	.Insert(fbRTimer);
fbTimerCollection.Get(14, ipTimer => ipTimer);

CASE ipTimer.Timer_Type OF 
	E_Timer_Type.Stopwatch: 
		// Do something
	E_Timer_Type.RTimer:
		// Do something
ELSE
	// Default
END_CASE 

I use this pattern regularly when creating steps for sequencers and retentive timers ensure that a process has actually run it's course, even if it is paused.

fbSequence
	.Insert(11, fbPurgeStep)
	.Insert(0, fbInitStep);
	.
	..
	...

Another thing is to probably reduce the use of REF=

fbRStopwatch.Input REF= stInput;
fbStopwatch.Input REF= stInput;

fbRStopwatch.Run();
fbStopwatch.Run();

stROutput := fbRStopwatch.Output;
stOutput := fbStopwatch.Output;

This could cause unintended side-effects especially when working with dynamic memory. My one big gripe with Beckhoff at the moment is that they haven't fixed the STRUCT access bug Codesys fixed a long time ago, the one were that makes the compiler throw the towel when you try to access properties that are structs and methods that return structs. One way I solve this is by replacing STRUCTs with FB's with only properties. Aside from the 4-16bytes that are object headers mainly used for RPCs, they're the same in memory.

Tomorrow, if I have time, I might investigate what happens when <Timer Instance>.FB_Init(E_Timer_Type.<type>) is called multiple times during a run and pen a few designs ideas trying to make all timers work in unison without having to instantiate a new one for each timer type.

Overall, I like the idea and I have learnt quite a bit. Also, I didn't even know about the twincontrols website, thanks for that, And thanks again for contributing.

@runtimevic
Copy link
Collaborator Author

runtimevic commented May 4, 2023

Hello @fisothemes

  • I just sent you a new version, I hope you like it and enjoy learning new things..
  • I have not been able to work with POINTER and using __NEW I have left the code commented in case you want to try it....
  • I think you should refactor the instructions of the FB_RTimer, it can be made easier..., I'll leave it to you...
  • There are things that I would change..., I think you can reduce the types of timers you have now that there are 7..., FB_Timer and FB_Stopwatch could become one...
  • You still need to add to your project the timers such as: FB_TOF, FB_TON, FB_RTP without using the ones from the Beckhoff library TOF, TON, RTP...
  • The issue of unit tests would also be missing, which I think are important and must be done at the same time as when you start with the creation of the small units that are part of the libraries..., when I have time I will also start with This part, which will be to compile the library, install it and create another project for unit tests or in the same project, create the unit tests...
  • While writing this I have noticed a small modification, I modify it and send it again...

You will tell me...

@runtimevic
Copy link
Collaborator Author

Hello @fisothemes
I have already sent the last changes, tell me what you think, you have an example of use with everything in the main...
I would like to start with the unit tests..., we must choose what we do them with...

@fisothemes
Copy link
Owner

Hello,
Sorry for the late reply I caught up in a lot of things.
Thanks for the contributions, I put your name in the MIT license, if you don't mind.

I'm trying to get up to speed with the changes you made. I like the changes so far. I didn't know you could access members of structs in property structs by using REFERENCE TO ST_Struct. In the past the compiler used always throw an error.

@fisothemes
Copy link
Owner

I would like to start with the unit tests..., we must choose what we do them with...

There are 4 things I checked for when running the timers to ensure that they run correctly.

  1. I check that the retentive timers run the same as their non-retentive variants under normal conditions such as a plc task with a 1ms cycle tick with a pre-set of 1m.
  2. I check if the r-timers behave correctly if the division between the cycle tick and pre-set is irrational, e.g. cycle tick is 44s for a pre-set of 1m.
  3. I check if the r-timers retain the time if they're not called by a for a period of timer in plc cycle.
  4. I check if the r-timers behave correctly if they called multiple times in the same cycle.

Writing unit tests for this might be tricky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants