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

URI Variable for BACnet #302

Closed
egekorkan opened this issue Oct 4, 2023 · 6 comments
Closed

URI Variable for BACnet #302

egekorkan opened this issue Oct 4, 2023 · 6 comments
Assignees
Labels
bacnet related to bacnet protocol binding

Comments

@egekorkan
Copy link
Contributor

There are a couple of unclear points regarding the use of URI variables in BACnet binding. I think they are all the same issue but let me write them separately for now.

The whole problem seems to originate from the fact that we want to respect the BACnet uri scheme but want to add some input parameters to reading and observing. I think those should be all form terms and the URI Variables section can be removed.

@JoelBender
Copy link

I'm not sure about the sections, but to be more accurate with the standard the service name should be title cased from subscribeCOVProperty to SubscribeCOVProperty just like you have ReadProperty, and while the property identifier is required in the service and therefore should also be a required component in the URI string, having present-value be the default should be fine.

The ASN.1 parameters for the SubscribeCOVProperty Request are issue-confirmed-notifications, lifetime, and cov-increment.

The BACnet URI scheme (Clause Q.8) is fine for most BACnet networks, but doesn't scale across multiple "sites" or BACnet intranets. Something to think about!

@egekorkan egekorkan added the bacnet related to bacnet protocol binding label Oct 5, 2023
@egekorkan
Copy link
Contributor Author

egekorkan commented Oct 16, 2023

So I had some offline chat with @mjkoster and @ektrahm and I will try to provide a PR to address this. The main problem is that we need to provide a variable input to reading, writing observing. That is why the initial design is done with URI Variables but it should be possible to use a BACnet specific terms. The goal is to have the kind of programming possible with the following lines of code (copied from here):

thing.observeProperty("availableResourceLevel", { uriVariables: { id: "water" } });

Here, uriVariables are used since they are in the TD. In the case of BACnet, we want something like:

thing.observeProperty("availableResourceLevel", { parameters: { "bacv:covIncrement": 0.2 } });

So with this in mind, I have three proposals:

  1. A term like that in forms:
  {
    "@context": "https://www.w3.org/2022/wot/td/v1.1",
    "properties": {
      "analog1": {
        "type": "number",
        "readOnly": true,
        "forms": [{
            "op": "observeproperty",
            "href": "bacnet://5/0,1/85",
            "bacv:service": "SubscribeCOVproperty",
            "bacv:covIncrement": {
              "type": "number", "minimum": 0
            }
          }
        ]}}}

The advantage here is that the protocol-specific term is in the forms. Disadvantage is that the developer needs to know that variable and provide it in the code.

  1. A term like that in the affordance level:
  {
    "@context": "https://www.w3.org/2022/wot/td/v1.1",
    "properties": {
      "analog1": {
        "type": "number",
        "readOnly": true,
        "bacv:covIncrement": {
          "type": "number", "minimum": 0
        },
        "forms": [{
            "op": "observeproperty",
            "href": "bacnet://5/0,1/85",
            "bacv:service": "SubscribeCOVproperty"
          }
        ]}}}

The advantage here is that it is more clear to the developer that it is something for their code and not for the driver to pick up. A disadvantage is protocol-specific data "creeping" into affordance level. However, this is not really protocol specific. In the end, it is about specifying which granularity of changes should be pushed by the Thing.

  1. Adding new terms into TD as part of TD 2.0 work. Some very rough design is below:
  {
    "@context": "https://www.w3.org/2022/wot/td/v1.1",
    "properties": {
      "analog1": {
        "type": "number",
        "readOnly": false,
        "additionalParameters": {
          "incrementUpdate": {
            "@type": "bacv:covIncrement",
            "type": "number",
            "minimum": 0
          }
        },
        "forms": [
          {
            "op":"writeproperty",
            "href": "bacnet://5/0,1/85",
            "bacv:service": "WriteProperty",
            "additionalParameters": false
          },
          {
            "op": "observeproperty",
            "href": "bacnet://5/0,1/85",
            "bacv:service": "SubscribeCOVproperty",
            "additionalParameters": true
          }
        ]}}}

I quite like this approach and maybe we can handle this together with other topics around Data Schema to input mapping (raised by @lu-zero). Maybe also writeproperty return values issue: w3c/wot-thing-description#875

@danielpeintner has also provided his opinion and mentioned that option 2 seems nicer for now. My opinion would be to also go with option 2 and move to option 3 later on.

@lu-zero
Copy link
Contributor

lu-zero commented Oct 16, 2023

In general the idea is to have the DataSchema on one side (the affordance data field), and a DataMap on the form:

{
    "@context": "https://www.w3.org/2022/wot/td/v1.1",
    "properties": {
      "analog1": {
        // explicit field to hold a DataSchema
        "data": {
          "type": "number",
          "readOnly": false,
          "minimum": 0,
          ... 
        },
        // another field to hold additional dataschemas array or map
        "additionalData": [{
          "type": "number",
          "readOnly": false,
          "minimum": 0,
        }], 
        "forms": [
          {
            "op":"writeproperty",
            "protocol": "bacnet",
            "href": "bacnet://5/0,1/85",
            "bacv:service": "WriteProperty",
            "map": {"/properties/analog1/data": "default"}, // nothing to map in a special way, should be the default
          },
          {
            "op": "observeproperty",
            "protocol": "bacnet",
            "href": "bacnet://5/0,1/85",
            "bacv:service": "SubscribeCOVproperty",
            "map": { "/properties/analog1/additionalData/0": "bacv:covIncrement" } // the keys of this map are jsonpointers 
          },
          {
            "op": "observeproperty",
            "protocol": "http+sse"
            "href": "https://host/path",
            "map": ...
          }
        ]}}}

@fennibay
Copy link
Contributor

I'd like to clarify briefly why we chose URI variables for command priority and COV increment.

We already put all parameters that are strictly BACnet-internal under the Form. However, there is a need to expose command priority and COV increment to a WoT Consumer that is not BACnet-aware, as they influence the contract between the Thing and the Consumer.

I guess in retrospect we should use different wording that refers to the WoT operations and not BACnet services on the PropertyAffordance:

  • commandPriority -> writePriority (for writeproperty)
  • covIncrement -> observeThreshold (for observeproperty)

Our original intention was to show them also on the href, but we thought that would lead to a longer discussion with ASHRAE/BACnet as these URI parameters are currently not specified in current BACnet URL spec. In practice there is no known implementation in the field that operates with this URI, so it is safe to assume only WoT Consumers will deal with these URI variables. Essentially this is a pragmatic way to proceed on the basis of v1.1and ultimately we should show explicitly how these URI variables the underlying protocol binding.

I agree that we should make the casing consistent. I would propose writePriority and observeThreshold.

Ultimately, actually it's the TM/TD author's decision what to expose in URI variables, we cannot influence it from a protocol binding.

@egekorkan
Copy link
Contributor Author

Call of 15.11:

  • @fennibay The priority and CoV is about the domain and maybe other protocols in the future will support that.
  • @egekorkan and @fennibay If we change the URI Scheme, uriVariables approach can work
  • @mjkoster There can be also fixed priorities or CoV, in that case, the form will contain that (as part of href)
  • @lu-zero uriVariable is an ad-hoc addition to the data schema so we should not use them unless we have to. In 1.1 we can stay ad-hoc this way. We can put a warning about this (i.e. TD.next will change this)

@egekorkan
Copy link
Contributor Author

This is resolved via #318, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bacnet related to bacnet protocol binding
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants