specification icon indicating copy to clipboard operation
specification copied to clipboard

RFC 0010: Actions (PUT/POST)

Open sbender9 opened this issue 7 years ago • 42 comments

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

PUT http://localhost:3000/signalk/v1/api/vessels/self/steering/autopilot/target/headingTrue
{
  "value" = 1.52,
  "source": "actisense.204",
}

via a delta

{
  "context": "vessels.self",
  "put": {
    "path": "steering.autopilot.target.headingTrue",
    "source": "actisense.204",
    "value": 1.52
  }
}

The response to a request to change a value

The possible responses the server can make to this request.

  • Permission denied
  • The request is not supported
  • There was an error processing the request
  • The request was processed and the value has been changed
  • The request has been received and is being worked on asynchronously

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:

{
  "state": "COMPLETED",
  "message": "Unable to reach device"
}

HTTP Response to a successful PUT request

HTTP response code 200 (OK) JSON response body:

{
  "state": "COMPLETED",
}

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:

{
  "state":"PENDING",
  "action": {
    "id":12567,
    "href": "/signalk/v1/api/actions/12567"
   }
}

Response to /signalk/v1/api/actions/12567 when the request has completed successfully

{
   "context" : "vessels.self",
   "path" : "steering.autopilot.target.headingTrue",
   "source": "actisense.204",
   "user": "[email protected]",
   "requestedValue" : 1.57,
   "startTime" : "2018-02-27T20:59:21.868Z",
   "id" : 12567,
   "endTime" : "2018-02-27T20:59:41.871Z",
   "state": "COMPLETED"
   "result" : "SUCCESS"
}

Response to /signalk/v1/api/actions/12567 when the request has failed

{
   "context" : "vessels.self",
   "path" : "steering.autopilot.target.headingTrue",
   "source": "actisense.204",
   "user": "[email protected]",
   "requestedValue" : 1.57,
   "startTime" : "2018-02-27T20:59:21.868Z",
   "id" : 12567,
   "endTime" : "2018-02-27T20:59:41.871Z",
   "state": "COMPLETED"
   "result" : "FAILURE",
   "message": "Unable to reach device"
}

Response to /signalk/v1/api/actions/12567 when the request is pending

(note that percentComplete is optional)

{
   "context" : "vessels.self",
   "path" : "steering.autopilot.target.headingTrue",
   "source": "actisense.204",
   "user": "[email protected]",
   "requestedValue" : 1.57,
   "startTime" : "2018-02-27T20:59:21.868Z",
   "id" : 12567,
   "state": "PENDING",
   "percentComplete": 0.45
}

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.

sbender9 avatar Feb 28 '18 15:02 sbender9

This is currently prototyped in node server at https://github.com/SignalK/signalk-server-node/tree/put-support

sbender9 avatar Feb 28 '18 15:02 sbender9

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!

mpvader avatar Mar 01 '18 07:03 mpvader

I’m impressed, looks like a good spec to me.

mpvader avatar Mar 01 '18 07:03 mpvader

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"
      }
    }
  }
}

bkp7 avatar Mar 01 '18 22:03 bkp7

I’m not proposing that the target information go in to the schema at all.

sbender9 avatar Mar 02 '18 02:03 sbender9

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.

sbender9 avatar Mar 02 '18 02:03 sbender9

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

fabdrol avatar Mar 02 '18 08:03 fabdrol

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
}}}}

bkp7 avatar Mar 02 '18 09:03 bkp7

You're misinterpreting the word targethere. 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 avatar Mar 02 '18 10:03 tkurki

@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.

bkp7 avatar Mar 02 '18 11:03 bkp7

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.

sbender9 avatar Mar 02 '18 15:03 sbender9

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.

tkurki avatar Mar 02 '18 15:03 tkurki

Or maybe more accurately: the measurement is not the control mechanism, as with heading.

tkurki avatar Mar 02 '18 15:03 tkurki

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.

bkp7 avatar Mar 02 '18 15:03 bkp7

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.

tkurki avatar Mar 02 '18 16:03 tkurki

For what it's worth, I've tested the branch sbender made and it works great. :) Makes sense to me.

mxtommy avatar Mar 04 '18 02:03 mxtommy

@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.

sbender9 avatar Mar 04 '18 03:03 sbender9

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.

mxtommy avatar Mar 04 '18 04:03 mxtommy

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

  • Everything worked (200)
  • The application did something wrong (400)
  • The API did something wrong (500)

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.

tkurki avatar Mar 04 '18 06:03 tkurki

I've updated to specify 400, 502 and 504 for error responses.

sbender9 avatar Mar 05 '18 01:03 sbender9

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": "[email protected]",
     "requestedValue" : 1.57,
     "start-time" : "2018-02-27T20:59:21.868Z",
     "id" : 12567,
     "client-id": 22,
     "state": "PENDING",
     "percentComplete": 0.45
   }
}

Thoughts?

sbender9 avatar Mar 05 '18 01:03 sbender9

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.

tkurki avatar Mar 05 '18 18:03 tkurki

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:

  • security requirements
  • whether the action should be directed at the master or slave server
  • what the purpose/implications of "user" are, which is a new concept in the Schema.

bkp7 avatar Mar 06 '18 11:03 bkp7

@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 avatar Mar 06 '18 11:03 fabdrol

@fabdrol I've updated to use camelCase

sbender9 avatar Mar 07 '18 14:03 sbender9

@bkp7

  • security will be addressed in an upcoming RFC which will cover security as a whole.
  • I think master/slave should be a server implementation detail. The client should not have to worry about it.
  • I guess I could remove user for now, but it will be there once the security RFC is completed

sbender9 avatar Mar 07 '18 14:03 sbender9

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 avatar Mar 07 '18 14:03 sbender9

@sbender9 thanks for the info

  • really pleased to hear about a security RFC coming
  • I take it from your comment re the slave/master that you envisage the client POSTing (or PUTting) to the Master and it in turn passes the command down to the Slave and the response back up again?

Also, I think you need to add source to the messaging in order to identify which value is required to be updated.

bkp7 avatar Mar 07 '18 14:03 bkp7

@sbender9 I'm for PUT (or PATCH) because, to me, POST means create something new where PUT/PATCH means update something that's there.

fabdrol avatar Mar 07 '18 15:03 fabdrol

My vote is for PUT as well.

timmathews avatar Mar 07 '18 15:03 timmathews