Skip to content

Bugfix/revert and add more logging for issues#412

Closed
maslyankov wants to merge 4 commits intokellerza:mainfrom
maslyankov:bugfix/revert-and-attempt-spikes-fix
Closed

Bugfix/revert and add more logging for issues#412
maslyankov wants to merge 4 commits intokellerza:mainfrom
maslyankov:bugfix/revert-and-attempt-spikes-fix

Conversation

@maslyankov
Copy link
Copy Markdown
Contributor

@maslyankov maslyankov commented Jan 23, 2025

Enhance register unpacking and sensor reading with improved error handling and logging
Revert #408 which caused:

Added logging should help resolve:

Changes:

  • Added logging for suspicious register values (0xFFFF) and large unpacked values.
  • Improved error handling during unpacking of register values, including specific warnings for single and double register unpacking failures.
  • Introduced a mapping of register addresses to sensors for better tracking and debugging.
  • Enhanced performance logging for register reads, including warnings for slow reads and multiple consecutive timeouts.
  • Updated debug logging to provide more context on register requests and responses.
  • Linting and formatting

These changes aim to improve the robustness and reliability of the sensor reading process. This should help us resolve the inconsistency of data when it gets congested.

@maslyankov maslyankov changed the title Bugfix/revert and attempt spikes fix Bugfix/revert and add more logging for issues Jan 23, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 68.62745% with 16 lines in your changes missing coverage. Please review.

Project coverage is 70.98%. Comparing base (81d718c) to head (1a82b69).
Report is 52 commits behind head on main.

Files with missing lines Patch % Lines
src/sunsynk/helpers.py 60.00% 10 Missing ⚠️
src/sunsynk/sunsynk.py 76.92% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
- Coverage   71.49%   70.98%   -0.52%     
==========================================
  Files          27       28       +1     
  Lines        1912     1961      +49     
==========================================
+ Hits         1367     1392      +25     
- Misses        545      569      +24     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@maslyankov
Copy link
Copy Markdown
Contributor Author

Cannot fix codecov as it gives me "Error 404" when I try to open the results page. It is probably irrelevant as no new functionality has been added to require new tests.

@kellerza
Copy link
Copy Markdown
Owner

For now I've reverted #408 in #413.
Will try review the other changes over the weekend.

@maslyankov
Copy link
Copy Markdown
Contributor Author

Okay, thanks! Changes are in two of the files only. Other ones are simply pre-commit formatting.

@kellerza
Copy link
Copy Markdown
Owner

The formatting is a bit of a mess. I pinned ruff in #417

Also a small update the CI script, as my PR tried to publish a new image

Can you test these solarman sunsynk changes?

@kellerza
Copy link
Copy Markdown
Owner

Can you rebase this on the latest?

…dling and logging

- Added logging for suspicious register values (0xFFFF) and large unpacked values.
- Improved error handling during unpacking of register values, including specific warnings for single and double register unpacking failures.
- Introduced a mapping of register addresses to sensors for better tracking and debugging.
- Enhanced performance logging for register reads, including warnings for slow reads and multiple consecutive timeouts.
- Updated debug logging to provide more context on register requests and responses.

These changes aim to improve the robustness and reliability of the sensor reading process.
@maslyankov maslyankov force-pushed the bugfix/revert-and-attempt-spikes-fix branch from 149e306 to 7d5a2d2 Compare January 29, 2025 13:50
@maslyankov
Copy link
Copy Markdown
Contributor Author

Can you rebase this on the latest?

Done

Comment thread src/sunsynk/helpers.py
raise

raise ValueError(f"Unsupported number of registers: {len(regs)}")
except Exception as e:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can we remove this outer general exception handler?

you already log 1&2 register exceptions above

Comment thread src/sunsynk/sunsynk.py
_LOGGER.warning("sensor %s not being tracked", sen.id)

# Create a map of register addresses to their corresponding sensors
reg_to_sensor: dict[int, Union[Sensor, None]] = {}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
reg_to_sensor: dict[int, Union[Sensor, None]] = {}
reg_to_sensor: dict[int, Sensor | None] = {}

Comment thread src/sunsynk/sunsynk.py
isinstance(current_sensor, t)
for t in (
BinarySensor,
EnumSensor,
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can’t 0 be a vlid state for Binary/Enum?

@kellerza
Copy link
Copy Markdown
Owner

I’m ok with the logging during struct unpack, but it seems there are too many logs during read

Any chance to rather do this via one of the stats sensors?

@kellerza kellerza closed this Mar 18, 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.

2 participants