feature: 'meta' deltas

[sbender9]

There is currently no way to send meta data via deltas. This PR adds a new type of delta just for meta data.

The deltas would look like this:

{
  updates: [
    {
      meta: [
        {
          "path": "electrical.batteries.1.voltage",
          "value": {
              "displayName": "Starter Battery V"
          }
        }
      ]
    }
  ]
}

TODO

• [x] add documentation

[bkp7]

Just to get some context; I think you are proposing that meta be sent by the server to subscribers over websockets? If so I assume that this would be sent upon initial subscription and then subsequently only if there are any changes to meta data?

Also you propose a new type of delta just for meta data, but should it be completely separate or an optional part of the existing delta format?

I agree that it would be very good to be able to see changes to meta data (warning zones, gauge scale, etc.) immediately pushed out to display devices and would make server/sensor set up significantly more transparent for users.

[sbender9]

Yes. These would get pushed out to clients when the connection is established.

This a new type of delta. Note that it is meta instead of values.

[timmathews]

What's wrong with sending a values delta with the path pointing at the meta object(s) like so?

{
  "context": "vessels.urn:mrn:signalk:uuid:c0d79334-4e25-4245-8892-54e8ccc8021d",
  "updates": [
    {
      "values": [
        {
          "path": "navigation.speedThroughWater.meta",
          "value": { /* meta object */ }
        },
        { /* additional meta values for other paths */ }
      ]
    }
  ]
}

[sbender9]

Then for existing apps/code, you would get navigation.speedThroughWater.meta.value = { /* meta object */ }

Which is not what the spec says.

[timmathews]

I was afraid that might be the case. Which I think means that we cannot update most values under sources or sails via delta at the moment because they don’t place all their values under value

[rob42]

The other case is for properties like a vessels name, flag, port which have no value either. Need a way to send a key/value pair without implicit value treatment at the client. Probably use the same solution for both. eg

{
  updates: [
    {
      attributes: [
        {
         "path": "navigation.speedThroughWater.meta",
          "value": { /* meta object */ }
        }
      ]
    }
  ]
}

attributes or properties is not an ideal choice, too likely to foul jsonschema definitions or docs as its overloaded with meanings. Maybe

[fabdrol]

@bkp7 @timmathews @rob42 There are two issues at hand here, let's keep this discussion on the original PR (meta deltas) and move the problem with value to an issue.

@sbender9 I (personally) see no issues with adding this to the delta spec; the updates array could contain both values and metas, or either one of them. There are some things to do, however, before we can merge anything.

Also, I'm interested in hearing @tkurki's opinion on this matter (meta deltas), as he is the author of the original delta spec.

[tkurki]

I would start with support for this in the reference server, not the specification. Haven't really thought this through yet.

[bkp7]

I think this is really good functionality to implement, but I agree with @tkurki in that I it needs more thought, and as part of that we need to check any proposal will work at each type of data point in the schema. It's not as simple as value/values, meta and properties such as name, etc. as we currently have some in the specification that are 'different', probably by mistake though....

I'm not sure we should separate at this point. In essence we have time series data (which we expect to change during a voyage), currently dealt with in Deltas. Then there is the data which is fixed in nature and is updated as part of maintenance events (change of name, change of meta details, etc). Separating out the meta from other fixed type data at this point may ultimately just add unnecessary complexity.

[sbender9]

Reference implementation: https://github.com/SignalK/signalk-server-node/pull/481

[sbender9]

Shoot. My push with the latest code failed today. Will get the latest code there tomorrow...

[sbender9]

OK, that's fixed. node server implementation looks good now.

[fabdrol]

Thanks @sbender9. I updated my review comment

[sbender9]

@fabdrol I updated the schema and added tests

[bkp7]

@sbender9, I've not done a full review, but the changes seem to have altered the behaviour of the api. For example the output from the test 'AIS delta produces valid Signal K' used to include a meta section in the full signalk but it doesn't do that now. Is that deliberate?

Is there any reason the meta within the delta shouldn't reference the meta in definitions? ie:

"value": {
  "$ref": "./definitions.json#/definitions/meta"
}

What's the rationale behind making meta and values mutually exclusive, and what is the use case for source alongside meta? The documentation doesn't cover this.

