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

D-Bus ItemsChanged: reduce CPU load caused by D-Bus traffic #789

Open
34 of 45 tasks
mpvader opened this issue Feb 26, 2021 · 3 comments
Open
34 of 45 tasks

D-Bus ItemsChanged: reduce CPU load caused by D-Bus traffic #789

mpvader opened this issue Feb 26, 2021 · 3 comments
Assignees

Comments

@mpvader
Copy link
Contributor

mpvader commented Feb 26, 2021

Update 2021-12-15: after some back and forth, this was finally implemented as "ItemsChanged". Its now been done in all important services.

Reducing D-Bus traffic - increasing scalability

For example a solar charger, sends multiple PropertiesChanged signals every second, one for each D-Bus path. Also cgwacs and dbus-fronius do: 9 signals per second per PV inverter. And for reference, on a CCGX, the max signals per second is approx 30.

The idea is to send only one every (for example) second, from root, with a Dict with all paths that have an updated value.

The plan:

  1. 1. Proof of concept and prepare the used Python, C as well as Cpp libs for both receiving and sending dict-updates. Mostly done, except for transmitting from Cpp-based services.
  2. 2. Update all consumers
  3. 3. Update the heavy users: vedirect-interface, vecan-dbus, dbus_fronius and cgwacs - and maybe mk2-dbus if not too difficult?
  4. 4. (ibu) Report again on the difference in CPU load; and discuss its consequences:
  5. 5. Update the rest of the services, will probably happen only some day
  6. 6. Once most are over, we might want to move the rest over as well, including localsettings, so we stick to one method, one implementation. To be discussed/decided then.

Services that consume (and thus need updating first)

  • dbus-generator-starter - tracks values from systemcalc, should be an easy swap
  • dbus-pump - Tracks values from systemcalc, should be an easy swap
  • dbus-systemcalc-py - Tracks values from lots of services, easy swap
  • dbus-mqtt - Uses its own PropertiesChanged handler, not velib_python, but shouldn't be difficult
  • vrmlogger - Tracks values from lots of services, easy swap
  • gui - tracks lots of services - easy and already done
  • hub4control - tracks systemcalc, mk2-dbus, and dbus-fronius - easy and already done
  • dbus-modbustcp - tracks lots of services - should be easy; not done yet.
  • dbus-recorder - essential for recording, not essential for demo mode; has its own code to record, not as simple as swapping velib_python but needs doing!
  • dbus-spy
  • dbus-vecan
  • mk2vsc
  • dbus-vebus-to-pvinverter
  • mqtt-rpc

Consumers that are not immediately required:

  • vesmart-server - Tracks values from localsettings only, so not essential, but making it compatible with dict updates is easy since using velib_python for dbus receiving.

Producers of highest prio:

  • vecan-dbus
  • vedirect-interface
  • dbus-cgwacs
  • dbus-fronius
  • dbus-systemcalc-py (systemcalc also produces)

Producers that can be done some day:

Python:

  • dbus-bornay-windplus
  • dbus-digitalinputs
  • dbus-fzsonick-48tl
  • dbus-modbus-client - becoming more relevant due to it being used for AC load monitoring
  • dbus-imt-si-rs485tc
  • dbus-modem - only tracks localsettings which does not send dict updates, no need to produce dict updates
  • dbus-vebus-to-pvinverter
  • localsettings - doesn't use velib_python, doesn't subscribe to PropertiesChanged, doesn't have to send dict updates
  • mqtt-rpc - does not monitor dbus, does its own GetValue/SetValue when required
  • venus-button-handler - only tracks settings from localsettings, which does not send dict updates

C:

  • can-bus-bms
  • dbus-adc
  • gps-dbus
  • dbus-ble-networking (this is not in use!)
  • dbus-valence
  • mk2-dbus
  • mqtt-n2k - bridges can-bus and mqtt, not a dbus player.
  • dbus-motordrive

Cpp:

  • dbus-qwacs - nobody knows what this is using; perhaps this is the moment we drop support for those meters and remove dbus-qwacs entirely.

Notes per language:

Python:

In the majority of cases all that is required is updating velib_python after merging dict_updates. In some cases, a project might need a small change, ie. using the with statement notation, to send updates in a batch.

@mpvader mpvader changed the title D-Bus: send multiple properties changed from root? D-Bus: send combined PropertiesChanged signals from root? Feb 26, 2021
@izak
Copy link
Collaborator

izak commented May 10, 2021

Some results from testing.

