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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 14 additions & 113 deletions ESPWebThingAdapter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


DefaultHeaders::Instance().addHeader("Access-Control-Allow-Origin", "*");
DefaultHeaders::Instance().addHeader("Access-Control-Allow-Methods",
Expand All @@ -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.

std::bind(&WebThingAdapter::handleThings, this,
std::placeholders::_1));

Expand Down Expand Up @@ -131,17 +131,7 @@ class WebThingAdapter {
std::bind(&WebThingAdapter::handleThingPropertiesGet,
this, std::placeholders::_1,
device->firstProperty));
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));
Comment on lines -134 to -144
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.


this->server.on((deviceBase + "/events").c_str(), HTTP_GET,
std::bind(&WebThingAdapter::handleThingEventsGet, this,
std::placeholders::_1, device));
Expand Down Expand Up @@ -362,16 +352,15 @@ class WebThingAdapter {
request->beginResponseStream("application/json");

DynamicJsonDocument buf(LARGE_JSON_DOCUMENT_SIZE);
JsonArray things = buf.to<JsonArray>();
JsonObject thing = buf.to<JsonObject>();
ThingDevice *device = this->firstDevice;
while (device != nullptr) {
JsonObject descr = things.createNestedObject();
device->serialize(descr, ip, port);
descr["href"] = "/things/" + device->id;
device->serialize(thing, ip, port);
thing["href"] = "/things/" + device->id;
device = device->next;
}

serializeJson(things, *response);
Comment on lines -365 to -374
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 support for having multiple web things per device? I would actually support that change since I never liked that feature, but it's a bit strange to only support one thing but still use things/ in the URL. I'd suggest that if you do this (and others agree), that the base URL for a thing's API (and the URL of its Thing Description) should be / rather than /things/{id}.

If the others do not agree and this feature is kept, we should in future consider supporting the Directory Service API and _directory._sub._wot DNS service name proposed in the WoT Discovery specification when providing a list of things.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I removed that, because it produces an array as output, which isn't WoT compliant.
So, I didn't even really had another option than to remove it.
Well, yes, the whole naming schema in generally is not very ideal. This was an effort supported by Siemens.
The priority was to make it WoT compliant and not so much to clean everything up. There are a lot of things which can get improved. The whole: We put everything in two header files is also not very ideal. It's also not a very good coding convention. But as I said: Priority on migrating it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I removed that, because it produces an array as output, which isn't WoT compliant.

The resource returned from the /things endpoint doesn't need to be a valid Thing Description because it isn't a Thing Description. It's more like a directory.

So, I didn't even really had another option than to remove it.

There are two other options:

  1. Leave the /things endpoint in place, it doesn't prevent individual Thing Descriptions from being valid (which is the goal)
  2. Make the /things endpoint compliant with the Directory Service API https://w3c.github.io/wot-discovery/#directory-thing-description (I understand that it will soon be possible to have a read-only directory)

As I said I personally don't mind removing this feature (as long as the rest of the URL structure is cleaned up), but others may not agree.

serializeJson(thing, *response);
request->send(response);
}

