Enhance helper functions for register value conversions #397

maslyankov · 2025-01-18T14:01:56Z

Added make_32bit to convert two 16-bit values into a 32-bit integer.
Introduced split_to_16bit to split a 32-bit integer into two 16-bit values.
Updated signed function to handle struct packing for signed integers.
Modified NumberRWSensor to utilize split_to_16bit for handling two 16-bit registers.
Refactored reg_to_value in Sensor class to use make_32bit for combining register values.

Related PR & issues that arised from it:

Why?

Using struct for handling signed integers (especially when merging registers) is better for several key reasons:

Proper Two's Complement Handling:
- Struct automatically handles two's complement representation for signed numbers
- Direct bit manipulation might miss the sign bit handling, leading to incorrect negative values

Platform Independence:

# Using struct (safe)
import struct
value = struct.unpack('>i', bytes([0xFF, 0xFF, 0x80, 0x00]))[0]  # Correctly handles -32768

# Direct bit manipulation (risky)
value = (register1 << 16) | register2  # Might not handle negative numbers correctly

Endianness Handling:
- Struct allows explicit specification of byte order (big-endian '>' or little-endian '<')
- Direct bit manipulation requires manual handling of endianness
Type Safety:
- Struct enforces proper data type conversion
- Helps prevent overflow issues and undefined behavior
Maintainability:
- Code intent is clearer with struct
- Less prone to bugs when dealing with different register sizes or signed/unsigned conversions
Standard Library Solution:
- Well-tested and documented
- Consistent behavior across Python versions and platforms

The small performance overhead of using struct is usually worth the reliability and clarity it provides when handling signed integers.

- Added `make_32bit` to convert two 16-bit values into a 32-bit integer. - Introduced `split_to_16bit` to split a 32-bit integer into two 16-bit values. - Updated `signed` function to handle struct packing for signed integers. - Modified `NumberRWSensor` to utilize `split_to_16bit` for handling two 16-bit registers. - Refactored `reg_to_value` in `Sensor` class to use `make_32bit` for combining register values.

codecov · 2025-01-18T14:03:00Z

Codecov Report

Attention: Patch coverage is 92.30769% with 3 lines in your changes missing coverage. Please review.

Project coverage is 71.00%. Comparing base (f23619a) to head (cc92fef).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
src/sunsynk/rwsensors.py	80.00%	2 Missing ⚠️
src/sunsynk/helpers.py	95.83%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #397      +/-   ##
==========================================
+ Coverage   70.97%   71.00%   +0.02%     
==========================================
  Files          26       26              
  Lines        1902     1914      +12     
==========================================
+ Hits         1350     1359       +9     
- Misses        552      555       +3

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

maslyankov · 2025-01-18T14:05:11Z

I will work on tests and linting, but first @kellerza I'd like to hear your thoughts on this.

JanKraslice · 2025-01-18T23:12:19Z

How do I get back to the 2024 version? I'm keeping it permanently. We can't play here.
Accuracy has to be 99.9%. Anything less is wrong and we can't deploy it.
Thank you kellerza I trust you

maslyankov · 2025-01-18T23:17:55Z

How do I get back to the 2024 version? I'm keeping it permanently. We can't play here.

Accuracy has to be 99.9%. Anything less is wrong and we can't deploy it.

Thank you kellerza I trust you

Bro stop spamming around like that. This is an "edge" version - still in testing. Use the release (stable) version if it's that serious of an issue for you. Everyone here is working free of charge to make the project better.

P.S.: if you use the addon, simply do not use the "(dev)" one.

kellerza

No objections to using std lib struct

It does feel like it can be a bit more unified. I made a proposal.
As long as we have tests then it should be ok.

src/sunsynk/rwsensors.py

src/sunsynk/sensors.py

kellerza · 2025-01-20T08:05:20Z

How do I get back to the 2024 version? I'm keeping it permanently. We can't play here. Accuracy has to be 99.9%. Anything less is wrong and we can't deploy it. Thank you kellerza I trust you

Like @maslyankov - either use the multi version, or downgrade using the HA supervisor / point to another docker image

- Introduced `pack_value` and `unpack_value` functions for packing and unpacking register values, enhancing register format handling. - Updated `NumberRWSensor` to utilize `pack_value` for 16-bit and 32-bit register packing. - Refactored `reg_to_value` method in `Sensor` class to use `unpack_value` for improved register value extraction. - Removed deprecated functions `make_32bit`, `split_to_16bit`, and `signed` from helpers to streamline code.

maslyankov · 2025-01-20T17:45:54Z