Test environment

  1. Venus 2.65 base system
  2. vedirect-interface replaced with a binary that sends dict updates on root
  3. systemcalc and vrmlogger patched to use velib_python that understands dict updates
  4. gui recompiled with new velib so it understands dict updates
  5. Rest of the system left as is (should have minimal effect on CPU usage).

This was tested vs an unmodified setup.

First test: Test effect on CPU use on a working system

System has:

  1. CCGX
  2. 2 x VE.Direct solar chargers
  3. 1 x Multiplus
  4. 1 x BMV 700
  5. System was set up to feed solar power into the grid, so that there would be constant power changes to communicate.

Unoptimised system

CPU use:

  • systemcalc: +-10%
  • vrmlogger: +-10%
  • dbus-daemon: +-6%
  • vedirect-dbus: +-2%

CPU idle: +-30%

Optimised system

CPU use:

  • systemcalc: +-8%-9%
  • vrmlogger: +-8%-9%
  • dbus-daemon: +-5%
  • vedirect-dbus: 3%-5% (slight increase)

CPU idle: +-40% (handsome improvement)

Second test: DBus throughput on a heavily loaded system

System has:

  1. CCGX
  2. 2 x VE.Direct solar chargers
  3. 1 x Multiplus
  4. 1 x BMV 700
  5. System was set up to feed solar power into the grid, so that there would be constant power changes to communicate.
  6. An additional 5 simulated solar chargers that change voltage/power/current values once a second.
  7. dbus-statistics look specifically at PropertiesChanged signals and ignores SetValue/GetValue calls.
  8. Statistics were assessed using wireshark and dbus-monitor's --pcap option.

Unoptimised system

  • Dbus RTT: > 5000ms (round trip time was over 5 seconds)
  • Dbus traffic: 7400 bytes/sec
  • Messages per second: 37
  • Average packet size: 197 bytes
  • dbus-daemon CPU load: 10%-15%
  • Watchdog rebooted the system as load average exceeded 10.

Optimised system

  • Dbus RTT: > 100ms-200ms
  • Dbus traffic: 5900 bytes/sec
  • Messages per second: 15
  • Average packet size: 388 bytes
  • dbus-daemon CPU load: 5%
  • Load average was still very high (due to simulated solarcharger processes).

Very heavily loaded optimised system

Another two simulated solarchargers were added, for a total of 7 simulated and 2 real solarchargers (plus a BMV).

  • Dbus RTT: 2000ms
  • Dbus traffic: 6400bytes/sec
  • Messages per second: 17
  • Average packet size: 376 bytes
  • dbus-daemon CPU load: 5%
  • Watchdog rebooted due to very high load, even though the dbus-daemon remained fairly responsive

Conclusion

  • Overall CPU idle time was increased (good)
  • dbus is no longer the bottle-neck
  • Messages/second reduced to a little less than half (good)
  • Message size roughly double what it used to be
  • dbus-daemon CPU use remains low (good)

Other observations

  • vrmlogger's rtt implementation is skewed under heavy load, because the load retarts the event loop and increases the measurement.
  • There are still a lot of improvement to be had by also implementing this in dbus-cgwacs and dbus-fronius, especially in a three-phase system where these two create heavy traffic, and especially where multiple PV-inverters are present. This was not yet done as it requires work on velib to also send dict updates.

@izak

This comment has been minimized.

@mpvader mpvader changed the title D-Bus: send combined PropertiesChanged signals from root? D-Bus dict-updates: send combined PropertiesChanged signals from root Sep 13, 2021
@mpvader mpvader changed the title D-Bus dict-updates: send combined PropertiesChanged signals from root D-Bus dict-updates: send PropertiesChanged signals from root Sep 13, 2021
izak added a commit to victronenergy/dbus-mqtt that referenced this issue Sep 29, 2021
izak added a commit to victronenergy/dbus_pump that referenced this issue Sep 30, 2021
This allows tracking dict_updates (multiple paths
set on the root path of PropertiesChanged) from
other services. Presently there are none, but in
future tanks may well produce dict updates.

victronenergy/venus#789
izak added a commit to victronenergy/dbus_generator that referenced this issue Sep 30, 2021
This is to allow tracking dict_updates that may be
coming from other services. Soon systemcalc will start
sending dict_updates.

victronenergy/venus#789
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Oct 5, 2021
Allow PropertiesChanged signals to contain multiple paths
    This is only on the consumption side for now. Production will follow.
    victronenergy/venus#789
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Oct 5, 2021
More improvements to reduce CPU use
    victronenergy/venus#861
Allow PropertiesChanged signals to contain multiple paths
    victronenergy/venus#789
