dmroeder / pylogix

Read/Write data from Allen Bradley Compact/Control Logix PLC's
Apache License 2.0
599 stars 182 forks source link

Return data gets jumbled up on a multi write when the tags don't exist. #224

Closed Destination2Unknown closed 1 year ago

Destination2Unknown commented 1 year ago

Code

from pylogix import PLC

with PLC() as comm:
    comm.IPAddress = "192.168.1.9"

    write_data = [("tag1", 100), ("tag2", 6.45), ("tag3", True)]

    # write the values
    ret = comm.Write(write_data)

    # print the status of the writes
    for r in ret:
        print(r.TagName, r.Status)

Prints:

tag2 Unknown error timed out
6.45 Unknown error timed out

Versions

dmroeder commented 1 year ago

I have a fix for this.

Destination2Unknown commented 1 year ago

@dmroeder Sweet!

Destination2Unknown commented 1 year ago

I think this was the issue:

https://github.com/dmroeder/pylogix/blob/95d6bc9413cb5d7f54eb231a802dd9f2d3950e08/pylogix/eip.py#L435-L436

It should be something like:

if not conn[0]:
    return [Response(t[0], None, conn[1]) for t in tags]
dmroeder commented 1 year ago

You mind testing the bugfix/multi-write branch when you get time? It works for me, though things weren't quite failing in the same way for me.

Destination2Unknown commented 1 year ago

Pretty much sorted, only case where it didn't work was where the tags don't exist and there is no connection to the PLC (i.e. if the IP Address is incorrect):

from pylogix import PLC

with PLC() as comm:
    comm.IPAddress = "192.168.133.199"

    write_data = [("tag1", 100), ("tag2", 6.45), ("tag3", True)]

    # write the values
    ret = comm.Write(write_data)

    # print the status of the writes
    for r in ret:
        print(r.TagName, r.Status)

prints:

tag2 Unknown error timed out
6.45 Unknown error timed out

https://github.com/dmroeder/pylogix/issues/224#issuecomment-1540346196

Destination2Unknown commented 1 year ago

This fairly niche example isn't quite right either...

write_data = [("myDint", 100), ("myString", "Hello"), ("myDint.1", True)]

# write the values
ret = comm.Write(write_data)

# print the status of the writes
for r in ret:
    print(r.TagName, r.Value, r.Status)

prints:

myDint 100 Success
myString Hello Success
myDint.1 (2, -1) Success
dmroeder commented 1 year ago

Yeah, that is weird. The latest should fix it.

Destination2Unknown commented 1 year ago

Yeah, that is weird. The latest should fix it.

This case is still incorrect:

from pylogix import PLC

with PLC() as comm:
    comm.IPAddress = "192.168.133.199" #incorrect IP address

    write_data = [("tag1", 100), ("tag2", 6.45), ("tag3", True)]

    # write the values
    ret = comm.Write(write_data)

    # print the status of the writes
    for r in ret:
        print(r.TagName, r.Status)

prints:

tag2 Unknown error timed out
6.45 Unknown error timed out
dmroeder commented 1 year ago

I see, somehow, I missed that comment originally

dmroeder commented 1 year ago

Okay, that one should be fixed on that branch too.

I considered whether to return the value that was written or the None. I went with value intended to write because that is what would be returned on a successful write. Though I could be convinced otherwise.

Destination2Unknown commented 1 year ago

Okay, that one should be fixed on that branch too.

I considered whether to return the value that was written or the None. I went with value intended to write because that is what would be returned on a successful write. Though I could be convinced otherwise.

I strongly favour returning None because it would make it crystal clear that there was an issue. Otherwise, it could lead to confusion when a write value is returned despite not being sent successfully.

Thank you for your assistance. This library is exceptional, and the support provided is truly outstanding!

dmroeder commented 1 year ago

Sold! I'll return None. I appreciate your feedback and testing.

Destination2Unknown commented 1 year ago

Looks good to me, thanks again!