I have streamlined the implementation. @kellerza please take a look if I've missed anything. I will then work on passing linting and passing/updating tests.

src/sunsynk/helpers.py

src/sunsynk/rwsensors.py

src/sunsynk/helpers.py

- Updated `pack_value` to return `RegType` instead of a tuple, improving type clarity. - Removed deprecated `patch_bitmask` function and reintroduced it with improved implementation. - Simplified `NumberRWSensor` to utilize `pack_value` for dynamic register packing based on address length. - Enhanced code readability and maintainability by consolidating bitmask operations.

- Removed unused `NumType` import from `sensors.py` to streamline code. - Fixed typo in `rwsensors.py` by correcting `self.addrees` to `self.address`. - Simplified type imports in `helpers.py` by removing `Tuple`, as it was not utilized. Fixes linting

- Updated `Sensor` class to ensure proper value calculation by converting the value to float before applying the factor in `sensors.py`. - Enhanced test suite in `test_helpers.py` by adding tests for `pack_value` and `unpack_value` functions, ensuring robust validation of register value conversions. - Improved existing tests for `signed` value conversions and added documentation for clarity.

- Updated `reg_to_value` method in `MathSensor` to use `unpack_value` for improved register value extraction. - Removed unused `NumType` import from `state.py` to streamline code. - Enhanced readability of test cases in `test_helpers.py` by formatting assertions for better clarity.

…16-bit and 32-bit packing. Added two's complement conversion for improved register value handling.

- Added validation to `NumberRWSensor` and `TimeRWSensor` to raise a `NotImplementedError` if the sensor address is not set, preventing invalid operations. - Implemented a post-initialization check in `SystemTimeRWSensor` to ensure exactly 3 registers are provided, raising a `ValueError` if the condition is not met.

src/tests/sunsynk/test_helpers.py

…ensors - Added error handling tests for `pack_value` in `test_helpers.py` to validate behavior for invalid inputs. - Updated `test_rwsensors.py` to include a factor for signed values in `NumberRWSensor`, improving test accuracy for negative value handling.

kellerza · 2025-01-22T11:12:35Z

Thanks @maslyankov!

kellerza reviewed Jan 20, 2025

View reviewed changes

src/sunsynk/rwsensors.py Outdated Show resolved Hide resolved

src/sunsynk/sensors.py Outdated Show resolved Hide resolved

kellerza reviewed Jan 20, 2025

View reviewed changes

src/sunsynk/helpers.py Outdated Show resolved Hide resolved

src/sunsynk/helpers.py Show resolved Hide resolved

src/sunsynk/rwsensors.py Outdated Show resolved Hide resolved

src/sunsynk/helpers.py Outdated Show resolved Hide resolved

maslyankov added 10 commits January 20, 2025 22:15

ruff test fixes

d2bafc2

ruff

7756a09

fix mypy issues

3587631

Enhance pack_value function to handle unsigned negative values for …

94133a1

…16-bit and 32-bit packing. Added two's complement conversion for improved register value handling.

This is actually handled by structs lib

d0a9135

kellerza reviewed Jan 21, 2025

View reviewed changes

src/tests/sunsynk/test_helpers.py Show resolved Hide resolved

kellerza merged commit a664930 into kellerza:main Jan 22, 2025
8 checks passed

kellerza mentioned this pull request Jan 22, 2025

Power values jump to 65535 kW and -65509 kW #398

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance helper functions for register value conversions #397

Enhance helper functions for register value conversions #397

maslyankov commented Jan 18, 2025 •

edited

Loading

codecov bot commented Jan 18, 2025 •

edited

Loading

maslyankov commented Jan 18, 2025

JanKraslice commented Jan 18, 2025

maslyankov commented Jan 18, 2025 •

edited

Loading

kellerza left a comment

kellerza commented Jan 20, 2025

maslyankov commented Jan 20, 2025

kellerza commented Jan 22, 2025

Enhance helper functions for register value conversions #397

Enhance helper functions for register value conversions #397

Conversation

maslyankov commented Jan 18, 2025 • edited Loading

Why?

codecov bot commented Jan 18, 2025 • edited Loading

Codecov Report

maslyankov commented Jan 18, 2025

JanKraslice commented Jan 18, 2025

maslyankov commented Jan 18, 2025 • edited Loading

kellerza left a comment

Choose a reason for hiding this comment

kellerza commented Jan 20, 2025

maslyankov commented Jan 20, 2025

kellerza commented Jan 22, 2025

maslyankov commented Jan 18, 2025 •

edited

Loading

codecov bot commented Jan 18, 2025 •

edited

Loading

maslyankov commented Jan 18, 2025 •

edited

Loading