izak added a commit to victronenergy/dbus-mqtt that referenced this issue Oct 19, 2021
This replaces the earlier implementation where
PropertiesChanged would carry a dict on the root path.

victronenergy/venus#789
izak added a commit to victronenergy/dbus-recorder that referenced this issue Oct 19, 2021
The previous PropertiesChanged on root implementation has been
replaced with an ItemsChanged signal that does the same job.
This time, instead of recording the message as is, we
instead unroll it. This will be compensated for during replay
in giving the user the option to replay the recording in packed
ItemsChanged signals or individual PropertiesChanged signals.

victronenergy/venus#789
izak added a commit to victronenergy/dbus-recorder that referenced this issue Oct 19, 2021
By default, send all changes that happen at the
same time as a single ItemsChanged signal on the
root path.

This replaces the previous implementation with
a PropertiesChanged message on root.

The old behaviour where individual PropertiesChanged
messages are sent is still possible by passing the
--nozip commandline option to play.py.

victronenergy/venus#789
izak added a commit to victronenergy/dbus_modbustcp that referenced this issue Oct 19, 2021
Support receiving the ItemsChanged signal.

victronenergy/venus#789
izak added a commit to victronenergy/dbus_pump that referenced this issue Oct 20, 2021
This replaces the previous dict_updates implementation
with one that instead tracks the ItemsChanged signal.

victronenergy/venus#789
izak added a commit to victronenergy/dbus_generator that referenced this issue Oct 20, 2021
This replaces tracking of dict_update tracking by a new
and taylor-made ItemsChanged signal.

victronenergy/venus#789
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Oct 21, 2021
refactor the recording script
    remove playback functionality, since this has moved to play.py a long time ago.
Record changes from ItemsChanged signals containing multiple paths and values
    This replaces an earlier unreleases PropertiesChanged implementation
    Recorded dicts are no longer stored as-is. Instead they are repacked at replay time.
        This allows easier editing when manipulating a simulation, as well as
        more efficiently replaying older recordings.
    It is now possible to replay recordings in a specific CSV format. A much easier
    format for making demos.

victronenergy/venus#789
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Oct 21, 2021
ItemsChanged signals can contain data for multiple paths
    This replaces the previous implementation that piggy-backed on PropertiesChanged

victronenergy/venus#789
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Oct 21, 2021
Support receiving the ItemsChanged signal
    This replaces the earlier PropertiesChanged-on-root implementation

victronenergy/venus#789
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Oct 21, 2021
Support receiving the ItemsChanged signal.

victronenergy/venus#789
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Oct 23, 2021
Support ItemsChanged / GetItems as consumer.

victronenergy/venus#789
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Nov 10, 2021
Switch to ItemsChanged signal for bulk updates.
 - Some individual changes still generate PropertiesChanged

victronenergy/venus#789
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Nov 10, 2021
- log failed devices
- group dbus updates using ItemsChanged message

victronenergy/venus#789
mpvader added a commit to victronenergy/meta-victronenergy that referenced this issue Dec 9, 2021
- Fixed name for Solar charger PV power, it was named Total yield (kWh), now its
  PV power (W).
- Add Battery temperature to the System input node.
- Add Battery temperature to the VE.Bus input node.
- Removed MppOperationMode from the solar-charger control node: thats not a
  parameter that can be written to.
- Add "BMS allows to charge" and "BMS allows to discharge" paths to VE.Bus input node.
  Note these relate only(!) to the VE.Bus BMS.
- Add support for dict-updates on the root path.
  victronenergy/venus#789
- Refresh npm-shrinkwrap: three small updates of dependencies.
@mpvader mpvader changed the title D-Bus dict-updates: send PropertiesChanged signals from root D-Bus ItemsChanged: send one signal from root, rather than many signals from many paths Dec 15, 2021
@mpvader mpvader changed the title D-Bus ItemsChanged: send one signal from root, rather than many signals from many paths D-Bus ItemsChanged: reduce CPU load caused by D-Bus traffic Dec 15, 2021
mpvader added a commit to victronenergy/meta-victronenergy that referenced this issue Dec 15, 2021
- Fixed name for Solar charger PV power, it was named Total yield (kWh), now its
  PV power (W).
- Add Battery temperature to the System input node.
- Add Battery temperature to the VE.Bus input node.
- Removed MppOperationMode from the solar-charger control node: thats not a
  parameter that can be written to.
- Add "BMS allows to charge" and "BMS allows to discharge" paths to VE.Bus input node.
  Note these relate only(!) to the VE.Bus BMS.
- Add support for dict-updates on the root path.
  victronenergy/venus#789
