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

implement XDR codec #53

Closed
wants to merge 1 commit into from
Closed

implement XDR codec #53

wants to merge 1 commit into from

Conversation

damianham
Copy link

implements the XDR codec - no conversion is made of the sensor data value - it is simply copied from the source as is until we know what conversions we are going to do for each sensor type

Handles the following input lines
$WIXDR,C,022.0,C,,_52 - (no sensor name - C0 is assumed)
$YXXDR,C,7,C_54 - (no sensor name C0 is assumed)
$OSXDR,C,9.5,C,1W0,C,9.0,C,1W1*56 - (multiple named sensor values)

@joabakk
Copy link
Contributor

joabakk commented Feb 22, 2016

I have started on an XDR conversion myself on my xdr branch. I think we should start with the compliant tuples of four and address the exceptions after. With such a mess of different inerpretations, the code quickly becomes messy.

@keesverruijt
Copy link
Member

I'm not sure I understand the code, but you can't treat XDR like any other NMEA0183 sentence. You must include the sender ID (i.e. the characters before XDR) in you decision to decide what it means. AAXDR,C, may mean something completely different to ABXDR,C,.

At the very least I'd expect some comments that explain what a WI, YXX or OS is.

@joabakk
Copy link
Contributor

joabakk commented Feb 22, 2016

I agree. As I said, i have started. Not easy to write code on an iPad. I forgot my power supply...

@joabakk
Copy link
Contributor

joabakk commented Feb 22, 2016

I am happy to step down if you are working on XDR at the moment. My thought was to check for every place where there "should" be an id, and match to the known mappings and units. The non compliant sentences would be an exception to this main rule. That's about as far as I came

@damianham
Copy link
Author

The WI,YX and OS are examples from an openplotter discussion on cruisersforum

This code should work for openplotter, if there are sentences generated by some manufacturers that don't comply and can't be processed by this module can we cross that bridge when we come to it ?

@joabakk
Copy link
Contributor

joabakk commented Feb 22, 2016

Sure. Keep up the good work

@tkurki
Copy link
Member

tkurki commented Feb 22, 2016

Can you clarify your comment

This code should work for openplotter

Does Openplotter expect Signal K deltas with paths like sensors.1W0.name?

@damianham
Copy link
Author

Open plotter provides NMEA 0183 input stream to signalk using kplex

It is my display app that is expecting paths like sensors.1W0.sensorData

This implementation is agnostic about the instrument names, open plotter is
generating the names and my app is using them to display values.

We definitely need to add transformations to ensure temperature is in
Celsius etc.

On Tuesday, 23 February 2016, Teppo Kurki notifications@github.com wrote:

Can you clarify your comment

This code should work for openplotter

Does Openplotter expect Signal K deltas with paths like sensors.1W0.name?


Reply to this email directly or view it on GitHub
#53 (comment)
.

@joabakk
Copy link
Contributor

joabakk commented Feb 22, 2016

I see on https://github.com/sailoog/openplotter/blob/master/i2c.py that sensors like heel and pitch are mapped in a more standard way and these actually have a mapping in sk.

@damianham
Copy link
Author

Yes there is definitely a duplication for some things. The i2c things are
sensors and the signalk spec has a sensors element so I thought it wasn't
such a bad idea to output the data within the sensors tree. We could also
generate an update with other paths for any apps that are subscribed to
other branches of the tree for what is essentially the same data.

I suggest following along with the cruisers forum open plotter thread to
get a feel for end user use case on handling sensor data, that is how I
jumped into this anyway

On Tuesday, 23 February 2016, joabakk notifications@github.com wrote:

I see on https://github.com/sailoog/openplotter/blob/master/i2c.py that
sensors like heel and pitch are mapped in a more standard way and these
actually have a mapping in sk.


Reply to this email directly or view it on GitHub
#53 (comment)
.

@tkurki
Copy link
Member

tkurki commented Feb 22, 2016

The basic idea of Signal K is that we have well defined, source agnostic paths for data and standard unit. For example if a sensor provides air temperature it is available as environment.airTemp in Kelvins, not sensors.XYZ.sensorData in Fahrenheit.

What you are proposing goes against this - instead of a standard path based what the data actually is the data would be available based solely on the specific source.

So instead of a generic passthrough with cryptic names we should create a mapping from vendor specific XDRs to standarg SK paths & values - in essence extend the current nmea0183 SK parser with another level of logic. Currently the conversion is driven by just the sentence - for XDR it will need another level where the vendor prefix is also taken into account.

A great step forward would now be to gather XDR sentences to one place and then create a generic solution with specific configs for the known kinds.

Can we continue here: #54

@damianham
Copy link
Author

Although I agree with all of this, there is a use case where a generic pass
through is useful and the meaning of the path is configured in the consumer
app.

