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

Migration to W3C WoT standard #140

Closed
wants to merge 1 commit into from
Closed

Migration to W3C WoT standard #140

wants to merge 1 commit into from

Conversation

Citrullin
Copy link

A follow up for #139.
Only contains the actual migration of the library without example.

Thing.h Outdated
@@ -695,10 +695,10 @@ class ThingDevice {
#endif
}

void serialize(JsonObject descr, String ip, uint16_t port) {
void serialize(JbsonObject descr, String ip, uint16_t port) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

b like binary?

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand your comment.

@@ -695,7 +695,7 @@ class ThingDevice {
#endif
}

void serialize(JbsonObject descr, String ip, uint16_t port) {
void serialize(JsonObject descr, String ip, uint16_t port) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok please squash commit in previous one there is no value in keeping the mistake

if more changes can be squashed please do

Copy link
Author

@Citrullin Citrullin Oct 14, 2021

Choose a reason for hiding this comment

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

Well, in other projects it is common practice to squash everything in one commit for the feature/bugfix. I can also just follow that and squash everything together, if you want to. I am not very opinionated about this.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@benfrancis benfrancis left a comment

Choose a reason for hiding this comment

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

Thanks very much for this significant contribution!

This PR requires approval from either @rzr or @tim-hellhake in order to land and I'm not a C++ programmer, but I do have some feedback to offer from the perspective of the similar changes we've been making in WebThings Gateway (we should aim to be consistent) and regarding W3C compliance.

I agree with @rzr that it would have been better to do this in stages, and also to file issues for all the changes you planned to make in advance, but I've left feedback on the PR as is.

It would be really helpful if you could write a summary of the changes made in this PR since it's quite big, including what changes this makes to Thing Descriptions (with examples), the changes made to the API exposed by devices, and what's left to do. I may have further feedback once the full picture is more clear.

In general this seems like a good starting point towards W3C compliance, but I have quite a few comments regarding W3C compliance, consistency with the gateway and the removal of a lot of features which may not actually be necessary.

In general my suggestion is to:

  • Start with implementing the Thing Description specification first, before implementing WoT Discovery and WoT Profile (which are still being finalised)
  • Leave existing features in place (but in some cases not described in the Thing Description) until they can be replaced in a W3C-compliant way
  • Later implement the WoT Discovery specification (DNS-based discovery and possibly Directory Service API) and WoT Profile specification (Core Profile)

ThingProperty *property = this->firstProperty;
if (property != nullptr) {
JsonArray forms = descr.createNestedArray("forms");
JsonObject forms_prop = forms.createNestedObject();
forms_prop["rel"] = "properties";
Copy link
Member

Choose a reason for hiding this comment

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

Forms don't need a rel member

Copy link
Author

Choose a reason for hiding this comment

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

I thought I should keep it for compatibility with Mozilla WebThings. But I guess at that point the compatibility is already gone anyways.

@@ -696,9 +692,11 @@ class ThingDevice {
}

void serialize(JsonObject descr, String ip, uint16_t port) {
descr["id"] = this->id;
descr["id"] = "uri:" + this->id;
Copy link
Member

Choose a reason for hiding this comment

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

What is this uri: prefix for?

Copy link
Author

Choose a reason for hiding this comment

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

So that it actually is a valid uri. The spec requires it to be a valid URI and not some random ID.
https://w3c.github.io/wot-thing-description/#thing

Copy link
Member

Choose a reason for hiding this comment

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

That's a hack and prevents users from supplying their own URI (e.g. an https:// or urn: URI) as an ID as in the current examples, which is the current intended use.

If you want to ensure that ID is a valid URI then check that it's a valid URI, don't just prepend a made-up URI scheme.

@@ -55,7 +55,7 @@ class WebThingAdapter {
}

MDNS.addService("webthing", "tcp", port);
MDNS.addServiceTxt("webthing", "tcp", "path", "/");
MDNS.addServiceTxt("webthing", "tcp", "path", "/.well-known/wot-thing-description");
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is following the proposed well-known URI in the WoT Discovery specification. This is a bit risky since that is still tentative. For the gateway we opted to implement the WoT Thing Description specification first, and implement the WoT Discovery specification later once it's a bit more solid, including DNS-based discovery and the Directory Service API.

I would suggest holding back on this for now, but if you are going to start implementing parts of the WoT Discovery specification now then it might also make sense to change the mDNS service name to _wot which is also (tentatively) defined in that specification.

Copy link
Author

Choose a reason for hiding this comment

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

Well, it's just the path. Even if the path changes. It really isn't such a big deal to change that. Well the mDNS part for HTTP makes trouble. I don't remember the exact problem, but there are some.
I didn't wanted to touch more than really necessary to make it WoT TD 1.0 compliant. The compliance doesn't change, if it is a different URI. The WoT TD 1.0 doesn't even define where to find the TD. So, using the endpoint in the discovery spec kind of makes sense to use here.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't wanted to touch more than really necessary to make it WoT TD 1.0 compliant.

If you don't want to make changes that are not necessary to be W3C compliant then there's no need to change the URL.

Also please note that it's not possible to make WebThings libraries WoT TD 1.0 compliant because version 1.0 lacks the necessary semantics. The goal is to make them WoT TD 1.1 compliant, since 1.1 includes several new features added to make this possible, such as new operation types.

I personally do not like the well-known URL because it's much neater for the Thing Description to be served as the root document of the API, from /, like an index.html for the Web of Things. I suggest that if well-known URL is implemented in the future it should be an alias, not the canonical URL of the Thing Description.

You are correct that the Thing Description specification doesn't define discovery, that's what the WoT Discovery specification is for.

WebThings Gateway consumes web things created using this library using either the existing DNS-based discovery or using the "Add Thing by URL" feature where the URL is provided manually. (This is explained in the text in the README that you are proposing to remove https://github.com/WebThingsIO/webthing-arduino/blob/master/README.md).

As you hopefully know, WebThings Gateway will consume these W3C compliant web things using the new wot-adapter which has now been plugged into that "Add Thing by URL" feature of the gateway.

@@ -70,7 +70,7 @@ class WebThingAdapter {
this->server.on("/*", HTTP_OPTIONS,
std::bind(&WebThingAdapter::handleOptions, this,
std::placeholders::_1));
this->server.on("/", HTTP_GET,
this->server.on("/.well-known/wot-thing-description", HTTP_GET,
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Comment on lines -134 to -144
this->server.on((deviceBase + "/actions").c_str(), HTTP_GET,
std::bind(&WebThingAdapter::handleThingActionsGet, this,
std::placeholders::_1, device));
this->server.on((deviceBase + "/actions").c_str(), HTTP_POST,
std::bind(&WebThingAdapter::handleThingActionsPost, this,
std::placeholders::_1, device),
NULL,
std::bind(&WebThingAdapter::handleBody, this,
std::placeholders::_1, std::placeholders::_2,
std::placeholders::_3, std::placeholders::_4,
std::placeholders::_5));
Copy link
Member

Choose a reason for hiding this comment

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

Does this remove all the Actions resource from things? Rather than remove features, what we did for the gateway was to leave the existing API in place, but just changed the format of the invokeaction payload to remove the object wrapper so that it's W3C compliant and only exposed invokeaction and queryallactions operations in the Thing Description so that everything in the TD is W3C compliant. The plan is to later implement the full actions protocol binding from the WoT Profile specification once that is more solid.

Comment on lines +697 to +699

JsonArray context = descr.createNestedArray("@context");
context.add("https://www.w3.org/2019/wot/td/v1");
Copy link
Member

@benfrancis benfrancis Oct 14, 2021

Choose a reason for hiding this comment

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

I see that you removed the https://webthings.io/schemas @context from Thing Descriptions and replaced it with the W3C URL https://www.w3.org./2019/wot/td/v1.

We are planning to do this in the gateway too once the gateway's Thing Descriptions are fully W3C compliant, but please note that in order for web things using the webthing-arduino library to use any of the capability schemas from https://webthings.io/schemas/ then they should still include the webthings.io @context. This is an important part of the WebThings platform and if you remove these schemas then it will significantly degrade the user experience of users consuming these web things via WebThings Gateway, because it uses these semantic annotations to generate a richer UI.

I suggest including both contexts in an array if webthings.io semantic annotations are used by the web thing (e.g. "@context": ["https://www.w3.org/2019/wot/td/v1", "https://webthings.io/schemas/"]).

We could alternatively use a prefix (e.g. "@context": ["https://www.w3.org/2019/wot/td/v1", {"wt": "https://webthings.io/schemas/"} ]) but we'd need to add support for correctly consuming that in the gateway.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

@benfrancis benfrancis Oct 14, 2021

Choose a reason for hiding this comment

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

Thing Descriptions are not required to be valid JSON-LD, the default serialisation is plain JSON. In WebThings we just use this as a hint to the consumer and have historically not provided a JSON-LD representation of the schemas.

If we want to fix that issue then the solution is not to forbid users from using the existing webthings.io schemas (which as I mentioned is a significant and necessary feature of the WebThings platform), but to make that URL return a valid JSON-LD document when the relevant Accept headers are included in a request. That's going to be a bit tricky to fix because it's currently hosted in GitHub pages.

Removing this @context is not an option in my opinion, and is not what we'll be doing elsewhere in the WebThings platform.

Choose a reason for hiding this comment

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

I can understand both views. https://webthings.io/schemas/ has no alternate RDF-based representation in the background, that is the reason why every JSON-LD parser would result into a fetch error.

@benfrancis is right that TD can also simple be parsed with a JSON parser and the context interpretation can be done in an application specific style. However, the cleanest solution would be this one: "@context": ["https://www.w3.org/2019/wot/td/v1", {"wt": "https://webthings.io/schemas/"} ]. This would be TD and JSON-LD compliant since a JSON-LD would not try to fetch the prefix defined namespaces.

For the future it would be great if https://webthings.io/schemas/ would be also available as RDF model. Maybe this can also be merged with iotschema.org. If there is an interest I can check if I find a student to work on that.

Copy link
Member

@benfrancis benfrancis Oct 26, 2021

Choose a reason for hiding this comment

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

Thanks for the suggestions @sebastiankb.

the cleanest solution would be this one: "@context": ["https://www.w3.org/2019/wot/td/v1", {"wt": "https://webthings.io/schemas/"} ]. This would be TD and JSON-LD compliant since a JSON-LD would not try to fetch the prefix defined namespaces.

That is certainly an option. Note that currently I think it's the case that WebThings Gateway would recognise the semantic annotations if both contexts were included anonymously, but unfortunately would not recognise them if a prefix is used. Therefore this option would require implementation work on WebThings Gateway in order for web things using this library to continue to benefit from the enhanced user experience these schemas provide. It would probably be easier to just serve a JSON-LD file from this URL, but we may need to switch hosting providers in order to support HTTP content negotiation. I'm curious why JSON-LD parsers do not try to fetch prefix defined namespaces BTW?

For the future it would be great if https://webthings.io/schemas/ would be also available as RDF model. Maybe this can also be merged with iotschema.org. If there is an interest I can check if I find a student to work on that.

This is definitely a long term goal. However, the webthings.io schemas are currently used by around 100 gateway add-ons and an unknown number of web things built with the WebThings Framework. Therefore it's not really possible for one person to simply "merge" the schemas and for the issue to go away since all those implementations would need to be updated too. I think what we'd need to do is add support for iotschema.org schemas (including suggesting new ones that we think are missing) to WebThings Gateway and support both schema repositories to begin with. We could then try to encourage developers to migrate to using iotschema.org over time. Note that this would require a significant amount of UI design work in addition to development, since currently every one of the 23 device capabilities, 29 property types, 4 action types and 5 event types each has its own dedicated UI design in WebThings, implemented as web components. (See the UI designs below for a taste of what's involved).

capabilities

It could certainly be helpful for a student to create an RDF model from the current human-readable schema repository though.

Choose a reason for hiding this comment

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

Hmm, if all we did was strip out the prefixes when consuming the Thing Description then yes, possibly.

I would also just see this as an interim solution until a RDF support of https://webthings.io/schemas/ is available. Please note that I am trying to mitigate the situation here, as @Citrullin has made some efforts to make the code W3C-WoT compliant.

I do think creating an RDF representation of the schemas would be a worthwhile task, thank you. I could then separately figure out how to host it.

OK, I will check to see who can look at it.

Copy link
Member

Choose a reason for hiding this comment

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

Please note that I am trying to mitigate the situation here, as @Citrullin has made some efforts to make the code W3C-WoT compliant.

I see. Well if that is the only goal then as I mentioned, making the webthings.io/schemas URL resolve to a valid JSON-LD resource is not actually necessary to make the Thing Descriptions exposed by this library W3C compliant.

If your goal is to address the issues with this PR in order that it can land, that's really not the biggest problem in my view. The much bigger problem is the unnecessary and indiscriminate removal of all features which can't be described by a WoT Thing Description 1.0 Thing Description. That's inconsistent with the much more careful and considered approach we are taking elsewhere in the platform, which is something I've offered to discuss in more depth to ensure we are being consistent.

Copy link
Author

@Citrullin Citrullin Nov 3, 2021

Choose a reason for hiding this comment

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

Please note that I am trying to mitigate the situation here, as @Citrullin has made some efforts to make the code W3C-WoT compliant.

I see. Well if that is the only goal then as I mentioned, making the webthings.io/schemas URL resolve to a valid JSON-LD resource is not actually necessary to make the Thing Descriptions exposed by this library W3C compliant.

Prefix would be fine as well, since it doesn't result in a resolve. Please correct me if I am wrong, but that is not an option as well, because you need to implement something in the Gateway, right?

If your goal is to address the issues with this PR in order that it can land, that's really not the biggest problem in my view. The much bigger problem is the unnecessary and indiscriminate removal of all features which can't be described by a WoT Thing Description 1.0 Thing Description. That's inconsistent with the much more careful and considered approach we are taking elsewhere in the platform, which is something I've offered to discuss in more depth to ensure we are being consistent.

Do you mean the endpoints /actions and /properties? If so, all the properties and actions are already listed in the TD itself. So, you would just duplicate the information. I mean fine with me to add it though. I don't have hard feelings about it.

Or do you to always return an array with multiple TDs in it? That wouldn't be TD compliant, even not with TD 1.1 as far as I know. And implementing TDD in Arduino would really be an overkill.

Can you list your must haves in order to make it mergeable? I am getting really confused, because there are so many things you mentioned.

Copy link
Member

Choose a reason for hiding this comment

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

Prefix would be fine as well, since it doesn't result in a resolve. Please correct me if I am wrong, but that is not an option as well, because you need to implement something in the Gateway, right?

Yes, we just discussed that above.

Do you mean the endpoints /actions and /properties? If so, all the properties and actions are already listed in the TD itself. So, you would just duplicate the information. I mean fine with me to add it though. I don't have hard feelings about it.

Or do you to always return an array with multiple TDs in it? That wouldn't be TD compliant, even not with TD 1.1 as far as I know. And implementing TDD in Arduino would really be an overkill.

Can you list your must haves in order to make it mergeable? I am getting really confused, because there are so many things you mentioned.

We've already discussed all of this, but I will summarise it all again below.

Copy link
Member

Choose a reason for hiding this comment

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

I may have stumbled across a solution to the context issue in w3c/wot-thing-description#442

Apparently because of the issues with content negotiation it is common practice to use one URI for the ontology which resolves to a human readable resource, then a second URI with a versioned subdirectory for the JSON-LD context.

On GitHub Pages it should be possible to use:

  • https://webthings.io/schemas serving the current HTML resource
  • https://webthings.io/schemas/v1 serving a JSON resource

Although this means that implementations using the existing https://webthings.io/schemas context would still not resolve to a valid JSON-LD resource, new implementations could use the new context URI without breaking incoming links to the existing HTML page and I think the new URI could be used in WebThings Gateway without too much work.

This seems like a reasonable compromise to me, so if someone wants to provide a JSON-LD context file, then we can certainly host it at https://webthings.io/schemas/v1. I imagine that GitHub Pages will host it with an application/json content type, but hopefully that's not an issue.

Comment on lines +716 to +717
JsonArray securityJson = descr.createNestedArray("security");
securityJson.add("nosec_sc");
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for making this an array? A string should also be valid (see https://w3c.github.io/wot-thing-description/#thing).

Copy link
Author

Choose a reason for hiding this comment

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

Well, then I don't need to worry, if it is one of multiple security schema.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand your response. It seems this is another unnecessary change?

Copy link
Author

Choose a reason for hiding this comment

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

It makes it easier to add another security schema. But I think I did that because of some compatibility issues. I am not sure anymore.

Copy link
Member

Choose a reason for hiding this comment

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

If the Thing Description is valid and there are compatibility issues then that's surely a bug in the consumer, not the producer.

#ifndef WITHOUT_WS
{
JsonObject forms_prop = forms.createNestedObject();
//links_prop["rel"] = "alternate";
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this commented line

Comment on lines -734 to -744
{
JsonObject links_prop = links.createNestedObject();
links_prop["rel"] = "actions";
links_prop["href"] = "/things/" + this->id + "/actions";
}

{
JsonObject links_prop = links.createNestedObject();
links_prop["rel"] = "events";
links_prop["href"] = "/things/" + this->id + "/events";
}
Copy link
Member

@benfrancis benfrancis Oct 14, 2021

Choose a reason for hiding this comment

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

In the gateway we've turned these links into forms using queryallactions, subscribeallevents and unsubscribeallevents operations.

I would at least suggest keeping the /actions endpoint using the queryallactions operation (which is currently loosely defined in the Thing Description specification).

Events are a bit trickier since there's not really a W3C compliant way to describe the current Events resource in the API which is more like an event log. For the gateway what we decided to do is to implement implement Server-Sent Events and expose those endpoints in the Thing Description instead.

I would suggest doing this in webthing-arduino too, but I think in the meantime it's reasonable to remove this. That does raise the question of how you're describing the Event resources though?

Copy link
Author

@Citrullin Citrullin Oct 14, 2021

Choose a reason for hiding this comment

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

Well, I removed it, because it made the application crash when querying certain things. I don't think the Arduino Library needs to be a feature complete library with the old Mozilla WebThings. The important requirement was to migrate it to the W3C WoT TD 1.0 standard. Which I completed.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the Arduino Library needs to be a feature complete library with the old Mozilla WebThings. The important requirement was to migrate it to the W3C WoT TD 1.0 standard. Which I completed.

I'm afraid that's not really your decision, it is the decision of the module owner.

Copy link
Member

Choose a reason for hiding this comment

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

You didn't answer how you are describing the Event resources in the TD. I don't know of a W3C valid way of describing the current API, see w3c/wot-thing-description#892

Comment on lines -311 to -334
} else if (uri == deviceBase + "/properties") {
if (method == HTTP_GET || method == HTTP_OPTIONS) {
handleThingPropertiesGet(device->firstProperty);
} else {
handleError();
}
return;
} else if (uri == deviceBase + "/actions") {
if (method == HTTP_GET || method == HTTP_OPTIONS) {
handleThingActionsGet(device);
} else if (method == HTTP_POST) {
handleThingActionsPost(device);
} else {
handleError();
}
return;
} else if (uri == deviceBase + "/events") {
if (method == HTTP_GET || method == HTTP_OPTIONS) {
handleThingEventsGet(device);
} else {
handleError();
}
return;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

I suggest at least the /properties and /actions endpoints should be kept.

Copy link
Author

Choose a reason for hiding this comment

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

Let me test it. If it doesn't break anything, I am fine with it.

@Citrullin
Copy link
Author

Close, since the team doesn't want to support the Web of Things TD 1.0 standard defined by the W3C.

@Citrullin Citrullin closed this Oct 21, 2021
This was referenced Oct 21, 2021
@benfrancis
Copy link
Member

Hi @Citrullin,

I can see that you are frustrated, but please don't be disheartened by requests for changes in a code review, this is a normal part of the contribution process.

I can assure you that the team does want WebThings to comply with W3C WoT specifications, it is currently the primary focus of the 2.0 release and I have personally been working on this nearly full time for a number of months now. Please understand that webthing-arduino is just one of 16 libraries, 2 gateway implementations and over 100 add-ons which all rely on the same Web Thing API, so any changes to this API require significant co-ordination. And also that WebThings is maintained by a group of volunteers who mostly contribute in their spare time and may not have time to review large patches with no accompanying explanation or prior discussion.

As you hopefully know, @relu91 and I have been working hard over many months to discuss the best approach to make WebThings W3C WoT compliant, which has included many long meetings with three different W3C task forces to introduce new features to W3C specifications to make it possible. See WebThingsIO/gateway#2802 for a high level summary of that discussion.

I'm afraid the approach in this pull request of simply removing any features which can not be described by the existing Thing Description 1.0 specification is not acceptable in my view.

Ideally it would have been better for you to participate in the wider discussion and co-ordinate your work with the changes made to the gateway API. However, if you are interested in continuing to contribute then I'd be happy to set up a call to co-ordinate our work and discuss how you can contribute more effectively.

Thank you for the time you have already invested in the project.

@Citrullin
Copy link
Author

@benfrancis I don't have the time to have such a discussion if keeping the Mozilla RFC is more important than being compliant to the W3C recommendation. From my point of view this topic is pretty simple. WebThings isn't a finished and usable product. It uses the Mozilla RFC which isn't an industry standard. So, going for the industry standard, even if it isn't ideal should have the highest priority. I understand that some features are missing. But those can be added later in another iteration, instead of practically standing still. It's software, you can still add those features.

I need a library I can use now and I can advertise now with. I am not going to wait 2 years or whatever until I can actually use that library and market it.

Ideally it would have been better for you to participate in the wider discussion and co-ordinate your work with the changes made to the gateway API. However, if you are interested in continuing to contribute then I'd be happy to set up a call to co-ordinate our work and discuss how you can contribute more effectively.

Discussions are not necessary here and it also doesn't make sense to discuss. You made your point clear. You don't want to be WoT TD 1.0 compliant and I can't change that. Therefore there isn't anything to coordinate.

@rzr
Copy link
Collaborator

rzr commented Oct 21, 2021

Take it easy friends, this project is under community maintenance... What I can suggest to you is to try to split into smaller efforts then it will be easier for us to be integrated (and yes it can take time, we wont stop you to maintain a fork but it would be in community interest to avoid that)

More hints can help:

https://rzr.github.io/rzr-presentations/docs/flows

My 2c

@benfrancis
Copy link
Member

@Citrullin wrote:

if keeping the Mozilla RFC is more important than being compliant to the W3C recommendation.

This is a misrepresentation.

  1. We are not trying to continue to be compliant with our old Web Thing Description specification, we are trying to keep features of the API which our existing consumers depend on.
  2. It is not possible to describe the whole existing API using a WoT Thing Description 1.0 compliant Thing Description because the specification is missing key features.
  3. It is also not necessary to remove features from the API in order to make the Thing Descriptions exposed by devices W3C compliant (even WoT Thing Description 1.0 compliant), you can just choose not to describe those features in the Thing Description until Thing Descriptions gain the semantics to describe them.

Discussions are not necessary here and it also doesn't make sense to discuss.

If you are not willing to engage in discussion, then you are welcome to go elsewhere.

@Citrullin
Copy link
Author

Citrullin commented Oct 21, 2021

@Citrullin wrote:

if keeping the Mozilla RFC is more important than being compliant to the W3C recommendation.

This is a misrepresentation.

1. We are not trying to continue to be compliant with our old [Web Thing Description](https://webthings.io/api/#web-thing-description) specification, we are trying to keep features of the API which our existing consumers depend on.

I don't understand how this should effect a library which doesn't consume anything, but only produces a TD. If your gateway is able to read the WoT TD 1.0 this shouldn't even effect this library at all.

2. It is not possible to describe the whole existing API using a WoT Thing Description 1.0 compliant Thing Description because the specification is missing key features.

It doesn't need to. This is a library which doesn't act as a client.

3. It is also not necessary to remove features from the API in order to make the Thing Descriptions exposed by devices W3C compliant (even WoT Thing Description 1.0 compliant), you can just choose not to describe those features in the Thing Description until Thing Descriptions gain the semantics to describe them.

Well, I tested it and removed it for a reason. I kept all the other code I don't use. I don't remember the details, but it probably broke something. Besides that, the library has generally not the highest code quality. Everything is just in two header files. Besides that, the structure can improve a lot. So, I don't even understand all the discussions for a library which should get a major refactoring anyways.

Discussions are not necessary here and it also doesn't make sense to discuss.

If you are not willing to engage in discussion, then you are welcome to go elsewhere.

That's why I just closed the PR. I made the library WoT TD 1.0 compliant and removed everything that was in the way of getting there.

@benfrancis
Copy link
Member

benfrancis commented Nov 4, 2021

Can you list your must haves in order to make it mergeable? I am getting really confused, because there are so many things you mentioned.

OK, to summarise:

  1. Remove the rel member from Forms since it isn't needed
  2. Do not prepend the string "uri:" to the id member (if you want to add a check that the ID is valid URI, that's fine)
  3. Remove .well-known/wot-thing-description from the path of Thing Description(s) and mDNS broadcast. We'll implement the WoT Discovery specification later and make this an alias of the canonical Thing Description URL(s)
  4. Keep the top level endpoints for properties and actions using top level Forms with readallpropertiesand queryallactions operations. These meta operations are an important feature of the web thing (e.g. see Parallel prop loading can overwhelm device thing-url-adapter#106) which should not be removed. (See below for a note about events)
  5. You may need to modify the payload of an invokeaction operation to remove the object wrapper, like we did in WebThings Gateway, so that the payload matches the input data schema
  6. I am personally OK with removing the feature that enables a single device to expose multiple web things at a /things endpoint, however the module owner (@rzr) has the final decision. Note that only the resources at /things/{id} are actually Thing Descriptions which need to be compliant with the W3C WoT Thing Description 1.1 specification. The resource at /things provides an array of multiple things. If this is kept then it could either be made compliant with the WoT Discovery Directory Service API, or just left as it is since it's not doing any harm (not every resource served by a web server has to be a Thing Description)
  7. Rename variable names from "link" to "form" where appropriate
  8. Replace the timestamp in events
  9. Replace the webthings.io context. I suggest including both the W3C and WebThings contexts in an array ("@context": ["https://www.w3.org/2019/wot/td/v1", "https://webthings.io/schemas/"]) since this won't require changes in WebThings Gateway. If WoT Consumers say this is an invalid TD because the webthings.io/schemas resource doesn't resolve to a valid JSON-LD document then I suggest that's a bug in the Consumer. However, we will separately work on creating an appropriate JSON-LD resource and changing the hosting solution to fix this.
  10. Change the security member back to a string
  11. Remove the commented out line on line 740

Finally, we need to decide what to do about events. There is no way to describe the current Event and Events resources in the current Web Thing REST API using a W3C Thing Description (even in v1.1) because they provide a list of past events rather than an event stream which can be subscribed/unsubscribed to.

In WebThings Gateway the solution we came up with for events was to implement Server-Sent Events endpoints at /events and /events/{id} using HTTP content negotiation so that a client can choose either the existing JSON resource or an event stream. If you are interested in implementing that in webthing-arduino too then that's great. If not, then then only solution I can think of is to simply omit references to events in the Thing Description altogether, but leave the existing events API in place in case someone wants to use it until we get around to implementing SSE. Note that events can still be subscribed to using the WebSocket endpoint, which can be described by a top level Form.

As the module owner, @rzr may have further comments.

@Citrullin @sebastiankb I hope this helps.

@rzr
Copy link
Collaborator

rzr commented Jul 7, 2022

May I orphan this module since my involvement in webthings has declined since I am busy with oniroproject ..

@benfrancis
Copy link
Member

@rzr I've sent you a message on Matrix

@ameerhazo
Copy link

ameerhazo commented Jul 15, 2022

Hello @benfrancis, are there any plans to include the usage of different ontologies such as iotschema.org in the future? The WoT implementation that my team is developing has been using this ontology to describe different kinds of smart devices such as our smart meters.

@benfrancis
Copy link
Member

@ameerhazo Yes, see WebThingsIO/gateway#2929

There's nothing to stop you using other ontologies in your Things today via semantic annotations on the Producer side, but on the Consumer side WebThings Gateway won't yet do anything special with them.

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.

5 participants