Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add device profile information #139

Merged
merged 3 commits into from Jun 11, 2020

Conversation

iain-anderson
Copy link
Member

@iain-anderson iain-anderson commented May 1, 2020

Signed-off-by: Iain Anderson iain@iotechsys.com

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number:

What has been updated?

Adds documentation for device profiles. I have removed the block diagram of the profile as it did not seem helpful.

Are there any specific instructions or things that should be known prior to reviewing?

Other information

Copy link
Member

@jpwhitemn jpwhitemn left a comment

Choose a reason for hiding this comment

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

@iain-anderson - some good stuff here. Some suggestions based on some prior documentation way back when. Use or lose as you see fit.

The profile contains various identification information. The `Name` field is required and must be unique in an EdgeX deployment. Other fields are optional and provide additional details:

* Description
* Manufacturer
Copy link
Member

Choose a reason for hiding this comment

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

Manufacturer and model are, today, relatively arbitrary. We envisioned a time when multiple manufactures (or multiple models by the same manufacturer) followed the same device profile, in which case these additional tags can help users in search for particular device profile.

* Description
* Manufacturer
* Model
* Labels
Copy link
Member

Choose a reason for hiding this comment

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

Labels just provided a way to organize or categorize the various profiles (envisioning a system where EdgeX manages all sorts of devices and the UI in charge needs some better way to organize them).

DeviceResources
---------------

A deviceResource specifies a value within a device that may be read from or
Copy link
Member

Choose a reason for hiding this comment

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

The term value is a bit ambigous here. Should we say "...specifies a sensor value or simply value ..."?

The device service allows access to deviceResources via its `device`
REST endpoint.

The `Attributes` in a deviceResource are the device-service-specific parameters
Copy link
Member

Choose a reason for hiding this comment

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

In some of our original documentation we said:
Attributes are typically “inward facing”. That is, attributes are used by the device service and provide details about how the device service can speak to the device to either get or set some of its values. In other words, attributes are detail protocol and/or device specific information and inform the device service how to communication with the sensor to get (or set) the deviceResource (aka the value) of interest.

whereas a Bluetooth device service could use a UUID to identify a value.

The `Properties` of a deviceResource describe the value. Each device resource
is given two properties, `value` and `units`. The following fields are
Copy link
Member

Choose a reason for hiding this comment

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

The 'value' property gives more detail about the value collected or set; such as the type of the data collected, whether the value can be read, written or both. The 'units' property gives more detail about the unit of measure associated with the value. The unit of measure can also have a type (although it is most often just a String), a default value, whether the unit can be read, written or both (although it is most often set to read only).

* offset - a value to be added to a reading before it is returned.
* mask - a binary mask which will be applied to an integer reading.
* shift - a number of bits by which an integer reading will be shifted right.

Copy link
Member

Choose a reason for hiding this comment

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

As type is the only "required" property, should we indicate that the others are device service specific and what will happen if you provide a property that isn't used by the device service?

Copy link
Member Author

Choose a reason for hiding this comment

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

The others are handled within the SDK - they should work for any device service


DeviceCommands define access to reads and writes for multiple simultaneous
values. Each named deviceCommand should contain a number of get and/or set
`resourceOperations`, describing the read or write respectively.
Copy link
Member

Choose a reason for hiding this comment

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

think an example would be helpful here (example: a thermostat may provide temp and humidity deviceResources, but a getThermoValues can provide a read of both).

Also, can a device command do both a read and a write simultaneously? Should we specify this is homogeneous (reads or writes)?

DeviceCommands
--------------

DeviceCommands define access to reads and writes for multiple simultaneous
Copy link
Member

Choose a reason for hiding this comment

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

isn't really access to device resources? or at least via device resources. The connection between commands and device resources isn't made.


The device service allows access to deviceCommands via the `device` REST
endpoint. If a deviceCommand and deviceResource have the same name, it will be
the deviceCommand which is available.
Copy link
Member

Choose a reason for hiding this comment

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