Consider the user who adds temperature sensors to various places on both
his engines and also on the wall of both engine rooms, the fridge, coffee
machine, shower cubicle, jacuzzi etc. will there be a path in signalk for
jacuzzi temperature ?

So I believe there are some cases where a generic pass through may be more
useful and sensors tree is probably where this will occur

On Tuesday, 23 February 2016, Teppo Kurki notifications@github.com wrote:

The basic idea of Signal K is that we have well defined, source agnostic
paths for data and standard unit. For example if a sensor provides air
temperature it is available as environment.airTemp in Kelvins, not
sensors.XYZ.sensorData in Fahrenheit.

What you are proposing goes against this - instead of a standard path
based what the data actually is the data would be available based solely on
the specific source.

So instead of a generic passthrough with cryptic names we should create a
mapping from vendor specific XDRs to standarg SK paths & values - in
essence extend the current nmea0183 SK parser with another level of logic.
Currently the conversion is driven by just the sentence - for XDR it will
need another level where the vendor prefix is also taken into account.

A great step forward would now be to gather XDR sentences to one place and
then create a generic solution with specific configs for the known kinds.


Reply to this email directly or view it on GitHub
#53 (comment)
.

@tkurki
Copy link
Member

tkurki commented Feb 22, 2016

Yep, there is a need for generic measurements, but I would like to throw in the option to map these temperature values to proper Signal K paths and if all else fails pass them along as generic sensor values. For example is WIXDR is typically produced by outside temperature sensors (my assumption, may be wrong) why not have that mapping available?

I am really not sure what if any value Signal K provides here. Would it be more straightforward to provide the original sentences unmodified, wrapped in JSON? Or does it make the messages inherently easier to handle by converting the XDR to quadruplets in JSON?

Then again having everything preparsed and all data in the same, uniform stream makes writing clients easier.

@damianham
Copy link
Author

Yes I agree mapping known values to proper signalk paths first and falling
back to pass through seems to be the best compromise.

I think signalk does add a lot of value to this case and yes it makes it
easier to handle that the data is converted to signalk
Json format, speaking from a consumer implementer point of view. There is
also a huge ancillary benefit in long term maintenance. Code blocks that
subscribe to signalk paths are much easier to understand than code parsing
NMEA sentences. The intention is given in the descriptive path name

On Tuesday, 23 February 2016, Teppo Kurki notifications@github.com wrote:

Yep, there is a need for generic measurements, but I would like to throw
in the option to map these temperature values to proper Signal K paths and
if all else fails pass them along as generic sensor values. For example is
WIXDR is typically produced by outside temperature sensors (my assumption,
may be wrong) why not have that mapping available?

I am really not sure what if any value Signal K provides here. Would it be
more straightforward to provide the original sentences unmodified, wrapped
in JSON? Or does it make the messages inherently easier to handle by
converting the XDR to quadruplets in JSON?

Then again having everything preparsed and all data in the same, uniform
stream makes writing clients easier.


Reply to this email directly or view it on GitHub
#53 (comment)
.

@keesverruijt
Copy link
Member

I am strongly not in favor of Signal K doing any passthrough. The entire theory of passing data through to a client that is suddenly magically able to make sense of the data seems flawed to me. Much better to put this intelligence in the converter from XYZ to SK so that all clients are able to understand the data.

@tkurki
Copy link
Member

tkurki commented Mar 29, 2016

While this thread has been hanging in the air we did a bit of work on the temperatures. The temperatures available in the N2K data model have now mappings in the Signal K data model. The easiest way to see all the temperatures is in the updated keyswithmetadata.json file available in the specification repo. See for example https://github.com/SignalK/specification/blob/3b7f56dfef9ceca8c813dff75524292e5c6e7198/keyswithmetadata.json#L556

This however does not take care of arbitrary values, but I think that we should extend the Signal K data model and not just push out sensor values with random id's.

For a consumer to act meaningfully to the data it needs to know the unit that it is send in and a semantic meaning to it, possibly in some commonly known grouping that makes sense to the user so that the values are not in random order.

Continuing with your proverbial jacuzzi example: I would send that out as environment.water.jacuzzi.temperature and the server that is sending that data should provide metadata for it under /signalk/v1/vessel/xxx/environment/water/jacuzzi/temperature/meta, including unit and nominal range. This way even a generic Signal K UI app could display it in a meaningful fashion.

@tkurki
Copy link
Member

tkurki commented Mar 29, 2016

Closing the issue here, as I think the discussion should continue under specificationrepository with the goal of documenting how Signal K data model could be extended and used to server ad hoc extension needs.

SignalK/specification#206

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

Successfully merging this pull request may close these issues.

4 participants