search

victronenergy / venus-influx-loader

NodeJS server that takes from MQTT into Influx, and config UI and still more
MIT License
14 stars 6 forks source link

fix(keepalive): request all data only once, then only changes #89

Closed mman closed 4 months ago

mman commented 4 months ago

This PR addresses https://github.com/victronenergy/venus-influx-loader/issues/71 by requesting full dump of data when connecting to the Venus, and then using new keep alive options to only receive changed dbus paths to lower the load/traffic/storage needs.

Note that we are still using the legacy DBus path to initiate keep alive in order to allow connecting to older Venus versions.

mman commented 4 months ago

@dirkjanfaber @wiebeytec Guys could you please (when you get a chance) look though the keepalive changes for me if it looks reasonable to you? I'm testing the functionality and it looks to be working nicely but want to have second pair of eyes before merging. Thx!

wiebeytec commented 4 months ago

I think I would set isFirstKeepAliveRequest=true in the connect/disconnect handlers. I don't know how this client works and if it does its own reconnections, but just to be sure. Remember that the client may get disconnect because of network issues, but also when the device reboots for instance.

There are some issues with the use through VRM and on-line MQTT servers. First, client.subscribe('N/+/+/#') is the same as client.subscribe('N/#') and will be rejected by the on-line MQTT servers soon, because of the load it incurs. I also see an (unused?) hostname mqtt.victronenergy.com. This works currently by accident, but is not compatible with our server sharding. See Determining the broker URL for a given installation.

Also, it uses VRM tokens to request JWTs, but the auth scheme Token ${app.config.secrets.vrmToken} works with MQTT too nowadays. See Connecting to the VRM MQTT servers. This also helps in preventing expiring JWTs.

mman commented 4 months ago

@wiebeytec Thanks a lot for your comments, I have filed separate issues to address the VRM and MQTT subscription. As for the isFirstKeepAliveRequest=true it's actually set to true as part of MQTT on('connect') event so it should be matching the requested logic. And handling disconnects and reconnects properly, will re-test once more before merging.