Skip to content

Upstream/general improvements#391

Merged
kellerza merged 11 commits intokellerza:mainfrom
maslyankov:upstream/general-improvements
Jan 15, 2025
Merged

Upstream/general improvements#391
kellerza merged 11 commits intokellerza:mainfrom
maslyankov:upstream/general-improvements

Conversation

@maslyankov
Copy link
Copy Markdown
Contributor

@maslyankov maslyankov commented Jan 15, 2025

Improvements to sensor definitions for single and three-phase models. Ass well as new / updated sensor groups.

Sorry for big PR...

Addresses:

- Add more sensors and rw sensors
- improve power sensors with missing high words
…s and correcting sensor current limits for Battery 1 and Battery 2 BMS.
…matting in fault reporting. This update includes additional fault descriptions for improved diagnostics.
…load, and battery management. This update includes additional voltage and current sensors for multiple phases, as well as comprehensive battery configuration sensors. Documentation for the Power Flow Card and battery sensors has also been updated for clarity.
@maslyankov
Copy link
Copy Markdown
Contributor Author

maslyankov commented Jan 15, 2025

@kellerza Please review my handling of the issues with recursive sensor dependencies here: https://github.com/maslyankov/sunsynk/blob/main/src/ha_addon_sunsynk_multi/sensor_options.py (https://github.com/maslyankov/sunsynk/blob/39262f08ae6bd89dfefd123531e9eaebef435e48/src/ha_addon_sunsynk_multi/sensor_options.py#L49C1-L50C1)

I have not added it to this PR. I'd like to hear your thoughts on it.

Comment thread src/sunsynk/definitions/three_phase_common.py Outdated
Comment thread src/sunsynk/definitions/three_phase_common.py Outdated
Comment thread src/sunsynk/sensors.py Outdated
Comment thread inverter-docs/deye/single-phase/Modbus.-.-.V118.pdf Outdated
@maslyankov maslyankov requested a review from kellerza January 15, 2025 10:26
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.97%. Comparing base (79d9c13) to head (72f6203).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #391      +/-   ##
==========================================
+ Coverage   70.93%   70.97%   +0.04%     
==========================================
  Files          26       26              
  Lines        1899     1902       +3     
==========================================
+ Hits         1347     1350       +3     
  Misses        552      552              

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

@kellerza
Copy link
Copy Markdown
Owner

@kellerza Please review my handling of the issues with recursive sensor dependencies here: https://github.com/maslyankov/sunsynk/blob/main/src/ha_addon_sunsynk_multi/sensor_options.py (https://github.com/maslyankov/sunsynk/blob/39262f08ae6bd89dfefd123531e9eaebef435e48/src/ha_addon_sunsynk_multi/sensor_options.py#L49C1-L50C1)

I have not added it to this PR. I'd like to hear your thoughts on it.

The biggest drawback with the recursive method is that checking if a sensor (or its deps) is visible/first only is quite complex.

With the current implementation sensor_options they are added in the following order, and visibility/first seems depends on the order. Adding deps are also delayed (since they could be visible)

The order, as comments in the code

  1. Add startup sensors to all inverters. Visible if configured anywhere.
  2. Add sensors configured for all inverters
  3. Add deps, usually hidden, but visible if configured on 1st inverter
  4. Add sensors configured for the 1st inverter
  5. Add deps for the 1st inverter, always hidden

@maslyankov
Copy link
Copy Markdown
Contributor Author

@kellerza Please review my handling of the issues with recursive sensor dependencies here: https://github.com/maslyankov/sunsynk/blob/main/src/ha_addon_sunsynk_multi/sensor_options.py (https://github.com/maslyankov/sunsynk/blob/39262f08ae6bd89dfefd123531e9eaebef435e48/src/ha_addon_sunsynk_multi/sensor_options.py#L49C1-L50C1)
I have not added it to this PR. I'd like to hear your thoughts on it.

The biggest drawback with the recursive method is that checking if a sensor (or its deps) is visible/first only is quite complex.

With the current implementation sensor_options they are added in the following order, and visibility/first seems depends on the order. Adding deps are also delayed (since they could be visible)

The order, as comments in the code

  1. Add startup sensors to all inverters. Visible if configured anywhere.
  2. Add sensors configured for all inverters
  3. Add deps, usually hidden, but visible if configured on 1st inverter
  4. Add sensors configured for the 1st inverter
  5. Add deps for the 1st inverter, always hidden

In the current implementation, do we add sensors as visible if they are defined in sensor group?

@kellerza
Copy link
Copy Markdown
Owner

In the current implementation, do we add sensors as visible if they are defined in sensor group?

Yes, the groups are expanded right at the start. Nothing depends on a group

@maslyankov
Copy link
Copy Markdown
Contributor Author

Owner

Okay. Any other changes to add to this branch?

@kellerza kellerza merged commit e3af430 into kellerza:main Jan 15, 2025
@kellerza
Copy link
Copy Markdown
Owner

Thanks @maslyankov !

@Doesi
Copy link
Copy Markdown

Doesi commented Jan 18, 2025

Since these changes, power sensors on my DEYE SUN 20K-SG01 HP3-EU-AM2 randomly jump to 65535 kW and -65509 kW. This has never happened before.

@proggaras
Copy link
Copy Markdown
Contributor

I can confirm. Here is an example for "Inverter L1 Power":
image

@maslyankov
Copy link
Copy Markdown
Contributor Author

maslyankov commented Jan 18, 2025

Can you create an issue? I think I have an idea why this might happen and a PR for possible fix. Thanks.
Otherwise, these changes on my setup have allowed values larger than 32kw on different phases (which previously never worked). I also had an occasion with such spikes though.

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.

4 participants