Multi reading needs to be mutex lock

[erbilball]

During develop my application, I think I found a defect. It was about reading method from plc.

I have a plc connection instance. I am trying to read values from plc in different intervals with different tag groups.
Each my tag groups has different tags and they have different intervals.

For example, my first read attempt is with 500 plc tags in 500 ms . Other one is with 1000 plc tags in 1000 ms.
if I try these groups reading one by one, everything is good. But when I started both of them. Some time they were getting conflicted.
I mean during one of them was reading values, other one was trying to get values before the process was end. And when this case occurs a few plc tag values returns null.

See sample flowing of my application with conflict.
Reading of taggroup1 is started,
Reading of taggroup1 is ended.
Reading of taggroup2 is started,
Reading of taggroup2 is ended.

**Reading of taggroup1 is started,

Reading of taggroup2 is started, ( before taggroup1 reading process is ended.)**

Reading of taggroup1 is ended.
Reading of taggroup2 is ended.

As you see, they are started one after another in short time. In my opinion first one is started and it is not finished yet. And the other one was calling same code lines in read method.

I added "locking' to reading method. Now all is fine. I have never getting null value. Locking provides us to ensure that only one thread at a time executes a critical section of code at a time.

[ottowayi]

Thank you taking the time to work on this and I'm always appreciative when I get pull requests. Unfortunately, I'm not sure if I'll be able to merge this at this time. This does solve the issue for reads, but we'd also need to account for all of the other methods as well. Currently the drivers are not thread safe, you can think of them more as representing a single connection, and the connections are serial. Your use case here I believe is more about scheduling or timing the requests, so it may be better to implement a lock around the driver itself and not inside of it since you're sharing a single driver across multiple threads. Another way to do this would instead of sharing the driver across threads, create a driver in each thread. This is commonly done for performance and speed, because you can divide large groups of tags across multiple threads/connections. Usually there is no harm in opening multiple connections, but if you have old or overloaded controllers/network cards you may run into CIP connection limits. I only ran into this when trying to open ~10 connections at once to a L55 behind an ENBT also connected to an iFix SCADA. So something to be aware of, but usually not a concern.

If going with multiple drivers, there a couple things to consider. A LogixDriver by default automatically uploads the controller tag list (optionally program tags too), these tag definitions are required for a lot of features like AOI/UDT support and are why you don't need to worry about the number of tags you're reading/writing at a time like you do with other libraries. This convenience comes with a startup cost though and can take a bit of time if you have a lot of tags. To avoid this, you can initialize all of your tags once at the start and then pass them to the drivers in each thread. Roughly doing:

with LogixDriver('1.2.3.4') as plc_init:
    tags = plc_init.tags

# then pass `tags` into your thread
# create a new driver that does not initialize its tags
with LogixDriver('1.2.3.4', init_tags=False) as plc_thread:
    plc_thread._tags = tags   # set the tag definitions
    ... # and whatever reads you need to do

With this approach (by using the driver as a context manager) it will open and close a connection every time. If you are doing a lot of requests for a long time, you may want to handle opening and closing the driver yourself and not within a with block. The downside to this is that the driver does not do anything to maintain the connection, so a long period of time without any activity will result in the plc closing the connection. (This is true outside of the multi-threading anyway) To prevent the connection for timing out, I would periodically add a heartbeat command into the queue. The heartbeat could be really any request, so like reading the plc time or program name or even just any random tag would work.

I do like the idea of making the drivers thread safe though, it would make it easier for what you're trying to do. To accomplish this though we would need to decouple the connection aspect from the driver and rework a lot of the internals. But, good news on that front. I'm currently working on a complete rewrite of the library. There is a lot of the code that I'm not happy with, there's some pretty confusing and ugly sections hacked together to make it work. I've played around with some ideas on how to separate out the CIP and EtherNet/IP connection from the drivers already. Unfortunately, I don't have a timeline on this, but I'm almost done with rewriting the entire EIP and CIP protocol implementations.

Hopefully I've provided you with some useful information and getting feedback or assistance from users is greatly appreciated.
I work on this as a hobby and so knowing other people are using it helps keep me motivated to continue with it. And I'd like to be clear, I'm not saying no to this, just not yet.