Seperate Client and ClientAsync

[swamper123]

Make seperate Class ClientAsync, independent from Client class, like mentioned in #180

[gijzelaerr]

why duplicate the code? you can keep a client instance inside your async client and call the client methods from the async methods.

[swamper123]

Just to prevent more missunderstandings:

• Client class shall be untouched, inclusive its as_methods (not python async, S7 async, but not working well like mentioned in Add python asyncio wrapper around as_ methods #164 )
• ClientAsync class shall still contain an instance of Client
• but ClientAsync shall not overwrite the Client as_methods(? but how to be SOLID then, if they are now python async and S7 async?)

[swamper123]

If it is about the return values, yes they are None at the moment, because I didn't know what the best way would have been to say "This async request is timeouted", since the client version doesn't have such a thing as well.

What I found was the s7_client_Errors dict which contains different errors. And in the snap7 code it seems that this error is being raised in case of a timeout.

What I can do is replacing these Nones with something like:

if request_in_time in s7_client_Errors.keys():
    return s7_client_Errors[request_in_time]
return bytearray(data)

We could also create own error classes and raise them as well.

[swamper123]

What if we make an abstract Class ClientBase() with abstract methods and all identical methods?
Both classes, Client and ClientAsync inherit from that ClientBase class and so we are explicit SOLID and DRY?
(I would adjust this PR for that)

[gijzelaerr]

I don't see an advantage there, it just adds complexity. Why not just make a ClientAsync class with a Client instance as property, add your async methods to ClientAsync that call/wrap the Client methods. And if someone still needs to use the original methods they can just use ClientAsync.client.<method>

[swamper123]

1. The async Client methods in Client.py doesn't work propperly. They return a value, yes, but these values might be wrong, because there isn't any check made if the async request is done. It returns the value where the pointer points (the space where the async request result shall be safed). check_error() doesn't catch pending requests propperly, just picking up S7Exceptions which may be thrown. But if the Job is still pending is not checked. This is the reason why there are functions like WaitAsCompletion(), CheckAsCompletion(), and SetAsCallback() (see here in snap7 docs). Try it on some real devices (I tried it on a S7-1200) or let some users test it as well with let's say 10000 requests and see how many times the real value was returned. Running a PLC-Server on the same device may have another behaviour, but this doesn't represent reality.

2. And even if there would be such a check in the Client class, we would reach nothing. Yes we made an async request, but have to wait until we do something again (because this repo is just a wrapper around snap7 and can't handle something else, until a result was received). Async requests just make sense, when there is a programm which can handle such things (like programs with asyncio). Sync stuff can't do that. And even worse, it may run faulty or exploding regulary, if not handled propperly.

3. Yes, abstract classes may make the Code more complex and I don't think there will be more than two types of Client classes in the future. But such an abstract Baseclass solves the Problem of the Liskov stuff, which you mentioned in Add as area read write #180. Also the "BaseClient" would contain all methods which are identical and allows the "O" in SOLID without an "L" conflict and also it still stays DRY.

[swamper123]

Perhaps a second thought from @spreeker would be nice as well, what the best way would be.

[gijzelaerr]

I'm not sure what you are trying to accomplish here. This PR just removes a couple of calls from the client class.

I think @spreeker probably thinks the same, just keep the client class as a thin wrapper around all the lower-level library function calls. Then implement your async code in a separate module which calls the methods implemented in the client class. Keep all the async code in the async module.

I described this before, but maybe I was not clear. I don't want to introduce any backwards-incompatible changes and I also want to keep exposing the original lower-level functions (like client. as_ab_read).

As also said before, both @spreeker and I are not convinced that adding async support to python-snap7 is a huge win, but we are open to your contributions. if you manage to add async support in a backwards-compatible way, then why not. But this is becoming quite a time-consuming thing now, I'm doing this in my spare time which is sparse. You now have 3 pull requests open that are all not finished, they don't really improve the codebase. If you want to pick up issues (which I still encourage you to do) then do so but pick up one task at a time and try to understand our concerns also.

[swamper123]

Then implement your async code in a separate module which calls the methods implemented in the client class. Keep all the async code in the async module.

And I told you, that an async wrapper won't work, because the async methods in Client.py aren't working correctly.
I will not use them, because stuff should work the right way, not just work. Somebody has to touch this lines sooner or later or you have to expand them into a "as_db_write_with_checked_solution_via_aswaitascomplete(*args)" thing, which wouldn't be "wrapper-like".
As well dropping Python2 and start at Python3.6 again is a backwards incompatibility as well in my view, but this is another story.

I will try to explain the last time, why I won't wrap async methods from client.py here.

Example as_db_read:
This reseves just a space to write the value into it, which is fine.

    def as_db_read(self, db_number, start, size):
        """
        This is the asynchronous counterpart of Cli_DBRead.
        :returns: user buffer.
        """
        logger.debug(f"db_read, db_number:{db_number}, start:{start}, size:{size}")
        type_ = snap7.types.wordlen_to_ctypes[snap7.types.S7WLByte]
        data = (type_ * size)()

This kicks off an snap7 async request. The value in result is the value where self._pointer is pointing at the moment and contains an invalid result. Because nothing is waiting or checking the snap7-job here, the program continues, while the snap7 Thread/Job is running in the backround. This is the root of the runtime-error.

        result = (self._library.Cli_AsDBRead(self._pointer, db_number, start, size, byref(data)))

This method just checks, if a Snap7Error was thrown or not afaik. So either the Thread has been to slow and the invalid value in result was accpeted (because it isn't a S7Error), the program runs further.
If you debug this stuff , result perhaps contain the right value, which now will be checked correctly.

The invalid result may be accepted as plausible result (depending on what's inside) and returned to upper methods. After the program returns, you haven't got anything in your hand to fix that.

        check_error(result, context="client")
        return bytearray(data)

If I would use this method inside an async wrapper method, I had to deal with incorrect values. And I can't catch them.

I offered a solution/way to expand this into an async background with python-inbuilt stuff and it works and was merged into master. To use it in an intuitive way and to stay DRY I chose to keep the names and overwriting the symantic here. It may not be a good way in projects with higher complexity and even more hierarchies, but this isn't the case in python-snap7 and propably will never be (either a request is sync or async).
So who ever uses client_async should be aware of this overwritten schematic, since it is in an extra modul, with matching commentary. Also this was accepted so far.

While working on #180 you mentioned there:

I was thinking a bit about the async code inheriting and overriding methods for a while, and I'm not fully sure if it is actually a good idea if the return type changes since it violates the Liskov Substitution Principe:

https://en.m.wikipedia.org/wiki/Liskov_substitution_principle

Now I'm not really a fan of OO anyway, but maybe it is a good idea to stay SOLID if we use it

https://en.m.wikipedia.org/wiki/SOLID

So I had to stop in #180 and #178 (because I won't invest time and in the end I have to rework everything on a new base) and tried to provide solutions here in #186.

The intricacy about this is:

• keeping the same Method names, to be "wrapper-intuitive"
• making stuff async in an extra module so that this stuff is seperated from the original class
• making async methods working right(, which is archived via asyncio)
• being SOLID
• don't overwrite schematic

So in my opinion there aren't many ways:

• copy&pasta every Method from client.py to client_async.py
• using an abstract BaseClass was wanted, because it just adds complexity. (, but would have been a more SOLID way.)
• droping asyncio stuff and make a note in the docs that snap7 async stuff doesn't support correctness of s7-values (which would be the worst solution in my mind)

I can't imagine another way without violating one of these upper listed points.
I don't see a way that stuff works correctly in the end and everybody is happy with that.

Perhaps we are talking at cross purposes. Perhaps somebody else knows a better way. But as long as this "core-problem" isn't solved, I won't contribute async stuff anymore.

[gijzelaerr]

Now I think a bit longer about it, the issue with the current as function implementation in client seems to be that we try to interpret the data pointer as a bytearray too soon, the data is not ready yet. This is wrong in any case and should be modified. One solution would be to just return the pointer, and update the doc that this should be converted to a bytearray when the data is ready. That way you can also access the data, without duplicating the call.

[swamper123]

Correct.

And snap7 offers 3 solutions to check if the data is ready:

1. Cli_CheckAsCompletion() : You start the request and if you like you can do stuff after that. Later you request if the Job was done in and if not, you can still do something else. This is the solution I used in the async code, because it was the easiest way (and most intuitive). Also asyncio allows to do something else, what is more complex (or inefficient) in sync code.

2. Cli_WaitAsCompletion(): This would be an inefficient, but sync "standard way". Do something and then "idle wait" until the job is done. The difference to the sync job is, that you can execute stuff between kicking off the request and waiting. In case you want to be slim(so kicking off and then check directly after that if it is done), this would be the worst solution, because it would be the same behaviour like a sync request. So you could do a sync request anyway.

3. Cli_SetAsCallback(): You start the request and if it is done a callback will be executed. This would be the most efficient way, but a complex one (even with asyncio, this would be a small challange). I have an idea for that, but won't work on it atm.

These are your possibilities to check if the Job is done/the data is ready in a snap7-way. But this would cost your "just-wrapper" style, because a user should choose on their own which one of this 3 solutions they would like. And like I said, doing something else while waiting for the job to be done in a "sync-way" (using functions as methods) is either inefficient or complex. Not so with asyncio.

EDIT: But this is a thing which should be discussed in #164 while we are here still discussing about code style, the does and don'ts.

[gijzelaerr]

So these 3 calls should be implemented in snap7.client. Nothing fancy, just a simple call to the lib and doing the error handling, just like how the other methods are implemented. Then you can call these methods the way you like from your async module, combining them into something more useful.

[swamper123]

It seems that this is an ToDo, but for another PR, including tests (especially when pointers are returned, I don't know how the garbage collector handles them) which somebody else may contribute.

When that PR (fixing the as_checking methods) is through, you and @spreeker have to decide if:

• ClientAsync is even necessary anymore and it is allowed to add "async_tool_method_XYZ" to ClientClass

or

• if "async_tool_method_XYZ" should be stored in ClientAsync(Client): (Perhaps ClientAsyncio may be even a better name, to be more clear which way of async is implemented), where typical scenarios (like do_something while waiting) are realised as Coroutines. There wouldn't be stuff like async def as_db_read(...) anymore, because the User itself has to implement that in his async code itself and this library just offers async_tools to help him dealing with stuff. Perhaps with some examples.

Then I would change stuff as you have chosen and may contribute more async tool_stuff, if you like. Deal?

[gijzelaerr]

The garbage collector will not touch that object as long as there is a reference, which we are keeping.

I don't have the time nor the need to add those functions at the moment, so let's see if someone will implement them over time.