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

Improvement/recursive handle sensor dependencies #377

Conversation

maslyankov
Copy link
Contributor

@maslyankov maslyankov commented Dec 26, 2024

tested and works:

build: https://github.com/maslyankov/sunsynk/actions/runs/12509593568
image: ghcr.io/maslyankov/hass-addon-sunsynk-multi:edge

Resulting improvement in config:

SENSORS:
  - energy_management
  - power_flow_card
  - pv_power
  - pv1_power
  - PV1 voltage
  - PV1 current
  - pv2_power
  - PV2 voltage
  - PV2 current
  - Load power
  - Load L1 power
  - Load L2 power
  - Load L3 power
  - Load L1 voltage
  - Load L2 voltage
  - Load L3 voltage
  - battery_temperature
  - Fault
SENSORS_FIRST_INVERTER:
  - settings
  - Grid Charge Battery current
  - Grid Charge Start Battery SOC
  - Grid Charge enabled
  - Battery Max Charge current
  - Battery Max Discharge current
  - Battery Capacity current
  - Date Time
  - Battery Shutdown Capacity
  - Battery Restart Capacity

now becomes:

SENSORS:
  - energy_management
  - power_flow_card
SENSORS_FIRST_INVERTER:
  - settings
  - diagnostics

…f visibility and dependencies. Ensure sensors are added to startup set regardless of visibility, and only include them in SOPT if explicitly requested or if they are direct dependencies. Enhance dependency tracking to avoid unnecessary affects tracking.
Copy link

codecov bot commented Dec 26, 2024

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.96%. Comparing base (92dd837) to head (9125515).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/ha_addon_sunsynk_multi/sensor_options.py 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #377      +/-   ##
==========================================
+ Coverage   70.82%   70.96%   +0.13%     
==========================================
  Files          26       26              
  Lines        1868     1877       +9     
==========================================
+ Hits         1323     1332       +9     
  Misses        545      545              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

self[DEFS.serial] = SensorOption(
sensor=DEFS.serial, schedule=Schedule(), visible=False
)
self._add_sensor_with_deps(DEFS.rated_power, visible=False)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add rated_power or serial in your config?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or will visible always be false?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it so that all dependencies that are not defined in the config are hidden by default. There are further improvements to this branch on other branches, but my repo has diverged from this one somewhat.

schedule=get_schedule(dep, SCHEDULES),
)
self[dep].affects.add(sopt.sensor)
self._add_sensor_with_deps(sen, visible=True)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need a first=True here as well? (passed to the SensorOptions?) - #374 made me think of this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this change because of failing tests. Will look into it if it is necessary.

@kellerza kellerza merged commit d15f80c into kellerza:main Jan 6, 2025
9 checks passed
@kellerza
Copy link
Owner

kellerza commented Jan 6, 2025

Thanks @maslyankov - this seems to fix some startup (restart) issues for me as well

@kellerza kellerza mentioned this pull request Jan 9, 2025
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.

RW Switches write fail (Bettery Low Capacity, Grid Enable and others maybe)
2 participants