iuriaranda / signalk-mqtt-bridge

SignalK Node server plugin that acts as a bridge between SignalK data and MQTT
MIT License
5 stars 2 forks source link

Remove debounce to handle values updated simultaneously #4

Closed rajlaud closed 1 year ago

rajlaud commented 1 year ago

Thanks for the great plugin.

However, I think I encountered a bug. When several values get updated simultaneously (for example, because they are generated by the same NMEA sentence or the same derived-data plugin), only the first value was being processed by this plugin.

For example, my battery monitor outputs a sentence that updates several SignalK values at once. However, the plugin was only picking up the first delta:

signalk-mqtt-bridge Got delta for topic vessels/self/electrical/batteries/0/voltage +841ms

But if I remove the debounce, finds all of the deltas:

signalk-mqtt-bridge Got delta for topic vessels/self/electrical/batteries/0/voltage +841ms signalk-mqtt-bridge Got delta for topic vessels/self/electrical/batteries/0/current +2ms signalk-mqtt-bridge Got delta for topic vessels/self/electrical/batteries/0/capacity/stateOfCharge +3ms

iuriaranda commented 1 year ago

Hey @rajlaud thanks for the contribution. Yeah I think the denounce is there to not DoS your MQTT broker and overload the network when there are a lot of updates in Signalk. But it can indeed cause trouble in this situations... Tbh it's something I ported over from the other MQTT signalk plugin, so I haven't put much thought into it.

Removing it is a solution ofc, and should be fine for most setups with a small Signalk, but could give trouble for bigger setups with a large volume of deltas.

I'll see next week whether we can resolve this differently.

avanlievenoogen commented 1 year ago

Hi @iuriaranda, I also ran into the same issue. (see discussion on slack (https://signalk-dev.slack.com/archives/C02EMP1RF/p1679673807972469) . I also removed the debounce and to prevent the plugin having to deal with all the data, I changed your plugin such that the subscription to signalK is not done in the start routine by the getBus command for all Delta's, but in the subScribeToTopic routine for only the topic you are interested in. The upside of this is that the plugin does not have handle all Delta's the downside of this is that wildcards are not supported so you have to send the keepalive signal for every signal you are interested in. I'm new to github so don't know if I can create a new pull request here, so I will create a new pull request so you can take a look at what I have done

iuriaranda commented 1 year ago

As mentioned in the other PR, and considering the debounceImmediate function is currently doing more harm than good, I'm ok removing it for now.

If we ever encounter overload problems in MQTT, we could add throttling as an option in the plugin configuration page, or find a buffering mechanism to store deltas and send them in batches to MQTT. But I wouldn't waste efforts right now on a problem that might not even exist.