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

Implementing WoT companion specification. #5

Merged
merged 12 commits into from
Oct 17, 2023

Conversation

jestun
Copy link
Contributor

@jestun jestun commented Sep 28, 2023

  • Added implementation of the WoT specification
  • Added possibility to map asset properties to predefined nodes in the address space.
  • Added example cases of mapping to predefined nodes
  • Added OPC UA properties to the Thing Description model
  • Added input and output arguments according to the WoT AssetManagement methods
  • Added support for local nodeset files
  • Mapped the AssetIds to be available through OPC UA method calls

@barnstee barnstee added the enhancement New feature or request label Sep 28, 2023
@barnstee barnstee self-requested a review September 28, 2023 12:07
Nodesets/Opc.Ua.WoT.Classes.cs Outdated Show resolved Hide resolved
Nodesets/Opc.Ua.WoT.DataTypes.cs Outdated Show resolved Hide resolved
Nodesets/Opc.Ua.WoT.NodeIds.csv Outdated Show resolved Hide resolved
Nodesets/Opc.Ua.WoT.NodeSet.xml Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This is an intermediate file from model compiler and can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently needed as there are some issues with methods input arguments, while importing a nodeset2.xml. This is probably related to the UA-.NETStandard stack issue: OPCFoundation/UA-.NETStandard#1056

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@jestun jestun Oct 4, 2023

Choose a reason for hiding this comment

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

Isn't it a bit against the idea of importing to first define the input/output arguments in the nodeset and then define them again a second time in the node manager?

Copy link
Member

Choose a reason for hiding this comment

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

It is. :-) But we shouldn't wait for the bug to be fixed in the stack.

Nodesets/Opc.Ua.WoT.Types.xsd Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the difference between configuring the TD to create new nodes vs mapping to existing nodes is clear it can be skipped,
this is just meant as an example for the latter.

UANodeManager.cs Outdated Show resolved Hide resolved
UANodeManager.cs Outdated
@@ -312,8 +375,11 @@ private void AddAsset(IList<IReference> references, string file, out ThingDescri
// generate DTDL content, convert back to WoT TD and compare to original
string dtdlContent = WoT2DTDLMapper.WoT2DTDL(contents);
string convertedWoTTDContent = WoT2DTDLMapper.DTDL2WoT(dtdlContent);
Debug.Assert(JObject.DeepEquals(JObject.Parse(convertedWoTTDContent), JObject.Parse(contents)));
//Debug.Assert(JObject.DeepEquals(JObject.Parse(convertedWoTTDContent), JObject.Parse(contents)));
Copy link
Member

Choose a reason for hiding this comment

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

Please comment back in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the new fields in the ThingDescription don't have any suitable fields in the DTDL so this compare will always fail.
To support this, the new fields should be added to the DTDL model.

Copy link
Member

Choose a reason for hiding this comment

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

Then you know what to do! :-)

string contents = File.ReadAllText(file);

// check file type (WoT TD or DTDL)
if (contents.Contains("\"@context\": \"dtmi:dtdl:context;2\""))
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the DTDL support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think DTDL support isn't needed here since the file is deleted by the assetId in the file name and it doesn't care about the contents at all.

Choose a reason for hiding this comment

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

I think, there are multiple strategies possible to check if there is a (valid) TD.

  1. File extension: *.td.json, *.td.jsonld
  2. content type: application/td+json
  3. @context check: https://www.w3.org/2022/wot/td/v1.1 is present
  4. JSON Schema check: apply official TD JSON Schema

Option 4 would be the best solution to be on the safe side in terms of trouble-free parsing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the ConfigureAsset method always creates a .jsonld file in the settings folder, I'd say we can safely execute the DeleteAsset method by only referencing the assetId. This means that any kind of checking before removal is unnecessary as long as the assetId matches what should be removed from the settings folder, no?

Copy link
Member

Choose a reason for hiding this comment

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

I think DTDL support isn't needed here since the file is deleted by the assetId in the file name and it doesn't care about the contents at all.

Yes, but we want to keep this here as a code example.

Copy link
Member

Choose a reason for hiding this comment

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

As the ConfigureAsset method always creates a .jsonld file in the settings folder, I'd say we can safely execute the DeleteAsset method by only referencing the assetId. This means that any kind of checking before removal is unnecessary as long as the assetId matches what should be removed from the settings folder, no?

I think Sebastian's point was to make the validation better than it currently is, that's all...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think DTDL support isn't needed here since the file is deleted by the assetId in the file name and it doesn't care about the contents at all.

Yes, but we want to keep this here as a code example.

Of course I can leave/add DTDL support there (in the DeleteAsset method), but what would it do in this case?
I can't really find any useful example on what to compare the input argument (assetId) to inside the parsed object unless we further change something. Ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Please simply compare the DTDL filename against the asset id and make sure the asset id ends up in the file name.

@barnstee
Copy link
Member

Have a look here for loading the predefined nodes directly from nodeset2.xml and therefore not requiring the model compiler: https://github.com/digitaltwinconsortium/ManufacturingOntologies/blob/2f155d8d2f3875b2267a3c7ed355c8d9bcaca641/Tools/FactorySimulation/Station/StationNodeManager.cs#L99

@jestun
Copy link
Contributor Author

jestun commented Sep 29, 2023

I've had some problems with the directly imported nodeset, the uanodes binary just works better. For example the input and output arguments are not passed correctly to the method call when using the nodeset, but works with the binary. There is an open issue on the .NET stack repo regarding this, but I haven't had time to dig any deeper into the problem.
The issue: OPCFoundation/UA-.NETStandard#1056

UANodeManager.cs Outdated
{
if (!_useWotNodesetBinary)
{
return;
Copy link
Member

Choose a reason for hiding this comment

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

Log an error here.

Copy link
Contributor Author

@jestun jestun Oct 4, 2023

Choose a reason for hiding this comment

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

This should never happen as File.Exists is already checked outside of this method, I think it's better if I just remove it.

UANodeManager.cs Outdated
{
if (!_useWotNodeset)
{
return;
Copy link
Member

Choose a reason for hiding this comment

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

Log an error here.

Copy link
Contributor Author

@jestun jestun Oct 4, 2023

Choose a reason for hiding this comment

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

This should never happen as File.Exists is already checked outside of this method, I think it's better if I just remove it.

UANodeManager.cs Outdated
@@ -173,7 +236,10 @@ public override void CreateAddressSpace(IDictionary<NodeId, IList<IReference>> e
{
externalReferences[ObjectIds.ObjectsFolder] = references = new List<IReference>();
}


ImportFromXml();
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps put your if (!_useWotNodesetBinary) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll add a check to both xml and binary here.

@@ -34,6 +34,10 @@ public class UANodeManager : CustomNodeManager2

private readonly string _wotNodeset = Path.Combine(Directory.GetCurrentDirectory(), "Nodesets", "Opc.Ua.WoT.NodeSet2.xml");
private readonly bool _useWotNodeset = false;

private readonly string _wotNodesetBinary = Path.Combine(Directory.GetCurrentDirectory(), "Nodesets", "Opc.Ua.WoT.PredefinedNodes.uanodes");
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to support both binary and nodeset.xml import? If we just support nodeset.xml import, we can remove the dependency on model compiler, which is something I've been asked to do many times before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently yes, importing the NodeSet as xml will currently not map the input and output arguments, but importing the binary will.
If someone can tell me how the input- and outputArguments can be mapped from the xml without too much hassle or
once this issue is fixed in the stack (if ever?), I think we could and should drop the binary as you suggest.

@barnstee barnstee merged commit 4a0e42b into OPCFoundation:main Oct 17, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants