Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server support for read array with index range #636

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions asyncua/common/ua_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Usefull method and classes not belonging anywhere and depending on asyncua library
"""

from typing import Tuple
import uuid
import logging
from datetime import datetime
Expand All @@ -14,6 +15,26 @@
logger = logging.getLogger('__name__')


def ua_index_range_to_list_slice(ua_index_range:str) -> Tuple[int,int]:
"""Converts an OPC UA NumericRange into a python tuple for list slicing
see: https://reference.opcfoundation.org/v104/Core/docs/Part4/7.22/
Args:
ua_index_range (str): The OPC UA NumericRange

Returns:
Tuple[int,int]: Equivalent python list slicing tuple with low and high bounds
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good method

index_range_list = ua_index_range.split(':')
low_idx = int(index_range_list[0])
if len(index_range_list) == 1:
high_idx = int(index_range_list[0])+1
elif len(index_range_list) == 2:
high_idx = int(index_range_list[1])+1 # python List[n:n] -> empty list, List[n:n+k] -> list of k elements; OPC UA see https://reference.opcfoundation.org/v104/Core/docs/Part4/7.22/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do nt write so long lines. split it. And also I am sure linters complain about nothaving 2 spaces before #

else:
raise Exception(f'Read with bad index range: {ua_index_range}')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thin ValueError is the correct excpetino to use in this case. Unless UA defines one for such case

return (low_idx,high_idx)


def value_to_datavalue(val, varianttype=None):
"""
convert anyting to a DataValue using varianttype
Expand Down
57 changes: 50 additions & 7 deletions asyncua/server/address_space.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import asyncio
import typing
from typing import Tuple
from asyncua.ua.uatypes import DataValue, NodeId, StatusCode, Variant
import pickle
import shelve
import logging
Expand Down Expand Up @@ -47,7 +50,7 @@ def read(self, params):
# self.logger.debug("read %s", params)
res = []
for readvalue in params.NodesToRead:
res.append(self._aspace.read_attribute_value(readvalue.NodeId, readvalue.AttributeId))
res.append(self._aspace.read_attribute_value(readvalue.NodeId, readvalue.AttributeId, readvalue.IndexRange))
return res

async def write(self, params, user=User(role=UserRole.Admin)):
Expand Down Expand Up @@ -686,7 +689,18 @@ def __len__(self):

self._nodes = LazyLoadingDict(shelve.open(path, "r"))

def read_attribute_value(self, nodeid, attr):
def read_attribute_value(self, nodeid:NodeId, attr:ua.AttributeIds, index_range: typing.Optional[Tuple[int,int]] = None) -> DataValue:
"""Reads the Value of the requested Attribute of the given Node ID.

Args:
nodeid (NodeId): The NodeID from which the Attribute's value is to be read
attr (ua.AttributeIds): The attribute that is to be read
index_range (typing.Optional[Tuple[int,int]], optional): In case the Attribute's value is an array,
the range of the array that is actual to be returned. Defaults to None.

Returns:
DataValue: Value of the requested Attribute on the specified NodeID
"""
# self.logger.debug("get attr val: %s %s", nodeid, attr)
if nodeid not in self._nodes:
dv = ua.DataValue(StatusCode_=ua.StatusCode(ua.StatusCodes.BadNodeIdUnknown))
Expand All @@ -696,11 +710,35 @@ def read_attribute_value(self, nodeid, attr):
dv = ua.DataValue(StatusCode_=ua.StatusCode(ua.StatusCodes.BadAttributeIdInvalid))
return dv
attval = node.attributes[attr]
value = ua.DataValue()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also it looks like the logic is more complicated than necessary. That local variable is useless.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had a second look. That variable is really completely useless. It is ALWAYS overwritten in one of the next 4 lines

if attval.value_callback:
return attval.value_callback()
return attval.value

async def write_attribute_value(self, nodeid, attr, value):
value = attval.value_callback()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just return here insteas of making a local variable

else:
value = attval.value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

if not index_range is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if index_range is not None if the advices syntax.
But I am rather sure you could write: if index_range

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and that test should be ealier, so you do not need to overwrite a local value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact IMO if we don't make the test here (after knowing if the value is read via attval.value_callback or via attval.value) it makes the implementation more complex.

The check if just a range is wanted or full array wanted relies on having the full array already, specially because I don't understand when attval.value_callback() is called and how does it work; as far as I can see it is not used, I couldn't find any location on the code where it is set. If you or someone can confirm that it can be removed, than the code can be simplified.

So I think this implementation is safer, but eventually worst performing (we can make some lambdas for trying to minimize number of copies, but...). In case value_callback is used, in order to make the check before we would need to change the value_callback so it could also take a range.

Other option is to sacrifice performance only when the value is obtained via value_callback and having some code redundancy for range (maybe mitigated with some helper function).

What do you recon to be the best alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oroulet I have some interest in having this feature fixed 😸
Could you help me getting this PR in shape to get merged?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix that one: if index_range is not None is the advices syntax.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you help me getting this PR in shape to get merged?

fix the comments from today and it might be good enough

if not attval.value.Value.is_array:
return ua.StatusCode(ua.StatusCodes.BadWriteNotSupported)
try:
value = DataValue(Variant(value.Value.Value[index_range[0]:index_range[1]]))
except Exception as ex:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catching exception is usually a bad idea. In that case there are two things that can happen: the value is not an array or indexes are out of bound. Only catch these two things

self.logger.warning(f'Read index range failed. IndexRange = {index_range}; Ex: {ex}')
dv = ua.DataValue(StatusCode_=ua.StatusCode(ua.StatusCodes.BadIndexRangeInvalid))
return dv
return value

async def write_attribute_value(self, nodeid:NodeId, attr:ua.AttributeIds, value:DataValue, index_range: typing.Optional[Tuple[int,int]] = None) -> StatusCode:
"""Writes the Value of the requested Attribute of the given Node ID