- Refresh npm-shrinkwrap: three small updates of dependencies.
mpvader added a commit to victronenergy/meta-victronenergy that referenced this issue Dec 17, 2021
- Fixed name for Solar charger PV power, it was named Total yield (kWh), now its
  PV power (W).
- Add Battery temperature to the System input node.
- Add Battery temperature to the VE.Bus input node.
- Removed MppOperationMode from the solar-charger control node: thats not a
  parameter that can be written to.
- Add "BMS allows to charge" and "BMS allows to discharge" paths to VE.Bus input node.
  Note these relate only(!) to the VE.Bus BMS.
- Add support for dict-updates on the root path.
  victronenergy/venus#789
- Refresh npm-shrinkwrap: three small updates of dependencies.
izak added a commit to victronenergy/dbus-characterdisplay that referenced this issue Jan 28, 2022
mpvader added a commit to victronenergy/meta-victronenergy that referenced this issue Jan 28, 2022
add support for ItemsChanged signal

victronenergy/venus#789
mpvader added a commit to victronenergy/meta-victronenergy that referenced this issue Feb 6, 2022
- Fixed name for Solar charger PV power, it was named Total yield (kWh), now its
  PV power (W).
- Add Battery temperature to the System input node.
- Add Battery temperature to the VE.Bus input node.
- Removed MppOperationMode from the solar-charger control node: thats not a
  parameter that can be written to.
- Add "BMS allows to charge" and "BMS allows to discharge" paths to VE.Bus input node.
  Note these relate only(!) to the VE.Bus BMS.
- Add support for dict-updates on the root path.
  victronenergy/venus#789
- Refresh npm-shrinkwrap: three small updates of dependencies.
mpvader added a commit to victronenergy/meta-victronenergy that referenced this issue Feb 8, 2022
- Fixed name for Solar charger PV power, it was named Total yield (kWh), now its
  PV power (W).
- Add Battery temperature to the System input node.
- Add Battery temperature to the VE.Bus input node.
- Removed MppOperationMode from the solar-charger control node: thats not a
  parameter that can be written to.
- Add "BMS allows to charge" and "BMS allows to discharge" paths to VE.Bus input node.
  Note these relate only(!) to the VE.Bus BMS.
- Add support for dict-updates on the root path.
  victronenergy/venus#789
- Refresh npm-shrinkwrap: three small updates of dependencies.
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Apr 1, 2022
node-red-contrib-victron: v1.4.16 -> v1.4.17

1.4.17:
- Add registration of the node output-vebus
- Remove the paths /SystemReset and /PvInverter/Disable from output-ess.
  These are also in output-vebus and are not related to the Energy Storage System.
- Update documentation to reflect the changes in paths.

node-red-contrib-victron upgrade 1.4.12 -> 1.4.16

1.4.16:
- Revert of removing output-vebus. The output-vebus node is
  perhaps used by some users, so reverting the removal of the node for
  now. The move of the paths to output-ess does not cover each case as
  there might be older Phoenix models on the VE.Bus.
- npm package babel-eslint is now known as @babel/eslint-parser.

1.4.15:
- The PropertiesChanged messages ended up returning an array; this fixes
  that by returning the value instead.
- Add the ability to query the user defined name (as part of input-system) in
  order to make testing easier. It can also be useful when creating a dashboard.

1.4.14:
- bump node-fetch from 2.6.6 to 2.6.7
- Alphabetized services.json. First on node name, then on path.
- Add several missing nodes:
  - input-alternator
  - input-dcsystem
  - input-evcharger
  - input-generator
  - input-genset
  - output-evcharger
  - output-multi
  - Remove node: output-vebus (the paths are part of output-ess)
- Update documentation to reflect the changes in order and paths.
- Add paths to input nodes that where present in the output nodes,
- allowing them to be read too.
- Fix typo in README.md

1.4.13:
- New nodes:
  - input-acload
  - input-multi and output-multi
- Moved GridLost alarm from input-system to input-vebus, add /Alarms/PhaseRotation
- Waded through the services file, comparing it to the dbus_modbustcp excel
  sheet. This resulted in a lot of additions, some removals and some renaming
  and updates of documentation.
- Add script for comparing entries with dbus_modbustcp excel sheet
- fix markdown syntax, typo and alphabetized input and output entries

node-red-contrib-victron: 1.4.11 -> 1.4.12

- In some cases invalid messages end up on the dbus. Now filtering the
  incoming messages so only valid messages are further processed.

node-red-contrib-victron 1.4.9 -> 1.4.11

