Server support for read array with index range #636

jcbastosportela · 2021-07-30T11:55:14Z

fixes #635

oroulet · 2021-07-30T20:12:28Z

Is that working? Nobody is supposed to use address space class directly...
Also add a test showing how to use that feature.
And move all string parsing at higher level. At address space level you can pass a tuple of indexes

jcbastosportela · 2021-07-30T23:40:49Z

Is that working? Nobody is supposed to use address space class directly...

Well, it works. I honestly don't thoroughly understand the inner of the project, more form the user perspective. However, after debugging and trying to follow the code I have came to the conclusion that when an external read (a read requested issued by a client) lands on the address_space.py::AttributeService::read()

uaprocessor.py::_process_message() calls the:
__internal_session.py::InternalSession::read() that calls:
____internal_server.py::InternalServer::attribute_service::read meaning address_space.py::AttributeService::read()

InternalSession holds an instance of internal_server.py::InternalServer which instantiates address_space.py::AddressSpace used to instantiate address_space.py::AttributeService

Now, if you ask me, I wonder why is the InternalSession calling the read via the InternalServer::attribute_service member's read method when InternalServer provides a method InternalServer::read_attribute_value that would call the address_space.py::AddressSpace::read_attribute_value.

Do you think this needs fixing also, or is this intentional and is like so for a reason. Maybe someone can help me in this point.

Also add a test showing how to use that feature.

I am not sure if I will know how to use the tests you have setup, but will give it a try.

And move all string parsing at higher level. At address space level you can pass a tuple of indexes

I can do that.

Moreover, as far as I can see this functionality is also missing for the write operation, maybe can be addressed later in other PR if I can understand the "correct" fix for this.

…ttribute_value

jcbastosportela · 2021-07-31T02:38:25Z

with this last changes address_space.py::AttributeService seems that is rendered unused. If this changes are okay maybe it must be removed. ~~I tried writing tests, but it wasn't working correctly, I will have to look after, for now maybe you can understand what I am trying to solve by looking the zip attached (server+xml, and the client)~~
examples.zip

oroulet · 2021-08-14T17:59:11Z

asyncua/server/address_space.py

+            value = attval.value_callback()
+        else:
+            value = attval.value
+        if not index_range is None:


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

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

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?

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

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

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

fix the comments from today and it might be good enough

oroulet · 2021-08-14T18:02:29Z

asyncua/server/address_space.py

@@ -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()


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

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

oroulet · 2021-08-14T18:02:51Z

asyncua/server/address_space.py

-        return attval.value
-
-    async def write_attribute_value(self, nodeid, attr, value):
+            value = attval.value_callback()


just return here insteas of making a local variable

oroulet · 2021-08-14T18:03:01Z

asyncua/server/address_space.py

-    async def write_attribute_value(self, nodeid, attr, value):
+            value = attval.value_callback()
+        else:
+            value = attval.value


oroulet · 2021-09-21T08:12:23Z

asyncua/common/ua_utils.py

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


good method

oroulet · 2021-09-21T08:12:51Z

asyncua/common/ua_utils.py

+    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/


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

oroulet · 2021-09-21T08:13:46Z

asyncua/common/ua_utils.py

+    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/
+    else:
+        raise Exception(f'Read with bad index range: {ua_index_range}')


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

oroulet · 2021-09-21T08:19:31Z

asyncua/server/address_space.py

+                return ua.StatusCode(ua.StatusCodes.BadWriteNotSupported)
+            try:
+                value = DataValue(Variant(value.Value.Value[index_range[0]:index_range[1]]))
+            except Exception as ex:


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

oroulet · 2021-09-21T08:20:45Z

asyncua/server/internal_server.py

-        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:


same here: if index_range is not None

oroulet · 2021-09-21T08:21:38Z

asyncua/server/internal_server.py

+        if not index_range is None:
+            try:
+                py_list_index_range = ua_index_range_to_list_slice(index_range)
+            except Exception as ex:


probably possible to only catch ValueError here

oroulet · 2021-09-21T08:22:13Z

asyncua/server/internal_server.py

        """
        directly read datavalue of the Attribute
        """
-        return self.aspace.read_attribute_value(nodeid, attr)  
+        if not index_range is None:


oroulet · 2021-09-21T08:23:03Z

asyncua/server/internal_server.py

+                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


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

oroulet · 2021-09-21T17:39:38Z

asyncua/server/internal_session.py

+        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
+                ]


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

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

oroulet · 2021-09-21T17:40:27Z

asyncua/server/internal_session.py

        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:


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

oroulet · 2021-09-21T17:50:17Z

asyncua/server/server.py

@@ -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:


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

oroulet · 2021-09-21T17:50:28Z

asyncua/server/server.py


-    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:


same here. string parsing is slow

oroulet · 2021-09-21T17:51:19Z

asyncua/server/internal_server.py

@@ -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:


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

ybozer · 2021-10-18T13:41:33Z

@oroulet I took over this task from @jcbastosportela since he doesn't have time for it.
And I worked out the remarks and made some more tests. Doing that I found the bug that I reported #689. So I am waiting for this bug to be fixed before pushing my changes for this PR. I'm saying this to inform you case you are thinking about making new release and you want to include this one.

Server support for read array with index range

efe146c

added index_range to server::read_attribute_value and server::write_a…

001f318

…ttribute_value

added unittests

3ac277b

oroulet reviewed Aug 14, 2021

View reviewed changes

oroulet reviewed Sep 21, 2021

View reviewed changes

asyncua/common/ua_utils.py

Returns:

Tuple[int,int]: Equivalent python list slicing tuple with low and high bounds

"""

Copy link

Member

oroulet Sep 21, 2021

Choose a reason for hiding this comment

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

good method

oroulet reviewed Sep 21, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server support for read array with index range #636

Server support for read array with index range #636

jcbastosportela commented Jul 30, 2021 •

edited

Loading

oroulet commented Jul 30, 2021

jcbastosportela commented Jul 30, 2021

jcbastosportela commented Jul 31, 2021 •

edited

Loading

oroulet Aug 14, 2021

oroulet Aug 14, 2021

jcbastosportela Aug 17, 2021

jcbastosportela Sep 20, 2021

oroulet Sep 21, 2021

oroulet Sep 21, 2021

oroulet Aug 14, 2021

oroulet Sep 21, 2021

oroulet Aug 14, 2021

oroulet Aug 14, 2021

oroulet Sep 21, 2021

oroulet Sep 21, 2021

oroulet Sep 21, 2021

oroulet Sep 21, 2021

oroulet Sep 21, 2021

oroulet Sep 21, 2021

oroulet Sep 21, 2021

oroulet Sep 21, 2021

oroulet Sep 21, 2021

oroulet Sep 21, 2021

oroulet Sep 21, 2021

oroulet Sep 21, 2021 •

edited

Loading

oroulet Sep 21, 2021

oroulet Sep 21, 2021

ybozer commented Oct 18, 2021


		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:

Server support for read array with index range #636

Are you sure you want to change the base?

Server support for read array with index range #636

Conversation

jcbastosportela commented Jul 30, 2021 • edited Loading

oroulet commented Jul 30, 2021

jcbastosportela commented Jul 30, 2021

jcbastosportela commented Jul 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oroulet Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ybozer commented Oct 18, 2021

jcbastosportela commented Jul 30, 2021 •

edited

Loading

jcbastosportela commented Jul 31, 2021 •

edited

Loading

oroulet Sep 21, 2021 •

edited

Loading