Args:
nodeid (NodeId): The NodeID from which the Attribute's value is to be written
attr (ua.AttributeIds): The attribute that is to be written
value (DataValue): Value to write
index_range (typing.Optional[Tuple[int,int]], optional): In case the Attribute's value is an array,
the range of the array that is actual to be written. Defaults to None.

Returns:
StatusCode: StatusCode of the write operation
"""
# self.logger.debug("set attr val: %s %s %s", nodeid, attr, value)
node = self._nodes.get(nodeid, None)
if node is None:
Expand All @@ -713,7 +751,12 @@ async def write_attribute_value(self, nodeid, attr, value):
return ua.StatusCode(ua.StatusCodes.BadTypeMismatch)

old = attval.value
attval.value = value
if index_range is None:
attval.value = value
else:
if not attval.value.Value.is_array:
return ua.StatusCode(ua.StatusCodes.BadWriteNotSupported)
attval.value.Value.Value[index_range[0]:index_range[1]] = value.Value.Value
cbs = []
# only send call callback when a value or status code change has happened
if (old.Value != value.Value) or (old.StatusCode != value.StatusCode):
Expand Down
30 changes: 24 additions & 6 deletions asyncua/server/internal_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@
"""

import asyncio
from asyncua.ua.uatypes import DataValue, NodeId, StatusCode
from asyncua.common.ua_utils import ua_index_range_to_list_slice
from datetime import datetime, timedelta
from copy import copy
from struct import unpack_from
import os
import logging
from urllib.parse import urlparse
from typing import Coroutine
from typing import Coroutine, Optional, Tuple

from asyncua import ua
from .user_managers import PermissiveUserManager, UserManager
Expand Down Expand Up @@ -292,18 +294,34 @@ def unsubscribe_server_callback(self, event, handle):
"""
self.callback_service.removeListener(event, handle)

async def write_attribute_value(self, nodeid, datavalue, attr=ua.AttributeIds.Value):
async def write_attribute_value(self, nodeid, datavalue, attr=ua.AttributeIds.Value, index_range:Optional[str]=None)->StatusCode:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, this is is supposed to be an efficient method. use indexes

"""
directly write datavalue to the Attribute, bypassing some checks and structure creation
so it is a little faster
"""
await self.aspace.write_attribute_value(nodeid, attr, datavalue)

def read_attribute_value(self, nodeid, attr=ua.AttributeIds.Value):
if not index_range is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here: if index_range is not None

try:
py_list_index_range = ua_index_range_to_list_slice(index_range)
except Exception as ex:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably possible to only catch ValueError here

self.logger.warning(f'Parse index range failed. IndexRange = {index_range}; Ex: {ex}')
return ua.StatusCode(ua.StatusCodes.BadIndexRangeInvalid)
else:
py_list_index_range = None
return await self.aspace.write_attribute_value(nodeid, attr, datavalue, py_list_index_range)

def read_attribute_value(self, nodeid:NodeId, attr=ua.AttributeIds.Value, index_range:Optional[str]=None)->DataValue:
"""
directly read datavalue of the Attribute
"""
return self.aspace.read_attribute_value(nodeid, attr)
if not index_range is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

try:
py_list_index_range = ua_index_range_to_list_slice(index_range)
except Exception as ex:
self.logger.warning(f'Parse index range failed. IndexRange = {index_range}; Ex: {ex}')
return ua.DataValue(StatusCode_=ua.StatusCode(ua.StatusCodes.BadIndexRangeInvalid))
else:
py_list_index_range = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also that code is exactly the same in both read and write, put it in an internal method

return self.aspace.read_attribute_value(nodeid, attr, py_list_index_range)

def set_user_manager(self, user_manager):
"""
Expand Down
62 changes: 57 additions & 5 deletions asyncua/server/internal_session.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
from asyncua.ua.uaprotocol_auto import ReadParameters, WriteParameters, WriteValue
from asyncua.ua.uatypes import DataValue, NodeId, StatusCode
from asyncua.ua.attribute_ids import AttributeIds
import logging
from enum import Enum
from typing import Coroutine, Iterable, Optional
from typing import Coroutine, Iterable, List, Optional

