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

Standard address space 1.04 #574

Closed

Conversation

cirp-usf
Copy link
Contributor

@cirp-usf cirp-usf commented Apr 6, 2018

For previous discussion, see #572

@oroulet
Copy link
Member

oroulet commented Apr 7, 2018

  • Instead of forcing creation of DataTypeDefinition by xml parser I think it should simply be defined in uatypes.py
  • Editing autogenerated files by hand should really be avoided. Can you explain again what you edited by hand and what error you got? We should try to find a better solution
  • If it is not too much work for you it would be great to split the commit in to. One for manual changes in code and one for autogenerated changes

@@ -924,7 +924,7 @@ def test_data_type_to_variant_type(self):
ua.ObjectIds.Structure: ua.VariantType.ExtensionObject,
ua.ObjectIds.EnumValueType: ua.VariantType.ExtensionObject,
ua.ObjectIds.Enumeration: ua.VariantType.Int32, # enumeration
ua.ObjectIds.AttributeWriteMask: ua.VariantType.Int32, # enumeration
ua.ObjectIds.AttributeWriteMask: ua.VariantType.UInt32, # enumeration
Copy link
Member

Choose a reason for hiding this comment

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

you need to remove comment too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I forgot that. I'll remove it.

@@ -302,7 +302,7 @@ def test_xml_enum(self):
self._test_xml_var_type(o, "enum")

def test_xml_enumvalues(self):
o = self.opc.nodes.objects.add_variable(2, "xmlenumvalues", 0, varianttype=ua.VariantType.Int32, datatype=ua.ObjectIds.AttributeWriteMask)
o = self.opc.nodes.objects.add_variable(2, "xmlenumvalues", 0, varianttype=ua.VariantType.UInt32, datatype=ua.ObjectIds.AttributeWriteMask)
Copy link
Member

@oroulet oroulet Apr 8, 2018

Choose a reason for hiding this comment

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

In fact that test is broken. AttributeWriteMask does not seem to inherit enum anymore...

@cirp-usf
Copy link
Contributor Author

cirp-usf commented Apr 8, 2018

Regarding DataTypeDefinition, the change was on generate_model.py, which don't seem to include uatypes. My solution feels like a hack and I'm all for something better, but I don't see how changing uatypes.py will make a difference. The parse_struct method in generate_model.py travels up the hierarchy looking for the parents of a struct and must find a DataTypeDefinition in the list of structs that is in model. DataTypeDefinition doesn't seem to be defined in Opc.Ua.Types.bsd, which is the file processed by these scripts. I think this case is similar to ExtentionObject, which exists in uatypes.py but was nonetheless added in parser.

Should I simply add a DataTypeDefinition to uatypes.py and see if it works?

@oroulet
Copy link
Member

oroulet commented Apr 8, 2018

It will work. Everything in uatypes is imported in the autogenerated files

@cirp-usf
Copy link
Contributor Author

cirp-usf commented Apr 9, 2018

I'm preparing a new branch where I'll rework these modifications with smaller commits.

@cirp-usf
Copy link
Contributor Author

cirp-usf commented Apr 9, 2018

I created a new branch. I intent to use this branch for sharing tests results and make a better PR later.

In that new branch, I only committed the new xml files and (in a different commit) added a DataTypeDefinition to uatypes.py.

That did not work. generate_protocol_python.py fails with the following error:

F:\Python\python-opcua\schemas>python3 generate_protocol_python.py
Parsing:  Opc.Ua.Types.bsd
Traceback (most recent call last):
  File "generate_protocol_python.py", line 227, in <module>
    model = p.parse()
  File "F:\Python\python-opcua\schemas\generate_model.py", line 304, in parse
    struct = self.parse_struct(child)
  File "F:\Python\python-opcua\schemas\generate_model.py", line 353, in parse_struct
    tmp = self.model.get_struct(tmp.basetype)
  File "F:\Python\python-opcua\schemas\generate_model.py", line 112, in get_struct
    raise Exception("No struct named: " + str(name))
Exception: No struct named: DataTypeDefinition

