fisothemes / TwinCAT-Retentive-Timers

Retentive variants of the standard library PLC timers
MIT License
20 stars 3 forks source link

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

Open runtimevic opened 1 year ago

runtimevic commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

fisothemes commented 1 year ago

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 commented 1 year ago

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
fisothemes commented 1 year ago

bStart

runtimevic commented 1 year ago

bStart

fisothemes commented 1 year ago

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 commented 1 year ago
fisothemes commented 1 year ago

With. So bStart unfortunately

runtimevic commented 1 year ago

With. So bStart unfortunately

fisothemes commented 1 year ago

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 commented 1 year ago

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

bStart, bPause, tPreSet, bDone, tElapsed.

fisothemes commented 1 year ago

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 commented 1 year ago

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

runtimevic commented 1 year ago

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 commented 1 year ago

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

fisothemes commented 1 year ago

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 commented 1 year ago

Hello @fisothemes

You will tell me...

runtimevic commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.