search

runtimevic / OOP-IEC61131-3--Curso-Youtube

Programación Orientada a Objetos OOP IEC61131-3 PLC Curso Youtube Runtimevic
https://runtimevic.github.io/OOP-IEC61131-3--Curso-Youtube/
48 stars 17 forks source link

Memory exception calling .Draw() on deleted clonedCircle. #13

Closed benhar-dev closed 7 months ago

benhar-dev commented 8 months ago

https://github.com/runtimevic/OOP-IEC61131-3--Curso-Youtube/blob/135816b26bd56955a013b2c7cc9468f983c5ea50/TC3_Design_Patterns/Prototype/Prototype/Circle.TcPOU#L30

There is a error with this line. You should not delete the object before returning from the clone method. The receiver will receive an invalid object and will raise a memory exception if they try to use it. The code which calls Clone() should be responsible for the disposing of the object later. Ideally with clonedCircle.Dispose();

You can see the bug if you step through the code, rather than letting it run.

runtimevic commented 8 months ago

Hi @benhar-dev , It's great that you look at it and can contribute, you are welcome... Design_Pattern_Creational_Prototype_FB_Circle_Method_Clone

benhar-dev commented 7 months ago

Answering your questions about variables first,

VAR_INST.

VAR_INST inside of a method call is the same as putting the variable in a VAR in the Function Block. Method variables are destroyed at the end of a method call, where as Function Block variables are destroyed only when the Function Block is destroyed.

I personally do not use VAR_INST when doing OOP programming, as I feel that VAR in the body of the function block is easier to read and understand.

VAR_STAT

Static variables. As we know, Function blocks (and Classes) instantiate and hold a collection of variable for themselves. As an example, we can have many TON function blocks which all run independent of each other as their variables are separate.

If you declare a variable as VAR_STAT, you make it static. At that point, all function blocks of the same type will share this variable. If one of the instantiated function blocks writes to it, all of the other function blocks of the same type will see this change. Its a common variable among instances. So. VAR is local, only accessible by the function block. VAR_STAT is common across all instances of a function block.

benhar-dev commented 7 months ago

Answering your question about __Delete and could we change the variable so it wouldn't live

A pointer is just an address to an object. If I use __NEW on a function block, it will create that object in memory, and will then update your pointer to point to it.

If you call __Delete on a pointer, it will find the object in memory and mark it for deletion. This deletion may not happen immediately, you have simply told the system you no longer want that object.

The object does not live inside the pointer, a point is just a variable which holds the address of an object, which you can then use ^ to make the pointer appear to be the object it points to. (called dereferencing)

As an example, if I were to do this...

myPointer := __NEW(Heater); // the heater object would be made, and the address of the heater object would go in myPointer
myPointer := 0; // I have just reset the pointer.  The heater object **still exists** in memory.  I've just overwritten my only way to access it

So, the pointer is not the object, only a special variable which holds the address of an object and can also pretend to be the object by using the ^ to dereference it.

The order of events should be..

  1. Create an object using __NEW
  2. Use an object
  3. Mark an an object for deletion once you are finished using __DELETE
  4. (at some point the system will delete the object, most of the time it happens between PLC cycles)

The system will delete objects marked for deletion and we do not fully know (or care) when this will happen as we are already finished with it. This is a system level thing which is done for us.

In your code you are doing the following

  1. Make the circle
  2. Delete the circle
  3. Use the circle <- Here you are using a circle which has already been marked for deletion. Here be danger.
  4. System removes the circle

You can see the bug if you set a breakpoint in your code and single step through it. When single stepping through the code, you will see that they system has already removed your object before you have a chance to use it.

Instead you should remove __DELETE from your clone method. Just return the new object as ITF_Shape as you are currently doing.

You must then implement a way to delete the object. My suggestion is to make ITF_Shape have a .Dispose() method. This way, after you finish drawing the circle you can call .Dispose(). I.e. you mark it for deletion only after you have finished using it.

The circle's dispose method should contain

METHOD Dispose

__DELETE(THIS);
runtimevic commented 7 months ago

Hello @benhar-dev , If it's not too much work for you, I would be grateful if you fork the project and fix it as you think and then make me a PR pull request to see the solution..., if not I'll fix it...

benhar-dev commented 7 months ago

Thank you for considering me for this task. However, I’m currently not in a position to directly contribute to the codebase.

runtimevic commented 7 months ago

Hi @benhar-dev , I have documented this issue #13 in the documentation:

If you want to contribute something else?... When I have time I will fix the error in the Prototype Design Pattern thanks, Víctor.

runtimevic commented 7 months ago

Hi @benhar-dev , If you can look at the solution I would be grateful... https://github.com/runtimevic/OOP-IEC61131-3--Curso-Youtube/blob/master/TC3_Design_Patterns/Prototype/Prototype.tpzip