That was the same error I got without adding DataTypeDefinition to uatypes. Maybe I did something while adding it, but I don't think so.

uatypes.py is not imported (nor even read) by any of the generate_* scripts in schemas. The automatic code generation process seems therefore to be independent of uatypes.py.

@oroulet
Copy link
Member

oroulet commented Apr 11, 2018

I looked at code. I wrote that stuff several years ago and xml parsing is never easy to read....
It looks like BaseType is only used for structs degined in xml. If BaseTypeDefinition is not defined anywhere then I think that your first approach was best. Almost looks like a bug in spec (It is not the first one ;-) ). Sorry for the extra work

@cirp-usf
Copy link
Contributor Author

Nothing to be sorry about :) My solution does feel like a hack.

DataTypeDefinition is defined, I think, in Opc.Ua.Types.xsd. I looks like an empty struct:

  <xs:complexType name="DataTypeDefinition">
    <xs:sequence>
    </xs:sequence>
  </xs:complexType>
  <xs:element name="DataTypeDefinition" type="tns:DataTypeDefinition" />

Also, as far as I understand, DataTypeDefinition is only used as a superclass for other types. It doesn't have any functionality of its own.

I don't understand the difference between bsd and xsd files. Maybe there is a better option reading the xsd file instead, but I have now idea how to do it.

There are other problems with my PR that I would like to tackle. The most important being autogenerated code that I had to manually edit. So I suggest leaving my hack for DataTypeDefinition there as a temporary solution while I go forward with the other issues.

I'll publish my work on this branch in order to share it, but I intend to make another PR from a clean branch when the issues are solved.

@cirp-usf
Copy link
Contributor Author

I made commits to my development branch.

I reverted the uatypes.py modification, added my DataTypeDefinition hack, made minor adjustments to the code generating scripts and ran the code generation in the following order:

  1. generate_protocol_python.py
  2. generate_ids.py
  3. generate_statuscode.py
  4. generate_uaerrors.py
  5. generate_event_objects.py
  6. generate_address_space.py

Then I committed the automatically generated code. As it stands, there is a syntax error in opcua\server\standard_address_space\standard_address_space_part5.py

