feat: Add ADR for device functions #644

iain-anderson · 2021-11-29T14:51:03Z

Signed-off-by: Iain Anderson [email protected]

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-docs/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

Changes have been rendered and validated locally using mkdocs-material (see edgex-docs README)

ghost · 2021-11-30T00:32:09Z

docs_src/design/adr/device-service/0020-invoking-functions.md

+          {
+            "name": "Parameter name",
+            "description": "description of what the parameter controls",
+            "type": "Any of the usual EdgeX data types"


Here are two fields on device function parameters to consider:

defaultValue (optional): Contains a default value if the parameter isn't supplied.

range (optional): min/max values for numeric types.

Added these (but used maximum / minimum for consistency with device resources)

Min/max makes sense. Thanks.

cloudxxx8

LGTM

ghost · 2021-12-03T22:33:21Z

docs_src/design/adr/device-service/0020-invoking-functions.md

+    {
+      "name": "Name by which the function is accessed",
+      "description": "Readable description of the function",
+      "attributes": { device-specific parameters which select this function },


I'm not clear what attributes are...are they device resources?

They're like the attributes in device resources - they tell the device service implementation how to call the required function
This could be something like a pathname, or a register specification and may include a mapping of the named parameters to some device specific identifier

ghost · 2021-12-03T22:34:26Z

docs_src/design/adr/device-service/0020-invoking-functions.md

+            "type": "Any of the usual EdgeX data types"
+          }
+        ],
+        "out":


Does "out" mean return value? I see (from below)...it does mean return value.

Added comment to clarify

I think "input" and "output" are clearer.

Or maybe instead of a single "parameters" attribute with "in/input" and "out/output", it should be a "parameters" attribute containing an array of function parameters and a separate "returns" attribute with an array of return values...like a Command Response, but for functions.

"parameters": [ { "name": "Parameter name", "description": "(optional) description of what the parameter controls", "type": "Any of the usual EdgeX data types", "defaultValue": "(optional) value to use if param is not supplied", "maximum": "(optional) for numerics, maximum allowed value", "minimum": "(optional) for numerics, minimum allowed value" } ] } "returns": [ { "name": "Name of returned value", "description": "(optional) description of what the value indicates", "type": "Any of the usual EdgeX data types" } ] }

Applied the above but used returnValues

iain-anderson · 2021-12-06T15:06:17Z

Note from Device Service WG:
It may be better to support asynchronous operation. The initial request would if successful return a request ID, and a second endpoint would be used to query the status of a request using this ID

farshidtz

The underlying issue seem to be a limitation on how profiles are defined, rather than what the REST API allows. I suggest separating the two topics, even if they are currently technically coupled. A REST API can be modeled to receive a request and invoke the appropriate operation, whether it is reading/setting of resources or invoking commands/functions. What is important is that REST semantics (method, path, status code, headers, etc) map to the underlying operation.

Overloading the REST API to expose a dedicate POST endpoint for direct function calls, without any control on what the function does, is very similar to SOAP over HTTP: https://en.wikipedia.org/wiki/SOAP#Example_message_(encapsulated_in_HTTP)

I believe if we have a list of requirements on what functions need to be called, we can try to model them as REST->resources/commands.

farshidtz · 2021-12-07T12:12:52Z

docs_src/design/adr/device-service/0020-invoking-functions.md

+            "type": "Any of the usual EdgeX data types"
+          }
+        ],
+        "out":


I think "input" and "output" are clearer.

farshidtz · 2021-12-07T12:16:16Z

docs_src/design/adr/device-service/0020-invoking-functions.md