Interesting... so a deviceResource of the same name is never directly accessible. It is only there to service as an underlying resource to a command?

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH I think we should discourage deviceCommands and deviceResources with the same name

endpoint. If a deviceCommand and deviceResource have the same name, it will be
the deviceCommand which is available.

CoreCommands
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be helpful to specify where/why there might be a resource operation but not a command.

@iain-anderson iain-anderson marked this pull request as ready for review May 6, 2020 12:42
@iain-anderson
Copy link
Member Author

Updated in light of Cloud's and Jim's comments

Identification
--------------

The profile contains various identification fields. The `Name` field is required and must be unique in an EdgeX deployment. Other fields are optional - they are not used in EdgeX but may be populated for informational purposes:

Choose a reason for hiding this comment

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

If the Name field must be unique, we need to define what happens when EdgeX encounters a duplicate name. This way, if someone inadvertently does duplicates a name, the error signature can be easily recognized.

Choose a reason for hiding this comment

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

Are optional fields absolutely not used? By this I mean, these fields in the device profile are never even read by the device service, let alone stored in Core Metadata or in Core Data, and furthermore, are not "visible," e.g. retrievable by some RESTful API somewhere in EdgeX. If that is the case, then these fields should not even be in the document.

Otherwise, we need to be clearer on how they may or may not be populated and how they are visible.

Keep in mind, any data that enters "EdgeX" can and will be used by someone's "EdgeX."

Copy link
Member Author

Choose a reason for hiding this comment

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

Clarified that the "optional" fields are not used by device services

Comment on lines 90 to 91
Get | GetCommand | N | At least one of Get and Put must be present
Put | PutCommand | N | At least one of Get and Put must be present

Choose a reason for hiding this comment

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

The Notes field disagrees/conflicts with the "Required?" field: "Required?" field states optional, but the Notes fields says that they are required.

Copy link
Member Author

Choose a reason for hiding this comment

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

Corrected


Field Name | Type | Required? | Notes
:--- | :--- | :--- | :---
Name | String | Y | Must be unique in this profile. May have the same name as a DeviceResource but this will make the DeviceResource not accessible individually

Choose a reason for hiding this comment

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

This is odd behavior which will trip up some users. We need to either explain why one would not want a DeviceResource to be individually accessible or disallow name collisions between ProfileResource and DeviceResource.

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted that using the same name is only really useful if you want to apply Mappings on the resource.


Field Name | Type | Required? | Notes
:--- | :--- | :--- | :---
Path | String | Y | Must be `/api/v1/device/{deviceId}/XXX` where XXX is the name of the command

Choose a reason for hiding this comment

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

The constraints on XXX spelled out above (e.g. must be a deviceResource or a deviceCommand found within this device profile) should be spelled out here as well.

Choose a reason for hiding this comment

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

Will "XXX" be escaped properly for HTTP or must the name be pre-escaped? e.g. "%20" or " ".


Field Name | Type | Required? | Notes
:--- | :--- | :--- | :---
Path | String | Y | Must be `/api/v1/device/{deviceId}/XXX` where XXX is the name of the command

Choose a reason for hiding this comment

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

Same comments about "XXX" as with the GET.

The device service allows access to deviceResources via its `device`
REST endpoint.

The `Attributes` in a deviceResource are the device-service-specific parameters

Choose a reason for hiding this comment

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

Do we have documentation about the supported/required Attributes for each of the device services we offer? As this is written, I'm left scratching my head wondering what an Attribute is and how to use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We required for new device services that the attributes were documented, see for example https://github.com/edgexfoundry/device-bacnet-c/blob/master/docs/deviceresource_specification.txt
We need to check that all of the existing services have this information

Signed-off-by: Iain Anderson <iain@iotechsys.com>
Signed-off-by: Iain Anderson <iain@iotechsys.com>
Copy link
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

LGTM

@jpwhitemn jpwhitemn merged commit 789c783 into edgexfoundry:master Jun 11, 2020
edgex-jenkins added a commit that referenced this pull request Jun 11, 2020
Signed-off-by: edgex-jenkins <collab-it+edgex@linuxfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants