Merged
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider how the
absoperation behaves forint32on the minimum value (-2^31); if the calc engine doesn't guard againstabs(INT32_MIN)overflow, explicitly clamping or converting to a larger numeric type beforeabsmay be safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider how the `abs` operation behaves for `int32` on the minimum value (`-2^31`); if the calc engine doesn't guard against `abs(INT32_MIN)` overflow, explicitly clamping or converting to a larger numeric type before `abs` may be safer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Updates the Solis Hybrid (S Series) meter template’s battery power calculation to correctly interpret register 33149 across firmware variants by decoding it as int32 and applying abs() before applying the charge/discharge direction from register 33135.
Changes:
- Change battery power register
33149decoding fromuint32toint32. - Wrap battery power in a
calc+absstage before multiplying by the direction-derived sign.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The battery
powercalculation was updated to correctly handle both firmware variants of the Solis Hybrid (S Series) inverter:Before: Register 33149 was read as
uint32. When the firmware reports a negative value as a signed 32-bit integer (e.g.0xFFFFFC7C= −900 W during discharge), this was misinterpreted as ~4,295 MW — a classic 32-bit unsigned overflow.After: Register 33149 is now read as
int32, and its absolute value is taken viacalc+abs. The sign is then applied separately by multiplying with +1 or −1 derived from the direction register 33135.This handles both known firmware variants:
uint32(always positive) with direction in register 33135 →abs(uint32)is identical to the raw value, sign applied via register 33135 ✓int32(negative = discharge, as seen in issue Ginlog Solis Hybrid inverter : Incorrect power values (32-bit overflow) when battery is discharging + Modbus timeouts #28732) →abs(int32)strips the sign correctly, which is then re-applied from register 33135 ✓Fixes #28732