python XXX.py
Traceback (most recent call last): 
 File "XXX.py", line 8, in <module>
    import opcua
  File "f:\python\python-opcua\opcua\__init__.py", line 10, in <module>
    from opcua.server.server import Server
  File "f:\python\python-opcua\opcua\server\server.py", line 16, in <module>
    from opcua.server.internal_server import InternalServer
  File "f:\python\python-opcua\opcua\server\internal_server.py", line 30, in <module>
    from opcua.server.standard_address_space import standard_address_space
  File "f:\python\python-opcua\opcua\server\standard_address_space\standard_address_space.py", line 8, in <module>
    from opcua.server.standard_address_space.standard_address_space_part5 import create_standard_address_space_Part5
  File "f:\python\python-opcua\opcua\server\standard_address_space\standard_address_space_part5.py", line 1588
    attrs.Value = ua.Variant("
                             ^
SyntaxError: EOL while scanning string literal

After fixing this (manually) the next error is:

  File "f:\python\python-opcua\opcua\server\standard_address_space\standard_address_space.py", line 44, in fill_address_space
    assert len(server.postponed) == 1561, len(server.postponed)
AssertionError: 2153

After working around this (changing the test value to 2153), I get the really complicated error:

[...]
  File "c:\users\usf\python\python-opcua\opcua\server\standard_address_space\standard_address_space.py", line 32, in __exit__
    assert len(remaining) == 0, remaining
AssertionError: [AddReferencesItem(SourceNodeId:NumericNodeId(i=15957), ReferenceTypeId:NumericNodeId(i=46), IsForward:True, TargetServerUri:None, TargetNodeId:NumericNodeId(i=15958), TargetNodeClass:NodeClass.DataType), 
AddReferencesItem(SourceNodeId:NumericNodeId(i=15957), ReferenceTypeId:NumericNodeId(i=46), IsForward:True, TargetServerUri:None, TargetNodeId:NumericNodeId(i=15959), TargetNodeClass:NodeClass.DataType),
AddReferencesItem(SourceNodeId:NumericNodeId(i=15957), ReferenceTypeId:NumericNodeId(i=46), IsForward:True, TargetServerUri:None, TargetNodeId:NumericNodeId(i=15960), TargetNodeClass:NodeClass.DataType), 
AddReferencesItem(SourceNodeId:NumericNodeId(i=15957), ReferenceTypeId:NumericNodeId(i=46), IsForward:True, TargetServerUri:None, TargetNodeId:NumericNodeId(i=15961), TargetNodeClass:NodeClass.DataType), 
[large list of references removed]

What I found out was that the node i=15957 (http://opcfoundation.org/UA/) from standard_address_space_part5.py wasn't being added. I had to move its parent (node i=11715) (Namespaces) to a position before the node i=15957 in standard_address_space_par5.py file. And I also had to move the node i=2253 (Server) before Namespaces

Once that was made, I got rid of this error but the length of server.postponed list in standard_address_space.py changed to 2134. After adjusting that assertion again I could start a server without problems.

So I detect two problems in automatic code generation. One was the string with embedded newline. The other was two nodes inserted out of order. Both problems in part 5.

I committed the manual modifications as well. I'll look into why the problems happened.

@oroulet
Copy link
Member

oroulet commented Apr 11, 2018

Ok. Then a better solution could be to change parser to print a warning if parent struct is not found and do not add it to list of parents. That should fix the issue.

@oroulet
Copy link
Member

oroulet commented Apr 11, 2018

Concerning the assert: please remove it entirely. It looks stupid. Better to print a warning of there is a small error

@oroulet
Copy link
Member

oroulet commented Apr 11, 2018

And commit all code generation stuff in one commit so i can pull it and have sa look

@cirp-usf
Copy link
Contributor Author

I've solved the first error on the autogenerated code, namely the syntax error. It was caused by a string in xml file having an embedded new line character. The relevant part of the xml file is:

  <UAVariable NodeId="i=15964" BrowseName="StaticStringNodeIdPattern" ParentNodeId="i=15957" DataType="String">
    <DisplayName>StaticStringNodeIdPattern</DisplayName>
    <Description>A regular expression which matches string node ids are the same in every server that exposes them.</Description>
    <References>
      <Reference ReferenceType="HasTypeDefinition">i=68</Reference>
      <Reference ReferenceType="HasProperty" IsForward="false">i=15957</Reference>
    </References>
    <Value>
      <String xmlns="http://opcfoundation.org/UA/2008/02/Types.xsd">
      </String>
    </Value>
  </UAVariable>

The resulting python file has

    attrs.Value = ua.Variant("
      ", ua.VariantType.String)

which have a syntax error. In fact, every multiline string would get printed with unescaped newlines and the resulting python file would be invalid.

What I did was simply replace the to_value() method calls of generate_address_space.py with repr(). I think this is the correct thing to do, given that we are producing strings that are meant to be python code. The method to_value was used only in two places so I removed it entirely.

This solved the syntax error problem.

I pushed another separate commit for this. I'll look at the other problem next (nodes out of order). I'll make yet another branch later with the modifications grouped in a single commit when I'm finished with this problem.

@cirp-usf
Copy link
Contributor Author

I've found the problem with adding nodes failing. This happens because the order of the nodes in part 5 xml file is such that some nodes are listed before their parents. So when create_standard_address_space_Part5 runs those nodes can not be added.

The problem is similar to the insertion of references that motivated #509, but this time with the nodes themselves. The solution I made applies the same principle of postponing to the nodes.

With this modification, the autogenerated files no longer have to be manually edited. I've pushed this to my fork for documentation.

@cirp-usf
Copy link
Contributor Author

I created a new branch with all these modifications in what I hope is a sane order. Is it OK to create a new PR from it? Or is there a better way? I'm new to this PR stuff...

@oroulet
Copy link
Member

oroulet commented Apr 13, 2018

Yes closer this one and make a new one. You can in fact create s pr from first line of code and flag it with WIP:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants