search

SignalK / specification

Signal K is a JSON-based format for storing and sharing marine data from different sources (e.g. nmea 0183, 2000, seatalk, etc)
Other
91 stars 68 forks source link

fix: units should not be a required field on metadata #512

Closed sarfata closed 5 years ago

sarfata commented 5 years ago

n2k-signalk tests do not pass with signalk-specification v1.0.4.

I have done some research and found that the issue started when we merged https://github.com/SignalK/specification/pull/462. In this PR, we have made units a required field on meta object.

However, when you use FullSignalK.addDelta(delta) and then .retrieve() we automatically add some meta tags based on information from the schema but that information is incomplete: we are missing some units which cause tests to fail.

I have added a test here that demonstrates the issue.

/vessels/urn:mrn:imo:mmsi:230099999/navigation/gnss/satellites/meta:Missing required property: units
/vessels/urn:mrn:imo:mmsi:230099999/navigation/gnss/horizontalDilution/meta:Missing required property: units
/vessels/urn:mrn:imo:mmsi:230099999/navigation/gnss/positionDilution/meta:Missing required property: units
/vessels/urn:mrn:imo:mmsi:230099999/navigation/gnss/geoidalSeparation/meta:Missing required property: units
/vessels/urn:mrn:imo:mmsi:230099999/navigation/gnss/differentialReference/meta:Missing required property: units
sarfata commented 5 years ago

We can fix this by adding units in the schema but most of these values are unitless. For example, the count of visible satellites or the horizontal dilution.

Maybe the fix here is to make units optional again?

ping @tkurki @sbender9 @bkp7

sarfata commented 5 years ago

Besides making units optional in the meta section (which I think is the right thing to do), another solution would be to edit fullsignalk.js and change:

            previous[pathPart].meta = meta;

to:

          // units is a required field in SignalK since 1.0.4
          if (Object.keys(meta).includes('units')) {
            previous[pathPart].meta = meta;
          }

I have tested and confirmed it works but really that seems like a worse solution to me.

sarfata commented 5 years ago

The description that was changed when we made units a required field, said:

If a client requests the meta property for a valid Signal K key via the HTTP REST interface, the server must return the description and, if applicable, units, even if no value has ever been generated for that key.

Therefore, I think units should never have been made required and I have pushed a commit to roll this back.