stlehmann / pyads

Python wrapper for TwinCAT ADS
MIT License
254 stars 94 forks source link

Notifications for structures not working #284

Closed mmachenry closed 2 years ago

mmachenry commented 2 years ago

Edit: see comments. This is more than just a documentation issue.

This is potentially a minor point, and I'd submit a PR to fix it, but I actually can't figure out how to do this without the documentation. On the docs here: https://pyads.readthedocs.io/en/latest/documentation/connection.html we see this program.

structure_def = (
    ('rVar', pyads.PLCTYPE_LREAL, 1),
    ('rVar2', pyads.PLCTYPE_REAL, 1),
    ('iVar', pyads.PLCTYPE_INT, 1),
    ('iVar2', pyads.PLCTYPE_DINT, 3),
    ('sVar', pyads.PLCTYPE_STRING, 1))
size_of_struct = pyads.size_of_structure(structure_def)
@plc.notification(size_of_struct)
def callback(handle, name, timestamp, value):
    values = pyads.dict_from_bytes(value, structure_def)
    print(values)
attr = pyads.NotificationAttrib(ctypes.sizeof(size_of_struct))
plc.add_device_notification('global.sample_structure', attr, callback)

This does not give the open shown in the docs but instead crashes with

    attr = pyads.NotificationAttrib(ctypes.sizeof(size_of_struct))
TypeError: this type has no size

Now that error makes perfect sense to me I guess. ctypes.sizeof is expecting a type not size of a type. But how does one make a type from structure_def? It's supposed to be a subclass of ctypes.Structure. That class uses a different but similar type for defining it's fields than structure_def. Is there a converter function to support this? Am I meant to create a structure_def and then also create my own instance of ctypes.Structure for that structure? I feel like that would be a lot of maintenance and there's a probably a better way to do this but I haven't found it from surfing around in your code.

Furthermore, is there a way to read the structure definition from the PLC rather than defining all of my structure types in Python? I believe the .so C library implements this as ADSIGRP_SYM_DT_UPLOAD which gets a structure definition but I see no reference to this in the pyads library.

Thanks a bunch.

mmachenry commented 2 years ago

By the way, I have tried an entirely different approach as well.

def structure_def_to_ctypes_structure(structure_def):
    fields = []
    for (field_name, field_type, array_size) in structure_def:
        if array_size == 1:
            fields.append((field_name, field_type))
        else:
            fields.append((field_name, field_type * array_size))
    class NewStruct (ctypes.Structure):
        _fields_ = fields
    return NewStruct

with pyads.Connection(plc_net_id, pyads.PORT_TC3PLC1, plc_ip) as plc:
    def callback(notification, data):
        handle, timestamp, value = plc.parse_notification(
        notification, structure_def_to_ctypes_structure(nctoplc_axis_ref))
        values = pyads.dict_from_bytes(value, nctoplc_axis_ref)
        print(values)

    attr = pyads.NotificationAttrib(length=pyads.ads.size_of_structure(nctoplc_axis_ref))
    plc.add_device_notification('Mycode.Mymotor.axis.NcToPlc', attr, callback)
    while True: pass

I can't really understand how the notification decorator works at all, honestly. I looked into the code and it calls parse_notification, which requires a type, but the decorator only takes the size of the type. So I'm not sure where it gets the full type from. So I tried this which gives me the ability to call parse_notification directly. This approach works just fine with any simple PLC types. However, with the structure I get:

TypeError: 'NewStruct' object is not subscriptable

The function seems to be expecting a PLCTYPE or a ctypes.Structure and that's what I've passed it. Rather than the size of a type which seems to be what the decorator gives it. But it's now trying to subscript the Structure and that doesn't work.

Clearly I'm discussing a bunch of different ways I've tried to solve the same problem and these aren't necessarily all one cohesive bug report. I just figured while we're discussing it I could raise these questions and maybe get some insight on it. But I think the take away point is that the docs for what I'm trying to do are off and an answer to how to get any of these methods to work would be a good fix to the documentation.

chrisbeardy commented 2 years ago

ok i've been doing some testing, this is working with version 3.2.2 and stopped working with 3.3.0.

you are essentially using it correctly but a change has clearly caused this bug to creep in. We need to fix this and then see what we can do to prevent it happening again.

I will look into fixing this issue. In the meantime perhaps if you can, change the issue name to something like "Notifications for structures not working" as It's cleary more than just the docs that are wrong!

If you need this feature badly and can do without the ones added in >=3.3 then I'd use that version otherwise you will have to implement a workaround for now, sorry.

To answer a further question, unfortuanetly you can not get the structure definition from the PLC at the moment, I'm not sure this is supported over ADS. The best you can do now is look at #229. ADSIGRP_SYM_DT_UPLOAD I think does provide more info but I'm not sure what it does give you, could you take a look, I'm not sure what the subItems is? All the Beckhoff docs in for example C# still makes you define your structure in advance.

mmachenry commented 2 years ago

Thanks so much. I'll downgrade right now, change the name of this issue, and look forward to an update.

chrisbeardy commented 2 years ago

so in 3.3 the return type of size_of_structure was changed. it now returns an int and not a ctype. Therefore if you change the code to look like this instead:

structure_def = (
    ('rVar', pyads.PLCTYPE_LREAL, 1),
    ('rVar2', pyads.PLCTYPE_REAL, 1),
    ('iVar', pyads.PLCTYPE_INT, 1),
    ('iVar2', pyads.PLCTYPE_DINT, 3),
    ('sVar', pyads.PLCTYPE_STRING, 1))

size_of_struct = pyads.size_of_structure(structure_def)

@plc.notification(ctypes.c_ubyte * size_of_struct)
def callback(handle, name, timestamp, value):
    values = pyads.dict_from_bytes(value, structure_def)
    print(values)

attr = pyads.NotificationAttrib(size_of_struct)
plc.add_device_notification('MAIN.structure', attr, callback)

it no longer crashes and does read, however there seems to be a another issue where its not reading DINTS correctly now when reading by structure....

chrisbeardy commented 2 years ago

actually that was me forgetting to make the DUT pack_mode := 1. It now works as expected. I will update the docs.

@mmachenry if you use the aove example with the latest pyads version it should work