FreeOpcUa / opcua-asyncio

OPC UA library for python >= 3.7
GNU Lesser General Public License v3.0
1.11k stars 358 forks source link

Add support for coroutine functions as attribute value setters #1571

Closed FMeinicke closed 7 months ago

FMeinicke commented 7 months ago

The previous implementation only allowed sync functions to be registered as an attribute value setter. This limits the setter function in a way such that it cannot read from other nodes using Node.read_value(), for example. Allowing coroutine functions to be set as attribute value setters enables these functions to also use coroutine functions themselves.

AndreasHeine commented 7 months ago

i remember in the past that writing should be sync in order to be consistent!? long time ago... @oroulet do you remember?

oroulet commented 7 months ago

yes there have been a MR before. It is a long time ago, but problem is that allowing async here allows for potential race conditions. We want to ensure writting and notification and all related things are done sync, before anyone else can come and mess up things

FMeinicke commented 7 months ago

Ah, I see. I didn't think about possible race conditions with async functions in the write function, to be honest. But I do understand your concerns.

Do you have any suggestions on how I could read from other nodes in a setter function without making the setter async? Just as a simplified, contrived example:

def setter(node_data: NodeData, attr: ua.AttributeIds, value: ua.DataValue) -> None:
    some_other_node = await server.nodes.objects.get_child()  #: not possible in sync function
    some_other_value = await some_other_node.read_value()  #: not possible in sync function
    if value.Value.Value > some_other_value.Value.Value:
        raise ua.userrors.BadOutOfRange()
    node_data.attributes[attr].value = value
AndreasHeine commented 7 months ago

you mean like the range from a AnalogItem?

oroulet commented 7 months ago

good questions.. these are anyway really high level method and should not be used there. There is no reason read is async at that level. first you should cacje the nodeid, then Iyou have a method called "read_attribute_value" which is sync

FMeinicke commented 7 months ago

Thank you for your suggestions. I'll try to make it work that way instead.

AndreasHeine commented 7 months ago

guess via nodeid as part of the setter:

def setter(node_data: NodeData, attr: ua.AttributeIds, value: ua.DataValue) -> None:
    range_high = "ns=1;i=1000"
    range_low = "ns=1;i=1001"

    # we need to access servers addressspace (as long as we read only it would be ok)
    high_data = server.iserver.aspace.get(range_high)
    low_data = server.iserver.aspace.get(range_low)

    # whatever you want to do below

    if value.Value.Value > some_other_value.Value.Value:
        raise ua.userrors.BadOutOfRange()
    node_data.attributes[attr].value = value

maybe a wrapper class for those variable types would be a good idea to implement the semantics

oroulet commented 7 months ago
# we need to access servers addressspace (as long as we read only it would be ok)
    high_data = server.iserver.aspace.get(range_high)
    low_data = server.iserver.aspace.get(range_low)
Should be ok to write as long as you are using syn calls
FMeinicke commented 7 months ago

Is there a way to write synchronously? Server.write_attribute_value() and Node.write_value() are both async, FWIW.

Or do you mean that I'd have to use the sync submodule API in this case?

oroulet commented 7 months ago

fancy.... and when you look at code, there is zero reason for them to be async, they do not await anything. InternalServer.write_attribute_value must be async for compatibiity with client version but you can try to make a MR to make that method sync in AddressSpace, then you can call it directly..

FMeinicke commented 7 months ago

and when you look at code, there is zero reason for them to be async, they do not await anything.

As I see it, there is a reason for AddressSpace.write_attribute_value() to be async: It's because it calls the AttributeValue's datachange_callbacks which are async:

https://github.com/FreeOpcUa/opcua-asyncio/blob/8cf8682a019bacc7e67c42fec72531bf3e87c31e/asyncua/server/address_space.py#L800-L804

So, I don't think there is an easy way to make AddressSpace.write_attribute_value() sync. Or am I missing something here?

oroulet commented 7 months ago

arrg did not see that one. I am not sure how to change that. . but is writing in another node a good idea when writing into a node? on server side you should better use subscription and react to a value change to change another node value. While reading to ensure some value validatino has a clear use case for that interface

FMeinicke commented 7 months ago

on server side you should better use subscription and react to a value change to change another node value.

Very good point. I somehow didn't think of using subscriptions for this use case. I agree that this is indeed the better way instead of writing to another node from within the value setter of a node.