search

tkurki / signalk-mqtt-gw

Signal K server plugin that provides gateway functionality between Signal K and MQTT
Apache License 2.0
14 stars 7 forks source link

Proposed changes to the MQTT gateway plugin #6

Closed krital closed 5 years ago

krital commented 5 years ago

Hi Teppo,

The following changes are proposed:

  1. Changed publishing logic so that topic namespace is expanded if the value of a signalk path is not String or Number but a JSON object.

I understand you have some reservations on this because lat / lon for example only make sense together. However for consistency we should have the same type of values on all topics and be compact over the wire. The topic name provides the context of the value in the payload and an MQTT subscriber can subscribe to as many or as few topics as required, potentially using wildcard topic strings.

  1. Ensured that MQTT payload values are always published as a String and not a Number which makes Mosca fail to stream it to potentially existing subscriber connections.

This had me going for a while.. When you publish a message with a payload of String or Number but have no active subscribers everything is fine. If you have any subscriptions though they get disconnected each time an MQTT message with a Number as a payload is streamed to them with the following stack: 

TypeError [ERR_INVALID_ARG_TYPE]: The "string" argument must be one of type string, Buffer, or ArrayBuffer. Received type number
at Function.byteLength (buffer.js:514:11)
at publish (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/mqtt-packet/generate.js:238:22)
at generate (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/mqtt-packet/generate.js:15:14)
at DestroyableTransform.process [as _transform] (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/mqtt-connection/lib/generateStream.js:13:16)
at DestroyableTransform.Transform._read (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/mqtt-connection/node_modules/readable-stream/lib/_stream_transform.js:184:10)
at DestroyableTransform.Transform._write (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/mqtt-connection/node_modules/readable-stream/lib/_stream_transform.js:172:12)
at doWrite (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/mqtt-connection/node_modules/readable-stream/lib/_stream_writable.js:237:10)
at writeOrBuffer (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/mqtt-connection/node_modules/readable-stream/lib/_stream_writable.js:227:5)
at DestroyableTransform.Writable.write (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/mqtt-connection/node_modules/readable-stream/lib/_stream_writable.js:194:11)
at Connection.callWrite3Args [as _callWrite] (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/reduplexer/index.js:50:25)
at Connection.write [as _write] (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/reduplexer/index.js:147:17)
at doWrite (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/reduplexer/node_modules/readable-stream/lib/_stream_writable.js:237:10)
at writeOrBuffer (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/reduplexer/node_modules/readable-stream/lib/_stream_writable.js:227:5)
at Connection.Writable.write (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/reduplexer/node_modules/readable-stream/lib/_stream_writable.js:194:11)
at Connection.(anonymous function) [as publish] (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/mqtt-connection/connection.js:64:12)
at /Users/krital/dev/signalk-server-node/node_modules/mosca/lib/client.js:200:23
at Server.authorizeForward (/Users/krital/dev/signalk-server-node/node_modules/mosca/lib/server.js:472:3)
at doForward (/Users/krital/dev/signalk-server-node/node_modules/mosca/lib/client.js:190:17)
at Client.forward (/Users/krital/dev/signalk-server-node/node_modules/mosca/lib/client.js:268:9)
at Array.handler (/Users/krital/dev/signalk-server-node/node_modules/mosca/lib/client.js:443:10)
at TrieAscoltatore.publish (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/ascoltatori/lib/trie_ascoltatore.js:54:11)
at TrieAscoltatore.newPublish (/Users/krital/dev/signalk-server-node/node_modules/mosca/node_modules/ascoltatori/lib/abstract_ascoltatore.js:144:20)

Not sure if this is by design or a Mosca bug but as we are several versions behind current perhaps we should try and update first before sending them a PR.

  1. When mapping SignalK paths to MQTT topics, also convert : characters in AIS data

When receiving a delta with AIS data and try to convert the path to an MQTT topic, the : character is not allowed and causes errors / disconnections. We are now also converting that to the MQTT topic name delimiter /

tkurki commented 5 years ago

Thanks for the PR, but I won’t merge it as it is.

Signal K object valued properties like coordinates and current (ocean, not electrical) are just that: the value is an object. They are like that in deltas and in the full model. As you say: to be consistent they should be similar across transports.

Especially location is like that: if a client wants for example to update the vessel position on the map it should be able to subscribe to location updates, not individual coordinate components. Or if the client wants to store all location updates in a database it should not need to collate the incoming messages from two queues.

But mostly this is about being consistent over transports unless there are extremely compelling reasons to do otherwise, which is not the case here I think.

tkurki commented 5 years ago

Please try to make independent PRs for independent changes - the other changes I agree with. Could you please do new PRs for them?

tkurki commented 5 years ago

I have no instructions in this repo, but please read https://github.com/SignalK/specification/blob/master/CONTRIBUTING.md#pull-request-and-commit-messages for general guidelines. For example the title of this PR does not really tell what it is about - if we want to generate a changelog from pr titles they need to spell out what the change is.

https://chris.beams.io/posts/git-commit/ is also good stuff.

krital commented 5 years ago

Hi Teppo, I guess after our meeting there is no need to proceed with this but we can apply changes to the new version?

tkurki commented 5 years ago

yes, let's continue in #7