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

Initial work on XDR parser #228

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 160 additions & 0 deletions hooks/XDR.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
// $IIXDR,C,C,10.7,C,AIRTEMP,A,0.5,D,HEEL,A,-1.-3,D,TRIM,P,1.026,B,BARO,A,A,-4.-3,D,RUDDER*18

const math = require('math-expression-evaluator');
const fs = require('fs');
const xdrDictionary = { definitions: [] };
Copy link
Member

Choose a reason for hiding this comment

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

What's the idea with having definitions and not just definitions directly under root?

Copy link
Member Author

Choose a reason for hiding this comment

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

See previous comment regarding xdr-parser-plugin


try {
// Populate a dictionary
const xdrDictPath = require.resolve('xdr-parser-plugin/xdrDict');
Copy link
Member

Choose a reason for hiding this comment

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

I am not terribly fond of having a configuration file that is loaded using require.resolve. I think configuration should be injected from the context that uses the parser. We can support both though if you think this is the way to go, just that we should not log a warning if there is no configuration file.

Copy link
Member Author

Choose a reason for hiding this comment

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

See previous comment regarding xdr-parser-plugin

if (fs.existsSync(xdrDictPath)) {
try {
const json = JSON.parse(fs.readFileSync(xdrDictPath, 'utf-8'));

if (json && Array.isArray(json.definitions)) {
xdrDictionary.definitions = [ ...json.definitions ];
}
} catch (err) {
console.warn('No dictionary found for xdr-parser-plugin');
}
}
} catch (e) {
console.warn('Using default dictionary');
}

xdrDictionary.definitions = [
...xdrDictionary.definitions,
{
type: "value",
data: "temperature",
units: "C",
name: "AIRTEMP",
expression: "(x+273.15)",
sk_path: "environment.outside.temperature",
},
{
type: "roll",
data: "angle",
units: "D",
name: "HEEL",
expression: "(x*pi/180)",
sk_path: "navigation.attitude"
},
{
type: "value",
data: "angle",
units: "D",
name: "RUDDER",
expression: "(x*pi/180)",
sk_path: "steering.rudderAngle"
}
]

module.exports = function (input) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd really like to have the parser support injection of XDR configuration in constructor. Hooks are stateless, so what if we add a third parameter to the parse call. Here it would look like

module.exports = function (input, session, options) {

and options would be the hook specific options, in XDR case the dictionary.

Another options would be to modify hook loading that a hook could export a constructor with parameters that returns the actual function. This would allow validating the expressions once on load. Now a malformed expression will remain active and probably? log an error for each parse attempt.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll look into this, good suggestion

if (!Array.isArray(xdrDictionary.definitions)) {
return null;
}

const isUpperCaseChar = (p, minLen = 0) => {
const num = parseFloat(p)
return (isNaN(num) && typeof p === 'string' && p.length > minLen && p.toUpperCase() === p)
}

const { definitions } = xdrDictionary
const { parts } = input
const subs = {}
const boundaries = parts.slice(1).filter(p => isUpperCaseChar(p, 1))
const values = []

for (const boundary of boundaries) {
const index = boundaries.indexOf(boundary);
const prevBoundary = index === 0 ? null : boundaries[index - 1];
const elements = [];
let fill = false;

for (p of parts.slice(1)) {
if (!fill && (!prevBoundary || p === prevBoundary) && elements.length === 0) {
fill = true;
}

if (fill === false || p === boundary) {
Copy link
Member

Choose a reason for hiding this comment

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

!fill

fill = false;
continue
}

if (p !== prevBoundary) {
elements.push(p);
}
}

if (elements.length) {
subs[boundary] = elements
}
}

for (const boundary of boundaries) {
const data = subs[boundary]

let typeFlag = null
let value = null
let unit = null

if (data.length === 3) {
([ typeFlag, value, unit ] = data);
}

if (value === null || typeFlag === null || unit === null) {
continue
}

if (isNaN(parseFloat(value))) {
continue
}

const def = definitions.find(({ name }) => (name === boundary));
Copy link
Member

Choose a reason for hiding this comment

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

If there is ever going to be only one active definition per name, so why not make definitions an object? Then this would be definitions[name].

The current structure doesn't support having multiple formulas for multiple units. Another object layer keyed by unit would allow definitions[name][unit] lookup, supporting multiple units per definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

This behaviour is mostly a result of me attempting to re-use the definitions from xdr-parser-plugin, which became less-and-less tenable as I went through development. All good points.


if (!def) {
continue;
}

if (def.units !== unit) {
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 both should be in singular or plural.

Copy link
Member Author

Choose a reason for hiding this comment

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

See previous comment regarding xdr-parser-plugin

// Not parsing as unit doesn't match
continue;
}

const expression = def.expression.replace(/x/g, value);
const attitudeTypes = ['yaw', 'pitch', 'roll'];

let path = def.sk_path;
let result = math.eval(expression);

if (!result || isNaN(result)) {
continue
}

result = parseFloat(result.toFixed(4))

if (attitudeTypes.includes(def.type.toLowerCase())) {
path = `${path}.${def.type}`
}

values.push({
path,
value: result
})
}

if (values.length === 0) {
return null;
}

return {
updates: [
{
source: 'XDR',
timestamp: new Date().toISOString(),
values,
},
],
}
}
1 change: 1 addition & 0 deletions hooks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,5 @@ module.exports = {
BWC: require('./BWC.js'),
BWR: require('./BWR.js'),
HSC: require('./HSC.js'),
XDR: require('./XDR.js'),
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"@signalk/nmea0183-utilities": "^0.8.0",
"@signalk/signalk-schema": "1.6.0",
"ggencoder": "^1.0.4",
"math-expression-evaluator": "^1.4.0",
"moment-timezone": "^0.5.21",
"split": "^1.0.1"
},
Expand Down
27 changes: 27 additions & 0 deletions test/XDR.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// $IIXDR,C,C,10.7,C,AIRTEMP,A,0.5,D,HEEL,A,-1.-3,D,TRIM,P,1.026,B,BARO,A,A,-4.-3,D,RUDDER*18

const Parser = require('../lib')

const chai = require('chai')
const expect = chai.expect

chai.Should()
chai.use(require('chai-things'))

describe('XDR', () => {
it('Converts OK using individual parser', () => {
const delta = new Parser().parse('$IIXDR,C,C,10.7,C,AIRTEMP,A,0.5,D,HEEL,A,-1.-3,D,TRIM,P,1.026,B,BARO,A,A,-4.-3,D,RUDDER*18')

delta.updates[0].values.should.contain.an.item.with.property(
'path',
'environment.outside.temperature'
)
delta.updates[0].values[0].value.should.be.closeTo(283.85, 0.005)

delta.updates[0].values.should.contain.an.item.with.property(
'path',
'navigation.attitude.roll'
)
delta.updates[0].values[1].value.should.be.closeTo(0.0087, 0.00005)
})
Copy link
Member

Choose a reason for hiding this comment

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

How about some non happy path tests - a malformed expression, input with no matching definition?

})