+
+```
+{
+  "deviceFunctions":


Won't this be confusing since there is also a "deviceCommands" field?

Examples could include perform a self-test, go into standby mode for an hour, invalidate an access key.

These all sound like commands to me.

I agree that it's a bit confusing. As it stands now, DeviceCommands define access to reads and writes for multiple simultaneous device resources whereas DeviceFunctions don't have anything to do with device resources. Maybe DeviceCommands should be DeviceResourceCommands.

Personally think DeviceCommands should be ResourceGroups but we are where we are

farshidtz · 2021-12-07T12:28:23Z

docs_src/design/adr/device-service/0020-invoking-functions.md

+
+`api/v2/device-funtion/<device-name>/<function-name>`
+
+This shold accept POST requests with parameters sent in a JSON (or CBOR) payload


The HTTP methods in REST have dedicated semantics (unlike SOAP). The function may do operations that are beyond the defined semantics of POST (e.g. removal of resources).

There are many references for REST design, here is one that is specific to IoT: https://tools.ietf.org/id/draft-keranen-t2trg-rest-iot-05.html#rfc.section.3.5.3

iain-anderson · 2021-12-15T15:25:15Z

Re-expressed access to the functionality in MessageBus terms rather than REST

bnevis-i · 2021-12-16T23:03:46Z

docs_src/design/adr/device-service/0021-invoking-functions.md

+Note: the `attributes` structure is analagous to `attributes` in a `deviceResource`. Each device service should document and implement a scheme of required attributes that will allow for selection of the relevant funtion. The function's `name` is intended for UI and logging purposes and should not be used for actual function selection on the device.
+
+**Define MessageBus topics on which function call requests and replies are to be made**
+
+`edgex/function-calls/device/[profile-name]/[device-name]/[function-name]`


On line 61 it is stated that name is only used for UI and logging, but then on line 65, name is used as part of the MQTT topic name.

removed comment on function names

bnevis-i · 2021-12-16T23:06:24Z

docs_src/design/adr/device-service/0021-invoking-functions.md

+
+`edgex/function-responses/device/[profile-name]/[device-name]/[function-name]`
+
+The device service will provide responses to function calls on this topic. The payload will be


Are messages sent with guaranteed delivery, or can messages be thrown away if the device is not there to receive them?

QoS is configurable in the main messagebus options

bnevis-i · 2021-12-16T23:07:24Z

docs_src/design/adr/device-service/0021-invoking-functions.md

+}
+```
+
+or if a call fails


What if a response is never sent or received?

bnevis-i · 2021-12-16T23:07:58Z

docs_src/design/adr/device-service/0021-invoking-functions.md

+|--------|--------
+| 0      | The operation was successful
+| 1      | Parameters were missing, out of range or non-parsable
+| 2      | The Device is DOWN or DISABLED


What component is determining that a device is down or disabled and sending the response?

bnevis-i · 2021-12-16T23:08:28Z

docs_src/design/adr/device-service/0021-invoking-functions.md

+| 0      | The operation was successful
+| 1      | Parameters were missing, out of range or non-parsable
+| 2      | The Device is DOWN or DISABLED
+| 3      | No such device or function


Ditto. Who is implementing the dead letter functionality?

Added text on the SDKs responsibilities

bnevis-i · 2021-12-16T23:10:38Z

docs_src/design/adr/device-service/0021-invoking-functions.md

+{
+  requestId: "184b894f-a7b7-4d6c-b400-99961d462419",
+  parameters: { (a map of parameter values keyed by parameter name) }
+}
+```


Perhaps we can use an existing specification and add the requestId to it?

https://en.wikipedia.org/wiki/JSON-RPC#Version_2.0
https://www.jsonrpc.org/specification

+1000000

We should at least explore prior art before we invent something new...

Have changed this so that it is using the same style of message body as proposed for N/S messaging

bnevis-i · 2021-12-20T15:17:26Z

Note from Device Service WG:
It may be better to support asynchronous operation. The initial request would if successful return a request ID, and a second endpoint would be used to query the status of a request using this ID

Is the protocol not already defined to operate asynchronously? Fire-and-forget with a requestId in one direction, and a fire-and-forget response in the other with the same requestId?

iain-anderson · 2021-12-20T15:25:47Z

Note from Device Service WG:
It may be better to support asynchronous operation. The initial request would if successful return a request ID, and a second endpoint would be used to query the status of a request using this ID

Is the protocol not already defined to operate asynchronously? Fire-and-forget with a requestId in one direction, and a fire-and-forget response in the other with the same requestId?

It is now :) Previously the ADR specified a REST endpoint for invoking functions

TomBrennan-Eaton

It seems to me that this could be aligned with the ADR for North-South command messaging. Wouldn't the command service be normally in the loop when these functions are actuated?
The requestId here is the correlation-id there.

Signed-off-by: Iain Anderson <[email protected]>

Formatting fixes and language clarification Add parameter range limits and defaults Signed-off-by: Iain Anderson <[email protected]>

Specify asynchronous operation via MessageBus rather than synchronous REST Signed-off-by: Iain Anderson <[email protected]>

Clarify parameter specification, add error table, move ADR to no. 0021 Signed-off-by: Iain Anderson <[email protected]>

Remove contradictory text on function names, add SDK responsibilities Signed-off-by: Iain Anderson <[email protected]>

tonyespy

The approach in this ADR is certainly one way to solve the problem of making RPC-like calls to device/sensors managed by EdgeX device services. Although it's not called out explicitly, it seems this version of the ADR limits this functionality to message bus usage only (i.e. you can't invoke device functions via REST), so I'd suggest making that more obvious, and given the overlap between this ADR and the North/South Messaging ADR, I would add a References section with a link it. Also, as I explained in my review of that ADR, I feel like we don't have our story straight with respect to how we would provide similar functionality to the API Gateway with respect to access control of functionality provided via message bus.

Although I didn't comment on the initial version, it proposed a similar approach over REST. Should this ADR mention that this approach was examined and rejected for some reason?

I also recall suggesting that we re-consider gRPC, especially given the fact that it's become an (optional) requirement for services that will leverage the new Spiffe/Spire-based token provider (per the Delayed Services ADR), and given Kong's support for proxying gRPC. Did you give any serious consideration to this approach?

tonyespy · 2022-01-14T18:19:00Z

docs_src/design/adr/device-service/0021-invoking-functions.md

+        {
+          "name": "Parameter name",
+          "description": "(optional) description of what the parameter controls",
+          "type": "Any of the usual EdgeX data types",


Does this include array, binary, and object types?

Don't see why not

tonyespy · 2022-01-14T18:24:50Z

docs_src/design/adr/device-service/0021-invoking-functions.md

+}
+```
+
+Note: the `attributes` structure is analagous to `attributes` in a `deviceResource`. Each device service should document and implement a scheme of required attributes that will allow for selection of the relevant funtion.