from asyncua import ua
from ..common.callback import CallbackType, ServerItemCallback
Expand Down Expand Up @@ -102,29 +105,78 @@ def activate_session(self, params, peer_certificate):
self.logger.info("Activated internal session %s for user %s", self.name, self.user)
return result

async def read(self, params):
async def read(self, params:ReadParameters)->List[DataValue]:
"""Reads a set of nodes to read

Args:
params (ReadParameters): Parameters with nodes to be read

Returns:
List[DataValue]: List of values read
"""
if self.user is None:
user = User()
else:
user = self.user
await self.iserver.callback_service.dispatch(CallbackType.PreRead,
ServerItemCallback(params, None, user))
results = self.iserver.attribute_service.read(params)
results = [
self.iserver.read_attribute_value(node_to_read.NodeId, node_to_read.AttributeId, node_to_read.IndexRange)
for node_to_read in params.NodesToRead
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you change passing all arguments to passing them one by one? you are proably taking away some arguments, and the loop was probably anyway inside the read() method in iserver

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. might have been a misundersting, maybe you did all of that to move out the string parsing. your string parsing function is fine, if the UA attribute uses a string we have no choice but pass it dow to address_space.py and do the parsing there if we have to

await self.iserver.callback_service.dispatch(CallbackType.PostRead,
ServerItemCallback(params, results, user))
return results

async def history_read(self, params) -> Coroutine:
return await self.iserver.history_manager.read_history(params)

async def write(self, params):
def check_user_access_to_node_to_write(self, user:User, node_to_write:WriteValue)->bool:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is an private method(not used anywhere else) then it should start with underscore

"""Checks if the given user has the access permissions for the WriteValue

Args:
user (User): The user making the access request
node_to_write (WriteValue): Value to write

Returns:
bool: True when the user has the permissions
"""
if user.role != UserRole.Admin:
if node_to_write.AttributeId != ua.AttributeIds.Value:
return False
al = self.iserver.read_attribute_value(node_to_write.NodeId, ua.AttributeIds.AccessLevel)
ual = self.iserver.read_attribute_value(node_to_write.NodeId, ua.AttributeIds.UserAccessLevel)
if (
not al.StatusCode.is_good()
or not ua.ua_binary.test_bit(al.Value.Value, ua.AccessLevel.CurrentWrite)
or not ua.ua_binary.test_bit(ual.Value.Value, ua.AccessLevel.CurrentWrite)
):
return False
return True

async def write(self, params:WriteParameters)->List[StatusCode]:
"""Writes a set of Nodes to write

Args:
params (WriteParameters): Parameters with nodes to be written

Returns:
List[StatusCode]: Status codes of the write operation of each of the nodes
"""
if self.user is None:
user = User()
else:
user = self.user
await self.iserver.callback_service.dispatch(CallbackType.PreWrite,
ServerItemCallback(params, None, user))
write_result = await self.iserver.attribute_service.write(params, user=user)
write_result = []
for node_to_write in params.NodesToWrite:
if not self.check_user_access_to_node_to_write(user, node_to_write):
write_result.append(ua.StatusCode(ua.StatusCodes.BadUserAccessDenied))
else:
write_result.append(
await self.iserver.write_attribute_value(node_to_write.NodeId, node_to_write.Value, node_to_write.AttributeId, node_to_write.IndexRange)
)
await self.iserver.callback_service.dispatch(CallbackType.PostWrite,
ServerItemCallback(params, write_result, user))
return write_result
Expand Down
9 changes: 5 additions & 4 deletions asyncua/server/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"""

import asyncio
from asyncua.ua.uatypes import DataValue, StatusCode
import logging
from datetime import timedelta, datetime
from urllib.parse import urlparse
Expand Down Expand Up @@ -663,15 +664,15 @@ async def load_enums(self) -> Coroutine:
_logger.warning("Deprecated since spec 1.04, call load_data_type_definitions")
return await load_enums(self)

async def write_attribute_value(self, nodeid, datavalue, attr=ua.AttributeIds.Value):
async def write_attribute_value(self, nodeid, datavalue, attr=ua.AttributeIds.Value, index_range:Optional[str]=None)->StatusCode:
Copy link
Member

@oroulet oroulet Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is supposed to be the super efficitent server side write method, there we should use python indexes, not string that need to be parsed

"""
directly write datavalue to the Attribute, bypasing some checks and structure creation
so it is a little faster
"""
return await self.iserver.write_attribute_value(nodeid, datavalue, attr)
return await self.iserver.write_attribute_value(nodeid, datavalue, attr, index_range)

def read_attribute_value(self, nodeid, attr=ua.AttributeIds.Value):
def read_attribute_value(self, nodeid, attr=ua.AttributeIds.Value, index_range:Optional[str]=None)->DataValue:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here. string parsing is slow

"""
directly read datavalue of the Attribute
"""
return self.iserver.read_attribute_value(nodeid, attr)
return self.iserver.read_attribute_value(nodeid, attr, index_range)
77 changes: 75 additions & 2 deletions tests/test_client.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@

from asyncua.ua.uatypes import DataValue, Variant
import logging
import pytest

Expand Down Expand Up @@ -123,5 +124,77 @@ async def test_browse_nodes(server, client):
assert isinstance(results[1][0], Node)
assert isinstance(results[0][1], ua.BrowseResult)
assert isinstance(results[1][1], ua.BrowseResult)




async def test_read_attribute_value_without_index_range_reads_full_array(server, admin_client, client):
objects = admin_client.nodes.objects
array_data = [i for i in range(100)]
v = await objects.add_variable(3, 'MyArray', array_data)
await v.write_value(array_data)
v_client = client.get_node(v.nodeid)

# calling read_attribute without giving an index range must read the full array
array_data_read = (await v_client.read_attribute(ua.AttributeIds.Value)).Value.Value
assert len(set(array_data).intersection(array_data_read)) == len(array_data)


async def test_read_attribute_value_with_index_range_reads_subarray(server, admin_client, client):
objects = admin_client.nodes.objects
array_data = [i for i in range(100)]
v = await objects.add_variable(3, 'MyArray', array_data)
await v.write_value(array_data)
v_client = client.get_node(v.nodeid)

# calling read_attribute giving an index range must read the sub array,
# in this case 10th element until 19th element, meaning 10 elements
array_data_read = (await v_client.read_attribute(ua.AttributeIds.Value, '10:19')).Value.Value
assert len(set(array_data).intersection(array_data_read)) == 10

async def test_read_attribute_value_with_single_value_in_index_range_reads_subarray(server, admin_client, client):
objects = admin_client.nodes.objects
array_data = [i for i in range(100)]
v = await objects.add_variable(3, 'MyArray', array_data)
await v.write_value(array_data)
v_client = client.get_node(v.nodeid)

# calling read_attribute giving an index range must read the sub array,
# in this case 10th element until 19th element, meaning 10 elements
array_data_read = (await v_client.read_attribute(ua.AttributeIds.Value, '30')).Value.Value
assert len(array_data_read) == 1
assert array_data[30] == array_data_read[0]


async def test_write_attribute_value_without_index_range_writes_full_array(server, admin_client, client):
objects = admin_client.nodes.objects
array_data = [i for i in range(100)]
v = await objects.add_variable(3, 'MyArray', array_data)
await v.set_writable()
await v.write_value(array_data)
v_ro = client.get_node(v.nodeid)

# calling write_attribute without giving an index range must read the full array
array_data_write = [i+100 for i in range(100)]
await v_ro.write_attribute(ua.AttributeIds.Value, DataValue(Variant(array_data_write)))
array_data_read = (await v_ro.read_attribute(ua.AttributeIds.Value)).Value.Value
assert len(set(array_data_write).intersection(array_data_read)) == len(array_data_write)


async def test_write_attribute_value_with_index_range_writes_sub_array(server, admin_client, client):
objects = admin_client.nodes.objects
array_data = [i for i in range(100)]
v = await objects.add_variable(3, 'MyArray', array_data)
await v.set_writable()
await v.write_value(array_data)
v_ro = client.get_node(v.nodeid)

# calling write_attribute with an index range and an array os same length must write the sub array in
# the position specified by the index range
array_data_write = [i+100 for i in range(9)]
await v_ro.write_attribute(ua.AttributeIds.Value, DataValue(Variant(array_data_write)), '10:20')
array_data_read = (await v_ro.read_attribute(ua.AttributeIds.Value)).Value.Value
# assert the sub array was written
assert len(set(array_data_write).intersection(array_data_read)) == len(array_data_write)

# it was written in the correct location
sub_array = array_data_read[10:21]
assert len(set(array_data_write).intersection(sub_array)) == len(array_data_write)