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

Remove debounce to handle values updated simultaneously #4

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

rajlaud
Copy link
Contributor

@rajlaud rajlaud commented Mar 20, 2023

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
Copy link
Owner

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
Copy link

avanlievenoogen commented Mar 27, 2023

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
Copy link
Owner

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.

@iuriaranda iuriaranda merged commit 84f42a8 into iuriaranda:main Mar 28, 2023
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.

3 participants