I'm not sure what the purpose of attributes are in this context? If extra data is required to make a function call, isn't this the purpose of parameters? I also don't understand what you mean by "a scheme of required attributes that will allow for selection of relevant function"?

sp: funtion

I

tonyespy · 2022-01-14T18:29:27Z

docs_src/design/adr/device-service/0021-invoking-functions.md

+
+## Decision
+
+**Add a new section to device profiles describing functions**


It seems odd to describe functions in device profiles. Typically what we're trying to accomplish is allow use of some sort of functional protocol to communicate with devices, and the protocol itself doesn't change for different instances of the class of devices supported by the device service. For instance, the original device-camera service only supports ONVIF (which is a SOAP-based protocol), likewise our RFID device service only supports LLRP. I suppose if a service supported more than one function-based protocol, then defining the functions in profiles makes sense, but I think this is the exception, not the norm.

Yes for protocols like ONVIF which define available operations at the protocol level. But for eg a Modbus device where you may have a function "Reset latching alarms" which is invoked by writing a zero into register 0x16, that ought to be defined in a device profile.

tonyespy · 2022-01-14T18:35:01Z

docs_src/design/adr/device-service/0021-invoking-functions.md

+
+**Define MessageBus topics on which function call requests and replies are to be made**
+
+`edgex/function-calls/device/[profile-name]/[device-name]/[function-name]`


Why include the profile name? It's not included as a parameter in the standard core/device services command API.

Also would it make sense to model our topic names using the conventions we use for REST which prefix parameters with a name (e.g. edgex/function-calls/device/[device-name]/function/[function-name])?

Should the topic also somehow indicate that this message is destined for core command and/or a specific device instance?

Are these functions meant to proxied by core command (per the North/South Messaging ADR)?

Have changed these to reflect the content of the N/S messaging ADR

tonyespy · 2022-01-14T18:36:50Z

docs_src/design/adr/device-service/0021-invoking-functions.md

