Solis write verification and window v2 decode fixes#3653
Solis write verification and window v2 decode fixes#3653springfall2008 merged 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens Solis inverter control behavior by adding post-write verification for CID writes and adjusting how TOU v2 time windows are decoded/retained to reduce “fighting” with inverter-side adjustments. It also updates the Solis unit tests accordingly and bumps the Predbat version.
Changes:
- Add read-back verification behavior to
SolisAPI.read_and_write_cid()and update related logging/docstring. - Change TOU v2 window decoding to occur only on first run while continuing to refresh
cached_valuesvia polling. - Update Solis tests to reflect the new read/write/verify flow and add a verify-failure test; bump
THIS_VERSION.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| apps/predbat/tests/test_solis.py | Updates read_and_write_cid tests for verification behavior and adds a verify-failure scenario. |
| apps/predbat/solis.py | Implements write verification logic in read_and_write_cid, fixes a comment typo, and changes TOU v2 decoding cadence. |
| apps/predbat/predbat.py | Version bump to v8.34.11. |
| result = await self.write_cid(inverter_sn, cid, value, old_value=old_value, field_description=field_description) | ||
|
|
||
| # Validate what we wrote so the cache is correct. | ||
| old_value = await self.read_cid(inverter_sn, cid) | ||
| if not result: |
There was a problem hiding this comment.
read_and_write_cid() calls write_cid() (which updates cached_values on success) before verifying the read-back. If the verify read doesn’t match, the method returns False but leaves cached_values[inverter_sn][cid] set to the desired value, which can break later change-detection (callers may skip re-writing because the cache looks correct). Consider only updating the cache after verification succeeds, or explicitly restoring/updating the cache to the read-back value when verification fails.
apps/predbat/solis.py
Outdated
| """Read CID value then write with verification (required by Solis API). | ||
| Automatically reads current value, writes with old_value verification, | ||
| updates cache on success, and logs appropriate messages. | ||
| Retries if value doesn't update. | ||
| Updates cache on success, and logs appropriate messages. |
There was a problem hiding this comment.
The docstring says "Retries if value doesn't update" but the implementation doesn’t perform any retry loop/delay; it does a single write and a single read-back. Either implement the retry behavior (e.g., a small number of read-back attempts with backoff) or remove/update the docstring to match actual behavior.
| result = await self.write_cid(inverter_sn, cid, value, old_value=old_value, field_description=field_description) | ||
|
|
||
| # Validate what we wrote so the cache is correct. | ||
| old_value = await self.read_cid(inverter_sn, cid) | ||
| if not result: | ||
| self.log(f"Warn: Solis API: Failed to write CID {cid} {field_description} on {inverter_sn}") | ||
| return False |
There was a problem hiding this comment.
read_and_write_cid() does the verify read_cid() call before checking whether write_cid() succeeded (if not result). This causes an extra API read even when the write failed, and it also overwrites the old_value variable making the control flow harder to follow. Consider checking result immediately after write_cid(), then only performing the read-back verification on success (and store the read-back in a separate variable name).
| self.log(f"Warn: Solis API: Failed to write CID {cid} {field_description} on {inverter_sn}") | ||
| return False | ||
|
|
||
| if old_value == value: | ||
| # No change needed | ||
| self.log(f"Solis API: CID {cid} {field_description} on {inverter_sn} already set to {value}") | ||
| self.log(f"Solis API: CID {cid} {field_description} on {inverter_sn} is set to {value}") | ||
| return True | ||
| return await self.write_cid(inverter_sn, cid, value, old_value=old_value, field_description=field_description) | ||
| else: | ||
| self.log(f"Warn: Solis API: Failed to verify CID {cid} {field_description} on {inverter_sn}, wrote {value} but read back {old_value}") | ||
| return False |
There was a problem hiding this comment.
These warning logs interpolate field_description unconditionally; when it’s None the message becomes "CID None", which is noisy and less readable. Consider formatting the message to omit the description when it’s not provided (similar to how other logs in this file handle the optional description).
| # Test 6: Verify fails (write succeeds but read-back doesn't match) - should return False | ||
| read_calls.clear() | ||
| write_calls.clear() | ||
| api.log_messages.clear() | ||
|
|
||
| async def mock_write_cid_no_update(inv_sn, cid, value, old_value=None, field_description=None): | ||
| write_calls.append({"inverter_sn": inv_sn, "cid": cid, "value": value, "old_value": old_value, "field_description": field_description}) | ||
| # Write returns True but does NOT update cid_state, so verify read returns old value | ||
| return True | ||
|
|
||
| api.read_cid = mock_read_cid # Restore stateful read | ||
| api.write_cid = mock_write_cid_no_update | ||
| cid_state[107] = "10" # Set initial value | ||
|
|
||
| result = await api.read_and_write_cid(inverter_sn, 107, "99", field_description="verify test") | ||
| assert result == False, "Expected False when verify read doesn't match written value" | ||
| assert len(write_calls) == 1, f"Expected 1 write call, got {len(write_calls)}" | ||
| assert any("Failed to verify" in msg for msg in api.log_messages), "Expected verify failure log message" | ||
| print("PASSED: read_and_write_cid returns False when verify read doesn't match") |
There was a problem hiding this comment.
The new verify-failure path in read_and_write_cid() isn’t covered by a test that asserts cache behavior (e.g., that cached_values is not left at the desired value when the read-back mismatches). Adding such an assertion would help prevent regressions in change-detection logic that relies on cached_values.
No description provided.