1.4.11: fix in dbus ItemsChanged
- now copying msg.path for dbus ItemsChanged signals

1.4.10: support for dbus ItemsChanged and minor fixes
- add support for newly added ItemsChanged signals in Venus OS D-Bus
- now using strict type check when comparing two variables
- the 'index.html' can be left out of the Victron Energy
  community url. This solves a too wide string for the help
  text of 'victron-client'

node-red-contrib-victron: 1.4.6 ->1.4.9

- add support for Ruuvi tags: humidity, pressure, batt. voltage and
  acceleration. The other value, temperature, was already supported.
- fix conditionally setting the number type (float values where not accepted)
- add relay 3 and 4 to input-relay and output-relay nodes. At the
  moment only useful on a raspberrypi having additional IOs or relays.
- add extra nodes (via @nmbath / pr #109)
- add input node for dcload meters (input-dcload)
- add input node for dcsource meters (input-dcsource)
- add input nodes for alternator and dcdc converters ((input-alternator, input-dcdc)
- added ATC (Allow To Charge) and ATD (Allow To Discharge) to input-battery
  note that not all BMS-es have those paths available. Most probably do not,
  probably only the Lynx Smart BMS has it.
- added various paths to the output-ess node, allowing more control (issue #108):
  - Feed DC overvoltage into grid : /Hub4/DoNotFeedInOvervoltage
  - Disable PV inverter : /PvInverter/Disable
  - Maximum overvoltage feed-in power L1 : /Hub4/L1/MaxFeedInPower
  - Maximum overvoltage feed-in power L2 : /Hub4/L2/MaxFeedInPower
  - Maximum overvoltage feed-in power L3: /Hub4/L3/MaxFeedInPower
  - VE.Bus Reset : /SystemReset
- added documentation to each node

node-red-contrib-victron v1.4.6

- Fixed name for Solar charger PV power, it was named Total yield (kWh), now its
  PV power (W).
- Add Battery temperature to the System input node.
- Add Battery temperature to the VE.Bus input node.
- Removed MppOperationMode from the solar-charger control node: thats not a
  parameter that can be written to.
- Add "BMS allows to charge" and "BMS allows to discharge" paths to VE.Bus input node.
  Note these relate only(!) to the VE.Bus BMS.
- Add support for dict-updates on the root path.
  victronenergy/venus#789
- Refresh npm-shrinkwrap: three small updates of dependencies.

node-red-contrib-victron v1.4.2 -> v1.4.5

Changes to nodes:
- solar-charger node: add support for multiple trackers (MPPT RS)
- solar-charger node: remove PV current and replace with PV Power.
- system-node: add lots of measurements
- ess-node: add Active SoC limit
- many nodes: add various missing enum names as well as update them,
  this doesn't change actual node-red functionality; it only makes
  the descriptions better as visible in the Edit Node pane.
- many nodes: update the names of the measurements to match how
  now defined on VRM

Other changes:
- node-red-contrib-victron has had a major update of its package-lock.json. But,
  that won't make much of a difference on how its installed in Venus OS, since
  the npm-shrinkwrap used already had all those newer versions. To prevent
  issues like this in the future, there is now a procedure written here:
  https://github.com/victronenergy/node-red-contrib-victron#releasing-a-new-version-developers
- though the npm-shrinkwrap has lots of changed lines, most of it is just whitespace,
  most dependency versions are unchanged.
izak added a commit to victronenergy/dbus-digitalinputs that referenced this issue Aug 3, 2022
This somewhat reduces dbus messages, mostly useful for pulsecounter
devices that may send updates more frequently.

victronenergy/venus#789
izak added a commit to victronenergy/dbus-bornay-windplus that referenced this issue Aug 26, 2022
izak added a commit to victronenergy/dbus_vebus_to_pvinverter that referenced this issue Aug 26, 2022
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Oct 6, 2022
send ItemsChanged instead of PropertiesChanged
victronenergy/venus#789
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Oct 18, 2022
- send ItemsChanged instead of PropertiesChanged
  victronenergy/venus#789
- remove deprecated /Ac/Current
- Don't react to uninteresting NameOwnerChanges
izak added a commit to victronenergy/dbus_vebus_to_pvinverter that referenced this issue Dec 8, 2022
Send one ItemsChanged instead of one per phase as earlier.

victronenergy/venus#789
jhofstee pushed a commit to victronenergy/meta-victronenergy that referenced this issue Dec 13, 2022
Reduce D-bus traffic, send one ItemsChanged instead of one per phase.

victronenergy/venus#789
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

No branches or pull requests

2 participants