Open sbender9 opened 6 years ago
This is currently prototyped in node server at https://github.com/SignalK/signalk-server-node/tree/put-support
That does not work because displays and devices need to be able to tell the difference between a request to change the state of thing and an update of the current state.
Exactly!
I’m impressed, looks like a good spec to me.
This looks good to me too.
One point though, rather than creating many new leaves with names prepended with target
, would it not make more sense to have the target
property at the same level as the existing value
. ie
{
"context": "vessels.self",
"put": {
"path": "navigation.headingTrue.target",
"value": 1.52
}
}
resulting in:
{
"uuid": "urn:mrn:signalk:uuid:c0d79334-4e25-4245-8892-54e8ccc8021d",
"navigation": {
"headingTrue": {
"value": 1.48,
"$source": "foo.bar",
"timestamp": "2017-08-15T19:00:15.402Z"
"target": {
"value": 1.52,
"$source": "foo.baz",
"timestamp": "2017-08-15T19:00:03.381Z"
}
}
}
}
I’m not proposing that the target information go in to the schema at all.
Oh, I see the confusion, my example uses 'targetHeadingTrue', which is incorrect. (Not sure where I got that) . Updating now...
The paths in the put requests would always be something already in the schema.
Overall: looks very good. One note; Shouldn’t the error response use a non-200 value? The proposed error response will result in a “successful” request in most HTTP clients, which means the code must include an extra inspection of the response body to detect the error, on top of the error handler for HTTP errors. Not DRY
Steering is the only schema which currently has 'target' sections so it is not a good generic example. For all other areas your proposal would require a new 'target' branch or leaf to be added, eg. zone temperature/humidity, all switching,/controls, etc. Given that gauges and displays would want to be able show the current value and the target value I would have thought it better to put the target at the same level as the value rather than in a separate unlinked leaf, so your proposed:
{
"uuid": "urn:mrn:signalk:uuid:c0d79334-4e25-4245-8892-54e8ccc8021d",
"navigation": {
"headingTrue": {
"value": 1.48
}
},
"steering": {
"rudderAngle": {
"value": 0.09
},
"rudderAngleTarget": {
"value": 0.07
},
"autopilot": {
"target": {
"headingTrue": {
"value": 1.52
}}}}}
becomes:
{
"uuid": "urn:mrn:signalk:uuid:c0d79334-4e25-4245-8892-54e8ccc8021d",
"navigation": {
"headingTrue": {
"value": 1.48,
"target": {
"value": 1.52
}
}
},
"steering": {
"rudderAngle": {
"value": 0.09,
"target": {
"value": 0.07
}}}}
You're misinterpreting the word target
here. The example is trying to set the heading that the autopilot is set to hold, whose SK path just happens to contain the word target.
It is not the target of the action and no new paths are added to the data model.
If you want to set for example the temperature your fridge is supposed to hold there willl be no word target in the path.
@tkurki, sorry I don't understand what you mean.
A fridge has both an actual and 'target' temperature. The fridge control unit is responsible for trying to maintain the target temperature.
A vessel has both an actual and 'target' heading. The autopilot is responsible for trying to maintain the target heading.
These 2 cases look identical to me and both need to have the actual and target values somewhere in the model as the actual and target will rarely (if ever) be exactly the same.
This also extends nicely to lighting and switching: A light has an actual dimmed value of 50% and target value of 20%. The lighting control unit is responsible for fading the lights to the target value.
I get what you're saying @bkp7 , but I'm not proposing to change the scheme here in any way.
I think in cases where we think it makes sense to have the 'target' value in the schema, we should add it explicitly. Like we have done for the autopilot target heading.
And we should look to the real world for examples.
It totally makes sense to have a target temperature for a fridge or other thermostats. We would see this on a real world thermostat. Both current and target temps. These both should be in the schema.
I'm not sure it makes sense for a light switch or dimmer. We'd be overly complicating things in this case. If I want to turn a light on, I should just PUT to electrical.switches.anchorLight.state = on. In this case, the only reason to have both current and target in the schema is to give the user an indication that things are not currently in the state that the user requested. I think this protocol deals with that because the UI can show this to the user by displaying an error message if the request fails and by indicating the current state in the UI.
Oh sorry, my mistake, I was misunderstanding what you were saying @bkp7.
But still some points: a vessel has a heading but an autopilot has a target heading. You can even set the target heading without engaging the autopilot. A fridge has a temperature but the fridge thermostat has a set temperature, and there may be just an icebox. Many of the sk paths are not subject to control.
Or maybe more accurately: the measurement is not the control mechanism, as with heading.
OK, I agree that adding explicit target values where they are needed is a way forward.
However, that relies on either maintaining a map between the underlying value and the target, or relying on a strictly enforced naming convention in order to find the associated target.
So for example a display unit wanting data for a compass would subscribe to navigation.headingMagnetic
which would include all metadata to render the compass except for the heading bug, which is found at steering.autopilot.target.headingMagnetic
. How is the display meant to know this? Even if it has the schema there is no logical link between the two.
In my example above I also picked rudder angle because at least these are close together, ie. subscribe to steering.rudderAngle
to get everything needed to render the gauge except for the commanded position which is found at steering.rudderAngleTarget
. This is what I mean by needing to have a strictly enforced naming convention to know to add 'Target' to the path.
In my proposal the commanded/target/bug position would be included in the same place, meaning the display unit does not need to keep a map of values and target, nor does it need the schema. I understood this to be one of the goals for display units? Also the heading bug (autopilot target heading) can be set even if the autopilot is not engaged, but should still be able to be displayed on the compass.
A generic compass will not show steering.autopilot.target.headingMagnetic
. Somebody would have to figure out how they want to render the set point of the autopilot in a compass gauge and during that process say that data is coming from that path.
Same thing for rudder angle: if you build a rudder angle gauge that shows both the actual and target you would define the paths for the data.
If you want to build such associations I suggest an additional layer, as a single grouping will end up in the same discussion as we have repeatedly had with alternator being associated with the engine/propulsion, while electrical would make pretty much equal sense.
This issue is about defining the mechanism how you control something that is controllable, synchronously or asynchronously and taking possible failures into account. I think we can make more progress by sticking to that here.
For what it's worth, I've tested the branch sbender made and it works great. :) Makes sense to me.
@fabdrol on the HTTP response code for errors:
Good article here: http://blog.restcase.com/rest-api-error-codes-101/
I think it makes sense to respond with 200 here because the server got the message successfully and tried to act on it. Something went wrong on the back end, but nothing went wrong at the HTTP protocol level. There's not really an http response code for that. 501 maybe? But that's normally sent when something really bad happens, an unexpected error.
Wont cover all all errors, but for "Unable to reach device" type errors, 504 might be good?
504 Gateway Timeout The server was acting as a gateway or proxy and did not receive a timely response from the upstream server.
I'm with Fabian here: from an API user's perspective it is much more straightforward to check status code for errors, not status code and response body. Also tooling, such as browser developer tools and command line tools work better with error codes.
For synchronous operations 400 (something wrong with the client's request), 502 (something went wrong carrying out the request on the server side), 504 (timeout on the server side trying to carry out the request) cover most cases - something crucial missing here?
The article you linked boils everything down to
The main point is that the client can expect to get 200 or 202 for success and something else for failure. As you said Scott earlier, the http status codes were created in another era for much simpler web page serving.
I've updated to specify 400, 502 and 504 for error responses.
I wonder if I should go ahead and propose a way to do get responses over other protocols like ws, tcp, etc.
I would propose that the put
delta be modified to include a client-id
. This would be a number generated by the client to identify the request and then would be included in responses.
Responses would be a delta like:
{
"context": "vessels.self",
"response": {
"path" : "steering.autopilot.target.headingTrue",
"user": "john@smith.com",
"requestedValue" : 1.57,
"start-time" : "2018-02-27T20:59:21.868Z",
"id" : 12567,
"client-id": 22,
"state": "PENDING",
"percentComplete": 0.45
}
}
Thoughts?
Signal K specification issues have a tendency to stall or end up mired in endless discussion. The http action part is well defined and I don't see anything missing. I think the next step would be to add it to the specification gitbook document as it is, as a the first (yay!) RFC to be included or maybe rewrite it a bit to be more in line with the rest of the docs, as a chapter in the gitbook.
Actions over streaming connections should be handled in a separate RFC. Not proceeding with it is also an option. That would clearly endorse the http method, making Signal K spec that much less complex. Then when real use cases come up that the http method does not cover we can revisit the issue - the spec is a work in progress.
I agree with @tkurki about sticking to just one http method. If you have optional ways to do something every server implementation really needs to implement them all. It's hard to imagine a real word device which can do ws but not http.
Assuming the documentation proceeds please can you include explanations for:
@sbender9 one small note: at the moment you mix camelCase (requestedValue
) with ... "slug case" (?): start-time
. Is there a particular reason for that? If not I'd propose to choose one and stick with it.
@fabdrol I've updated to use camelCase
@bkp7
Any opinions on using PUT verses POST? I think currently I have two votes for POST (@tkurki and @mxtommy) and one vote for PUT (me)
@sbender9 thanks for the info
Also, I think you need to add source to the messaging in order to identify which value is required to be updated.
@sbender9 I'm for PUT (or PATCH) because, to me, POST means create something new
where PUT/PATCH means update something that's there
.
My vote is for PUT as well.
Regarding POST/PUT, I don't actually have a vote per se. I'm torn. PUT would be more "correct", but it adds an additional http request (HTTP OPTIONS) before every "PUT" which can increase the time from the user doing the interaction that caused the PUT, and the PUT getting to where it needs to go. If you're trying to do something as close to "realtime" as possible, it can be annoying. If we want to be correct it should be "PUT". If we want ultimate performance, POST.
(Just to give an example of where performance would be imporant, If using signalk to control a windlass, when you let go of the button, you want the windlass to stop ASAP, not in half a second. Granted that's more of a worst case, OPTIONS requests don't take THAT long usually, but just to show when it would be important :) )
@bkp7 Yes on the master/slave handling.
Agreed on adding source. I think it should be optional.
@mxtommy I don't think same origin PUT requires a preflight OPTIONS request - that is CORS Cross Origin Resource Sharing and is specific to browser usage.
Server's response in case there are multiple sources and source is missing should be defined.
If real-time is important, then interaction should be over WS. In other words, real-time isn’t relevant to HTTP API. Or is that off the table now?
Time delays do matter. It also affects a users' perception of "quality" ie they expect the reaction to an input to be "instant". Is there any merit in specifying ws instead of http? Can anyone envisage a device with controllable elements which can't be made to support ws? Or a client which is incapable of ws?
@tkurki updated to describe what happens when there are multiple sources.
@bkp7 I would think that small devices that use ws to send updates to a server, should not have to also run an HTTP server just for processing a PUT request.
@sbender9, what I meant was could you imagine a server device which was designed to accept updates and could support http but not ws. My point is that perhaps we should use ws exclusively for this functionality and not http at all.
My personal preference is to stick to specifying one way to to do things where ever possible. If ws is lighter weight and faster perhaps that is the way to go?
Http is request-response, has established semantics and using http to do stuff is very very common. Development and debugging tooling and processes are easier for http.
Actions typically affect the real world with real world latencies. The extra latency caused by the OPTIONS request that @mxtommy mentioned is related to CORS requests from browsers, so I don't see that as a huge issue. If it is then choose POST and be done with it.
@bkp7 you are only considering the server. Previously people have argued that ws is not necessarily available on microcontroller network stacks.
I do not think that ws is lighter weight with all the extra baggage it involves. You will have to specify and implement all the mechanisms outlined in this RFC (and a few extra, like request response) over ws on all the languages people want to use to get to the same level that normal http libs bring to the table from the start.
I seriously believe ease of use trumps ultimate performance here.
Let's ditch JSON and go binary, because for some use cases JSON overhead is just too much.
I love ws - just not for this.
I think the next step would be a PR adding this to the specification documentation.
PR to spec at #507
RFC 0010: Actions (PUT/POST)
Summary
We do not currently detail the way to take actions to control things on a boat. There should be a well defined way to do things like turn lights on and off and control an autopilot, etc.
Motivation
This is currently being done in proprietary ways making it difficult for app developers to add support. 'update' deltas are one way this could be done and that has been proposed in the past. That does not work because displays and devices need to be able to tell the difference between a request to change the state of thing and an update of the current state.
Detailed design
There has been some discussion already about the exact methods used to do this. PUT/POST over HTTP and/or put deltas over ws. I'm just going to use the word PUT for now here.
Making a request to change a value
To change a value, a PUT request should be sent via HTTP or using a SignalK 'put' delta.
The "source" field is optional. If a request is sent without the source and their is more than one source for the value, that will result in a 400 HTTP error response.
via http
via a delta
The response to a request to change a value
The possible responses the server can make to this request.
I will cover these for HTTP methods since there is no defined way to do request/response over ws or other protocols.
HTTP response for permission denied
HTTP response code 403 (Forbidden)
HTTP response when the request is not supported
HTTP response code 405 (Method Not Allowed)
HTTP response when there is an error processing the request
HTTP response codes: 400 (something wrong with the client's request) 502 (something went wrong carrying out the request on the server side) 504 (timeout on the server side trying to carry out the request
JSON response body:
HTTP Response to a successful PUT request
HTTP response code 200 (OK) JSON response body:
HTTP Response to a request that is being worked on asynchronously
The response in this case includes an optional
href
that can be used to check the status of the request.HTTP response code 202 (Accepted) JSON response body:
Response to
/signalk/v1/api/actions/12567
when the request has completed successfullyResponse to
/signalk/v1/api/actions/12567
when the request has failedResponse to
/signalk/v1/api/actions/12567
when the request is pending(note that percentComplete is optional)
Unresolved questions
It is not clear how to handle this with protocols like WS, TCP, etc. I propose that at least initially, the client will assume that the request is "PENDING" and watch for a delta update of the value to confirm that it was changed.