-
Notifications
You must be signed in to change notification settings - Fork 81
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
opcua: nsu in nodeId (fixing #1334) #1335
base: master
Are you sure you want to change the base?
Conversation
73543d3
to
71765c5
Compare
5982c62
to
32cbe50
Compare
@erossignon many thanks! beside of the "ns=..." nodeId, TD needs also to allow "nsu=..." as node ID. Still we need to clearify, if the nodeID can be part of the href or is dedicated ua:nodeId term is needed. |
The idea is that we use For example, we now have {
"base": "opc.tcp://192.168.120.237:4840/",
"properties": {
"foo": {
"forms": [
{
"opcua:nodeId": "nsu=http://example.org/SpecialNamespace/;s=\"LED\".\"State\""
}
]
}
}
} which would become the following {
"base": "opc.tcp://192.168.120.237:4840/",
"properties": {
"foo": {
"forms": [
{
"href": "nsu=http://example.org/SpecialNamespace/;s=\"LED\".\"State\""
}
]
}
}
} The problem I see are href parsers since the absolute URL would become the following
A more Web style friendly format would be to use query parameters
Note1: the difference is the Note2: it has the downside that one needs to reconstruct the format string at OPC UA side. Question: I wonder whether people share the concern that if we purely use the OPC UA format string href parsers will destroy/corrupt the information. Note3: we also need to be careful with some specific characters like |
May be one way is to encode the href portion using encodeUriComponent decodeUriComponent
to prevent any clashes |
Going with URL encoding was also the preliminary decision taken for MQTT at w3c/wot-binding-templates#292 (comment) |
The problem I see with doing so is that for humans, it is not easy to read the "encoded" string. Anyhow, It seems we need a general decision that can be taken across all/other bindings... |
We had a chat about this together with @sebastiankb @danielpeintner @wiresio and @Kaz040. Here our points, that need to be discussed with the OPC UA community as well.
|
In the OPC UA WoT Binding WG we discussed 2 relevant options to provide the nodeId information: Option I:
Note: The UA server endpoint information Pro:
Con:
Option II:
Pro:
Con:
What do you think? Ping @barnstee @randy-armstrong @erossignon @danielpeintner @egekorkan @relu91 |
W3C WoT TD Task Force call today:
|
Many thanks for your feedback!
This example coming from a real sample implementation which uses nested
OK, good point. It was hoped that the values of queries would be excluded...
Actually, the answer is already given above.
It seems that a more readable approach is not possible... |
There also seems to be a nice alternative where the UA nodeID remains as is (=readability) and keeps the WoT consistency:
The fragment identifier # takes any character, an URL encoding is not needed. (btw: this approach was also discussed for MQTT in the past) |
This is perfect! |
It might be a clean solution from the readability point of view, but it still breaks the URI semantics explained in RFC3986 (see my old comment in the thread cited by @sebastiankb). We can still ofc bend a little bit the rules... but it doesn't convince me 100%. Encoding the string does not seem to be a big burden from my point of view, it has the benefit of working with JSON too... (even using the fragment solution you would still to |
I have just found an interesting piece of information from the OPC UA specification about NodeIds:
I'm not sure I understand the examples presented, e.g.
Why only I will discuss this in the next UA WG meeting. @relu91 do you have time to join us on Tuesday at 4 pm CET? |
At yesterday's working group meeting, we achieved a breakthrough with the following solution for OPC UA:
|
W3C WoT TD TF call today:
|
Hi @erossignon |
d45c924
to
fc725c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are almost there. I have some questions and propose some changes. See below.
Thank you @erossignon 👍
@@ -2,7 +2,7 @@ | |||
"extends": "../../tsconfig.json", | |||
"compilerOptions": { | |||
"outDir": "dist", | |||
"module": "commonjs", | |||
"module": "Node16", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@relu91 you kept "commonjs"
because of scrambled output, right? Do you mind changing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should be kept, any reason for changing it @erossignon ?
"module": "Node16", | ||
"moduleResolution": "Node16" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think those 2 statements are necessary since the root tsconfig.json
defines them already like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes they could be moved, and modulResolution is also redundant when using Node16 as module. But we can keep both in the main tsconfig.json for documentation.
@@ -2,7 +2,10 @@ | |||
"extends": "../../tsconfig.json", | |||
"compilerOptions": { | |||
"outDir": "dist", | |||
"rootDir": "src" | |||
"rootDir": "src", | |||
"skipLibCheck": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we run into issues if we do the checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use skipLibCheck only as a last resort.
@@ -51,7 +52,7 @@ | |||
"@types/chai-as-promised": "^7.1.4", | |||
"@types/chai-spies": "^1.0.4", | |||
"@types/mocha": "^9.0.0", | |||
"@types/node": "^18.19.71", | |||
"@types/node": "22.13.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change necessary? @relu91 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we still support Node 18, so the maximum upgrade that we can do here is to move the minor. See https://github.com/eclipse-thingweb/node-wot/blob/master/.github/workflows/ci.yaml#L22
@sebastiankb @danielpeintner could you document the decision that was taken in this PR or point to the decision? |
No description provided.