Finally, before this gets merged can we consider the interaction with the API separation #435 please. Assuming #435 gets merged I think it may be better to do that first and apply the changes to the API embedded in this PR to the API project. I'm happy to implement the above changes into the new API once #435 is sorted.

[sbender9]

@bkp7 I'm not seeing the missing meta info from 'AIS delta produces valid Signal K' . I double checked and there is meta data there.

I did not intend to make these mutually exclusive. The schema allow 'values' or 'meta' or both.

I'm fine with waiting for #435

[bkp7]

@sbender values and meta are mutually exclusive by virtue of the oneOf at line 21. The following delta will not validate against the oneOf:

{
  "context": "bar",
  "updates": [
    {
      "source": {
        "label": "schema"
      },
      "timestamp": "2013-10-08T15:47:28.263Z",
      "meta": [
        {
          "path": "a.b.c",
          "value": {
            "units": "m"
          }
        }
      ],
      "values": [
        {
          "path": "a.b.c",
          "value": 1234
        }
      ]
    }
  ]
}

I'd suggest adding this as an extra test.

Fixing the schema should just be a case of changing oneOf to anyOf at line 21, but unfortunately there is a bug in TV4 the schema validator we are currently using (which is no longer maintained) which means that you will get loads of failed tests if you change it. Until we move away from TV4 (which I'm working on) my suggested work around is to replace the existing oneOf with:

        "oneOf": [
          {
            "required": ["values"],
            "properties": {"$source":{}, "source": {}, "timestamp": {}, "values": {}},
            "additionalProperties": false
          },
          {
            "required": ["meta"],
            "properties": {"$source":{}, "source": {}, "timestamp": {}, "meta": {}},
            "additionalProperties": false
          },
          {
            "required": ["values", "meta"],
            "properties": {"$source":{}, "source": {}, "timestamp": {}, "values": {}, "meta": {}},
            "additionalProperties": false
          }
        ],

[bkp7]

changes seem to have altered the behaviour of the api

@sbender9 really sorry, I must have had some files in an altered state when I was running it. Have looked at it again and it's fine, please accept my apologies.

[fabdrol]

@sbender9 @bkp7 what's the status of this one?

[sbender9]

Ready now as far as I'm concerned

[sbender9]

I don't understand why this test is failing. Any ideas?

[bkp7]

@sbender9, it's failing because it is somewhat behind Master (still on 1.0.3 I think). I have issued a PR #489 on the meta-delta branch which should fix it.

[bkp7]

@sbender, I haven't time until mid next week, but if you don't get a chance to look at the review points in the meantime, I will create a PR with suggestions then.

[bkp7]

@sbender, having looked at this in more detail I don't think the meta is in the right place. If you look at the test with both values and meta it is confusing as neither the source(or $source) or timestamp would apply to the meta. Therefore, I think the meta should be up a level next to updates. Thoughts?

[sbender9]

I think source does make sense here. The meta data could, for example, be coming from a sensor.

[bkp7]

@sbender, yes it could come from a sensor/slave server but so could items such as vessel name. We define: 'master is the canonical source for identity and configuration information for the entire vessel'.

Meta is configuration information and the idea of having different versions of meta data could lead to unfortunate results. For example if you had 2 coolant temperature sensors, one set up with alarms, etc. and the other providing just a value, if the first sensor fails and you start to use the second, the user would not expect to lose the alarm functionality. In other words the meta should be the same irrespective of the source of the value.

Perhaps we should make it clear that meta is configuration information and it is up to the master to manage that when it comes from different sources.

[fabdrol]

@sbender9 @bkp7 what's the latest on this? There are still changes requested, and will this still be fine given some other PRs that are in process of review related to deltas?

[sbender9]

I still think this is a good idea, but there’s a lot comments here and it’s hard parse where to go from here.

As far as I can tell, the only remaining issue is how to handle meta coming from multiple sources for the same path. I think we can leave that up to the client to handle since source is included.

[fabdrol]

Hey @sbender9 any thoughts on this PR? I was reading through my own comments from a while back but I'm lost on what's going on right now. Guess it needs updating and go into master as @tkurki suggested? It's still a ~useful~necessary feature