Expand Down Expand Up @@ -474,6 +463,7 @@ class WebThingAdapter {
void handleThingActionPost(AsyncWebServerRequest *request,
ThingDevice *device, ThingAction *action) {
if (!verifyHost(request)) {
Serial.println("Invalid Host");
return;
}

Expand All @@ -486,31 +476,19 @@ class WebThingAdapter {
new DynamicJsonDocument(SMALL_JSON_DOCUMENT_SIZE);
auto error = deserializeJson(*newBuffer, (const char *)body_data);
if (error) { // unable to parse json
Serial.println("Unable to parse JSON");
b_has_body_data = false;
memset(body_data, 0, sizeof(body_data));
request->send(500);
delete newBuffer;
return;
}

JsonObject newAction = newBuffer->as<JsonObject>();

if (!newAction.containsKey(action->id)) {
b_has_body_data = false;
memset(body_data, 0, sizeof(body_data));
request->send(400);
delete newBuffer;
return;
}

ThingActionObject *obj = device->requestAction(newBuffer);

ThingActionObject *obj = action->create(newBuffer);
if (obj == nullptr) {
b_has_body_data = false;
memset(body_data, 0, sizeof(body_data));
request->send(500);
delete newBuffer;
return;
memset(body_data, 0, sizeof(body_data));
request->send(500);
return;
}

#ifndef WITHOUT_WS
Expand Down Expand Up @@ -567,84 +545,7 @@ class WebThingAdapter {
request->send(response);
}

void handleThingActionsGet(AsyncWebServerRequest *request,
ThingDevice *device) {
if (!verifyHost(request)) {
return;
}
AsyncResponseStream *response =
request->beginResponseStream("application/json");

DynamicJsonDocument doc(LARGE_JSON_DOCUMENT_SIZE);
JsonArray queue = doc.to<JsonArray>();
device->serializeActionQueue(queue);
serializeJson(queue, *response);
request->send(response);
}

void handleThingActionsPost(AsyncWebServerRequest *request,
ThingDevice *device) {
if (!verifyHost(request)) {
return;
}

if (!b_has_body_data) {
request->send(422); // unprocessable entity (b/c no body)
return;
}

DynamicJsonDocument *newBuffer =
new DynamicJsonDocument(SMALL_JSON_DOCUMENT_SIZE);
auto error = deserializeJson(*newBuffer, (const char *)body_data);
if (error) { // unable to parse json
b_has_body_data = false;
memset(body_data, 0, sizeof(body_data));
request->send(500);
delete newBuffer;
return;
}

JsonObject newAction = newBuffer->as<JsonObject>();

if (newAction.size() != 1) {
b_has_body_data = false;
memset(body_data, 0, sizeof(body_data));
request->send(400);
delete newBuffer;
return;
}

ThingActionObject *obj = device->requestAction(newBuffer);

if (obj == nullptr) {
b_has_body_data = false;
memset(body_data, 0, sizeof(body_data));
request->send(500);
delete newBuffer;
return;
}

#ifndef WITHOUT_WS
obj->setNotifyFunction(std::bind(&ThingDevice::sendActionStatus, device,
std::placeholders::_1));
#endif

DynamicJsonDocument respBuffer(SMALL_JSON_DOCUMENT_SIZE);
JsonObject item = respBuffer.to<JsonObject>();
obj->serialize(item, device->id);
String jsonStr;
serializeJson(item, jsonStr);
AsyncWebServerResponse *response =
request->beginResponse(201, "application/json", jsonStr);
request->send(response);

b_has_body_data = false;
memset(body_data, 0, sizeof(body_data));

obj->start();
}
Comment on lines -570 to -645
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned above, for the gateway we left the existing actions API in place so as not to remove features, and only modified the payload of the invokeaction operation (i.e. POST /actions/{actionName}).

Copy link
Author

Choose a reason for hiding this comment

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

Actions under action/actionName are supported. That isn't an issue. You can take a look into the examples.
I only removed the part that you can call /action. I don't know, if that ever worked. When I tried it, it broke everything. Since it is also not important, I just removed it.

Copy link
Member

Choose a reason for hiding this comment

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

Since it is also not important

If you remove the /actions endpoint then there is no way of getting a list of all actions currently running on the device. It might not be important to you, but it's not necessary to remove this feature in order for the Thing Description to be W3C compliant. I even added a queryallactions operation to the W3C Thing Description specification specifically for this purpose.


void handleThingEventsGet(AsyncWebServerRequest *request,
void handleThingEventsGet(AsyncWebServerRequest *request,
ThingDevice *device) {
if (!verifyHost(request)) {
return;
Expand Down
81 changes: 35 additions & 46 deletions Thing.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,7 @@ class ThingActionObject {
JsonObject data = obj.createNestedObject(name);

JsonObject actionObj = actionRequest->as<JsonObject>();
JsonObject inner = actionObj[name];
data["input"] = inner["input"];
data["input"] = actionObj;

data["status"] = status;
data["timeRequested"] = timeRequested;
Expand All @@ -126,8 +125,7 @@ class ThingActionObject {
setStatus("pending");

JsonObject actionObj = actionRequest->as<JsonObject>();
JsonObject inner = actionObj[name];
start_fn(inner["input"]);
start_fn(actionObj);

finish();
}
Expand Down Expand Up @@ -197,7 +195,7 @@ class ThingAction {
// 2.11 Action object: A links array (An array of Link objects linking
// to one or more representations of an Action resource, each with an
// implied default rel=action.)
JsonArray inline_links = obj.createNestedArray("links");
JsonArray inline_links = obj.createNestedArray("forms");
Copy link
Member

Choose a reason for hiding this comment

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

If I've understood correctly, you should also rename variable names from "links" to "forms".

Copy link
Author

Choose a reason for hiding this comment

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

Ohh well, I guess I forgot to cherry pick that commit. Will do.

JsonObject inline_links_prop = inline_links.createNestedObject();
inline_links_prop["href"] = "/things/" + deviceId + "/actions/" + id;
}
Expand Down Expand Up @@ -297,7 +295,7 @@ class ThingItem {
// 2.9 Property object: A links array (An array of Link objects linking
// to one or more representations of a Property resource, each with an
// implied default rel=property.)
JsonArray inline_links = obj.createNestedArray("links");
JsonArray inline_links = obj.createNestedArray("forms");
Copy link
Member

Choose a reason for hiding this comment

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

Same here, the variable names should be renamed.

JsonObject inline_links_prop = inline_links.createNestedObject();
inline_links_prop["href"] =
"/things/" + deviceId + "/" + resourceType + "/" + id;
Expand Down Expand Up @@ -460,8 +458,6 @@ class ThingEventObject {
data["data"] = *this->getValue().string;
break;
}

data["timestamp"] = timestamp;
Comment on lines -463 to -464
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 removing the timestamp?

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 was never a real timestamp. So, I don't see any value in keeping it, if it only reflects the time since bootup or something random. I don't remember anymore. It was just not the real time.

Copy link
Member

@benfrancis benfrancis Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps the clock is wrong on your device, I don't know.

But the timestamp is still useful for determining the order of events, even if the time is not correct. See w3c/wot-profile#100

}
};

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

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 •
edited
Loading

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

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.

descr["title"] = this->title;
descr["@context"] = "https://webthings.io/schemas";

JsonArray context = descr.createNestedArray("@context");
context.add("https://www.w3.org/2019/wot/td/v1");
Comment on lines +697 to +699
Copy link
Member

@benfrancis benfrancis Oct 14, 2021 •
edited
Loading

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.

It isn't JSON-LD compliant.
https://json-ld.org/playground/#startTab=tab-expanded&json-ld=%7B%22%40context%22%3A%5B%22https%3A%2F%2Fwebthings.io%2Fschemas%2F%22%5D%7D

Copy link
Member

@benfrancis benfrancis Oct 14, 2021 •
edited
Loading

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.

Copy link

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 •
edited
Loading

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.

Copy link

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 •
edited
Loading

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.


if (this->description != "") {
descr["description"] = this->description;
Expand All @@ -715,7 +713,8 @@ class ThingDevice {
descr.createNestedObject("securityDefinitions");
JsonObject nosecSc = securityDefinitions.createNestedObject("nosec_sc");
nosecSc["scheme"] = "nosec";
descr["security"] = "nosec_sc";
JsonArray securityJson = descr.createNestedArray("security");
securityJson.add("nosec_sc");
Comment on lines +716 to +717
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.


JsonArray typeJson = descr.createNestedArray("@type");
const char **type = this->type;
Expand All @@ -724,43 +723,33 @@ class ThingDevice {
type++;
}

JsonArray links = descr.createNestedArray("links");
{
JsonObject links_prop = links.createNestedObject();
links_prop["rel"] = "properties";
links_prop["href"] = "/things/" + this->id + "/properties";
}

{
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";
}
Comment on lines -734 to -744
Copy link
Member

@benfrancis benfrancis Oct 14, 2021 •
edited
Loading

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 •
edited
Loading

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


#ifndef WITHOUT_WS
{
JsonObject links_prop = links.createNestedObject();
links_prop["rel"] = "alternate";

if (port != 80) {
char buffer[33];
itoa(port, buffer, 10);
links_prop["href"] =
"ws://" + ip + ":" + buffer + "/things/" + this->id;
} else {
links_prop["href"] = "ws://" + ip + "/things/" + this->id;
}
}
#endif

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.

JsonArray context = forms_prop.createNestedArray("op");
context.add("readallproperties");
context.add("writeallproperties");
forms_prop["href"] = "/things/" + this->id + "/properties";


#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


if (port != 80) {
char buffer[33];
itoa(port, buffer, 10);
forms_prop["href"] =
"ws://" + ip + ":" + buffer + "/things/" + this->id;
} else {
forms_prop["href"] = "ws://" + ip + "/things/" + this->id;
}
}
#endif

JsonObject properties = descr.createNestedObject("properties");
while (property != nullptr) {
JsonObject obj = properties.createNestedObject(property->id);
Expand Down
26 changes: 2 additions & 24 deletions WiFi101WebThingAdapter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,30 +308,8 @@ class WebThingAdapter {
handleError();
}
return;
} 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 {
Comment on lines -311 to -334
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.

}
} else {
ThingProperty *property = device->firstProperty;
while (property != nullptr) {
String propertyBase = deviceBase + "/properties/" + property->id;
Expand Down