Python crashes after receiving a second notification

[kirlek]

After moving my code from Windows to Linux I get crashes after the second notification. Both running pyads 2.2.5

This is an example of code that works on Windows but crashes on Linux:

import pyads
from time import sleep
import os

if not os.name == 'nt': # On Linux
    pyads.open_port()
    adr = pyads.AmsAddr(AMS, pyads.PORT_SPS2)
    pyads.add_route(adr, IP)
    sleep(8)
    print(pyads.get_local_address())

plc = pyads.Connection(AMS, pyads.PORT_SPS2, IP)
plc.open()

@plc.notification(pyads.PLCTYPE_REAL)
def callback(handle, name, timestamp, value):
    print(
        '{0}: received new notitifiction for variable "{1}", value: {2}'
        .format(name, timestamp, value)
    )

handles = plc.add_device_notification('REALVALUE', pyads.NotificationAttrib(4), callback)
sleep(10)
plc.del_device_notification(handles[0], handles[1])

The output on Linux is:
REALVALUE: received new notitifiction for variable "2017-10-10 11:22:06.225441", value: 1368.362548828125
REALVALUE: received new notitifiction for variable "2017-10-10 11:22:06.230441", value: 1367.6650390625
python3: AdsLib/RingBuffer.h:71: void RingBuffer::Read(size_t): Assertion `n <= BytesAvailable()' failed.
Aborted (core dumped)

[dabrowne]

I think it's the firing of the third notification which is causing the crash. Does it always happen on the third one?

It looks like those notifications are happening in quick succession, does the crash still happen on notifications for variables which aren't updating so quickly?

[kirlek]

I tested with a variable that is updated about every 15 seconds.

Now it crashes when firing the second notification. (About 15 seconds after the first is printed.)

[stlehmann]

The Ringbuffer issue seems to happen with older versions of TwinCAT2. Which version do you use?

[kirlek]

From plc.read_device_info():
version: 2
revision: 10
build: 902

[stlehmann]

I have the same problem with a PLC running version 2.10 Build 1325. The device notification packet sent from the PLC is actually longer than it should be according to the length parameter. Could you do me a favor and log the device notification sent by the PLC with Wireshark and send it to me.

[kirlek]

I have sent you an email with the wireshark capture.

[stlehmann]

@kirlek Thanks for the capture. It is exactly as I expected. There are some additional 25 Bytes at the end of the Device Notification that should not be there. I suspect that the old Twincat protocol somehow differed from the current one. This now leads to the Ringbuffer issue.

[kirlek]

Is this something that needs to be fixed in https://github.com/Beckhoff/ADS ?

[stlehmann]

I think so, yes. I have already taken a look at the source code but I'm actually not that good with C++ so that might be something for the Beckhoff guys.

[dabrowne]

@pbruenn Have you encountered an issue like this before with older TwinCat versions?

[pbruenn]

No, but it sounds like we need to harden the AdsLib.
From what I understand AdsLib crashes because of full buffers, if you send TCP packages which contain more bytes than expected from the AmsHeader length.
Could you open an issue in the upstream repository with a small wireshark capture attached?
I will try to prepare some test case for this. But be warned, this kind of issue will take some time to fix, as I think we need to rethink the whole receive path.
Regards, Patrick

[pbruenn]

Thanks for the Wireshark. From what I can see there. The AMS Headers look good. Just the length in the NotificationHeader is somehow wrong. A patch should be simple so I pushed a version online:
https://github.com/Beckhoff/ADS/tree/dev-fix-broken-notifications-v1

However simulating the error is much more difficult so I wasn't able to test with bad packages, yet.
I will work on that, but for the meantime I thought I show you my patch so you can test it, in case you find time for that.

[stlehmann]

I am able to simulate the error with the Python ADS testserver. I will do this asap and come back to you.

[kirlek]

I can confirm that https://github.com/Beckhoff/ADS/tree/dev-fix-broken-notifications-v1 works for my use case when applied to https://github.com/dabrowne/ADS.

Thank you!

[stlehmann]

Yes, confirm. The fix works well.

[stlehmann]

@pbruenn Thanks for the fast help here.

[pbruenn]

The pleasure is all mine, it's really fun to work with all of you to keep improving ADS. I will have a closer look on your Python ADS testserver, too ;-).

The fix is now on master: Beckhoff/ADS@82e2ecf