victronenergy / node-red-contrib-victron

MIT License
87 stars 18 forks source link

Bugfix: stop polling every five seconds #147

Closed dirkjanfaber closed 1 year ago

dirkjanfaber commented 1 year ago

Currently the nodes poll every five seconds for information. Even when the information remains the same, it is pushed through the flow again. This should be changed by pushing it only once on startup and on change.

dirkjanfaber commented 1 year ago

It appears that this can also be seen as a feature, so disabling this might disappoint some users. Perhaps the better alternative is to start using the config node and making it configurable. And consider sending a cached answer instead of poling again.

pdcurtis commented 1 year ago

@dirkjanfaber I certainly use this as a feature, for example I use it for smoothing, to create daily totals and for limit checking. There are easy workarounds but it could make a lot of extra work and testing for some users with established systems. Caching would be sensible and invisible to users. An option in nodes would a very useful enhancement, for example, I frequently reduce the flows using filter (RBE) nodes which would saved or limit the flows by suitably configured delay nodes. I monitor processor loads but do not have any accurate measurements on the overheads of various configurations of nodes and flows.

mpvader commented 1 year ago

Hi, What is the node-red common practice for this?

For those flows that need repeat values, a node will exist already to take care of that I’d think?

pdcurtis commented 1 year ago

@mpvader I have used the combination of saving in Context using a Changenode with an Inject Node when I need a steady stream of messages. I do not know of a single node to do it.

You are welcome to look at my code if you want - remote access is enabled and Site name is NB Corinna. But note:

  1. Since updating to 2.91 and 2.92 Load Current seems to have disappeared from the Solar Charger Node so quite a lot of relevant code is disabled temporarily until I investigate. Odd as it is still on Portal.
  2. I am testing the changes in @dirkjanfaber 's faberd/issue-144 branch (b4a1d83) which seems very successful in meeting its objectives.
  3. I have Persistent Global Context Storage enabled so beware if you copy any code!
tkurki commented 1 year ago

So if I get only changes how do I tell everything is working? If I want to get notified about potential error condition of missing data how do I do that?

Afaik there is no canonical way to do this in NR, depends on how the data is produced. A switch closes and triggers an update via interrupt vs a temperature sensor is read periodically.

I’d make polling interval configurable and maybe add an option to send only changes, not change the default behavior. Nor would i classify this as a bugfix.

dirkjanfaber commented 1 year ago

So if I get only changes how do I tell everything is working?

With the status node, one can check if the node still is connected, which indicates that everything is still working.

That being said, I understand the concerns and benefits about getting data every x seconds. Fetching the answer from the cache instead of from the dbus will decrease the bus load, which should be the default. Basically re-sending the last answer periodically.

Getting it in a configuration pane is what I aim for. Perhaps even with the non-default choice of making it possible to enable fetching fresh data every x seconds from the dbus instead of from the cache.

mpvader commented 1 year ago

Hey Dirk-Jan, if everybody prefers it the way it is now, resending every five seconds, with added benefit that 100% there is nothing going wrong thats related to a cache (and caches often cause issues...), simply by not having a cache: then for me its also fine to just leave it all as is now.

The bit of extra D-Bus load is acceptable to me.

pdcurtis commented 1 year ago

I have just noticed that the polling interval is no longer exactly 5 seconds but faster and the timing is no longer regular see screenshot of debug window of output from a SmartSolar MPPT 100/20 48 Screenshot from 2022-11-10 11-35-16

thepaulcooper commented 1 year ago

I vote for keeping the 5s poll or, if making it configurable, keep it as the default.

dirkjanfaber commented 1 year ago

The solution will be an extra checkbox on the edit panel: image

When checked, the node will output only on value changes. When not checked, the behaviour is the same as it has always been.

dirkjanfaber commented 1 year ago

Closing this issue (merged with master)