+{
+  requestId: "184b894f-a7b7-4d6c-b400-99961d462419",
+  parameters: { (a map of parameter values keyed by parameter name) }
+}
+```


+1000000

We should at least explore prior art before we invent something new...

tonyespy · 2022-01-14T18:37:50Z

docs_src/design/adr/device-service/0021-invoking-functions.md

+| Status | Meaning
+|--------|--------
+| 0      | The operation was successful
+| 1      | Parameters were missing, out of range or non-parsable


Does this include other errors with the request format?

Added a separate code for other format errors

iain-anderson · 2022-01-17T13:37:19Z

It seems to me that this could be aligned with the ADR for North-South command messaging. Wouldn't the command service be normally in the loop when these functions are actuated? The requestId here is the correlation-id there.

I've changed the topic naming and message format to be more aligned with the N-S messaging ADR. Request and correlation IDs are still separate; the lifetime of a correlation id may span multiple requests

Align with north-south messaging ADR (edgexfoundry#23) Signed-off-by: Iain Anderson <[email protected]>

ghost reviewed Nov 30, 2021

View reviewed changes

cloudxxx8 previously approved these changes Nov 30, 2021

View reviewed changes

ghost reviewed Dec 3, 2021

View reviewed changes

iain-anderson dismissed cloudxxx8’s stale review via e36fc1a December 6, 2021 14:59

farshidtz reviewed Dec 7, 2021

View reviewed changes

iain-anderson force-pushed the issue-643 branch from 7448e28 to 159018f Compare December 7, 2021 16:55

jpwhitemn added ADR Architecture Decision Record documentation kamakura labels Dec 14, 2021

iain-anderson force-pushed the issue-643 branch from 550c280 to d447dba Compare December 15, 2021 15:18

iain-anderson marked this pull request as ready for review December 15, 2021 15:25

bnevis-i reviewed Dec 16, 2021

View reviewed changes

TomBrennan-Eaton reviewed Jan 6, 2022

View reviewed changes

iain-anderson added 5 commits January 13, 2022 17:12

feat: Add ADR for device functions

d35d610

Signed-off-by: Iain Anderson <[email protected]>

feat: Device functions ADR

2fc4be1

Formatting fixes and language clarification Add parameter range limits and defaults Signed-off-by: Iain Anderson <[email protected]>

feat: Device functions ADR

639fb93

Specify asynchronous operation via MessageBus rather than synchronous REST Signed-off-by: Iain Anderson <[email protected]>

feat: Device functions ADR

c5266fd

Clarify parameter specification, add error table, move ADR to no. 0021 Signed-off-by: Iain Anderson <[email protected]>

feat: Device functions ADR

0d6d03b

Remove contradictory text on function names, add SDK responsibilities Signed-off-by: Iain Anderson <[email protected]>

iain-anderson force-pushed the issue-643 branch from ba10442 to 0d6d03b Compare January 13, 2022 17:12

tonyespy requested changes Jan 14, 2022

View reviewed changes

feat: Device functions ADR

8f0ee4b

Align with north-south messaging ADR (edgexfoundry#23) Signed-off-by: Iain Anderson <[email protected]>

iain-anderson force-pushed the issue-643 branch from 7ab8194 to 8f0ee4b Compare January 17, 2022 13:42

jpwhitemn removed the kamakura label Apr 29, 2022

jpwhitemn added this to the Levski milestone Apr 29, 2022

jpwhitemn added the levski fall 2022 release label Apr 29, 2022

cloudxxx8 mentioned this pull request Jun 15, 2022

Firmware upgrade to end devices through EdgeX (i.e., core-data RedisDB) edgexfoundry/edgex-go#4052

Closed

cloudxxx8 mentioned this pull request Jul 22, 2022

StartStreaming errors not passed to the caller edgexfoundry/device-usb-camera#23

Closed


		`api/v2/device-funtion/<device-name>/<function-name>`

		This shold accept POST requests with parameters sent in a JSON (or CBOR) payload


		`edgex/function-responses/device/[profile-name]/[device-name]/[function-name]`

		The device service will provide responses to function calls on this topic. The payload will be


		## Decision

		Add a new section to device profiles describing functions


		Define MessageBus topics on which function call requests and replies are to be made

		`edgex/function-calls/device/[profile-name]/[device-name]/[function-name]`

feat: Add ADR for device functions #644

Are you sure you want to change the base?

feat: Add ADR for device functions #644

Conversation

iain-anderson commented Nov 29, 2021

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudxxx8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 3, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmocek Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iain-anderson commented Dec 6, 2021

farshidtz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iain-anderson commented Dec 15, 2021

bnevis-i Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnevis-i Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnevis-i commented Dec 20, 2021

iain-anderson commented Dec 20, 2021

TomBrennan-Eaton left a comment

Choose a reason for hiding this comment

tonyespy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iain-anderson commented Jan 17, 2022

ghost Dec 3, 2021 •

edited by ghost

Loading

dmocek Dec 8, 2021 •

edited

Loading

bnevis-i Dec 16, 2021 •

edited

Loading

bnevis-i Dec 16, 2021 •

edited

Loading