Conversation
There was a problem hiding this comment.
Pull request overview
Updates Solis storage-mode encoding to ensure the TOU (bit 1) flag is set for specific modes, and aligns the Solis unit tests with the new register values.
Changes:
- Set the TOU bit when computing Solis storage-mode values for Self-Use and Feed-in priority.
- Update Solis tests to expect the new encoded values (e.g., 35 and 98 instead of 33 and 96).
- Modify the
test_solis_apiCLI helper to actively callset_storage_mode_if_neededafter a run.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| apps/predbat/solis.py | Adjusts compute_solis_mode_value to include TOU for Self-Use / Feed-in priority; updates the CLI test helper to invoke storage-mode writes. |
| apps/predbat/tests/test_solis.py | Updates assertions for new encoded storage-mode values; some comments/messages now need to be corrected to match the new behavior. |
| value = compute_solis_mode_value(ENUM_SELF_USE, 16) # old_value has backup set | ||
| assert value & (1 << SOLIS_BIT_BACKUP_MODE), "Backup bit should be preserved" | ||
| assert value == 49, f"Expected 49 (33+16), got {value}" | ||
| assert value == 51, f"Expected 51 (32 +2 +16), got {value}" |
There was a problem hiding this comment.
The assertion message “Expected 51 (32 +2 +16)” is mathematically inconsistent with 51 (it also includes the self-use bit 1). Update the message text to match the actual bit breakdown to make failures easier to interpret.
| assert value == 51, f"Expected 51 (32 +2 +16), got {value}" | |
| assert value == 51, f"Expected 51 (32 + 16 + 2 + 1), got {value}" |
| # Clears TOU and off-grid bits | ||
| old_with_tou = (1 << SOLIS_BIT_SELF_USE) | (1 << SOLIS_BIT_TOU_MODE) | (1 << SOLIS_BIT_OFF_GRID) # bits 0,1,2 = 7 | ||
| value = compute_solis_mode_value(ENUM_SELF_USE, old_with_tou) | ||
| assert not (value & (1 << SOLIS_BIT_TOU_MODE)), "TOU bit should be cleared" | ||
| assert value & (1 << SOLIS_BIT_TOU_MODE), "TOU bit should be set" | ||
| assert not (value & (1 << SOLIS_BIT_OFF_GRID)), "Off-grid bit should be cleared" | ||
| assert value == 33, f"Expected 33, got {value}" | ||
| assert value == 35, f"Expected 35, got {value}" | ||
| print("PASSED: Clears TOU and off-grid bits on mode change") |
There was a problem hiding this comment.
This block’s comments/print text say it “Clears TOU” but the assertions now require the TOU bit to be set and the resulting value to be 35. Update the comment/print wording so it reflects the new behavior (clearing off-grid while ensuring TOU is enabled).
| for device_sn, values in solis_api.cached_values.items(): | ||
| await solis_api.set_storage_mode_if_needed(device_sn, "Feed-in priority") | ||
| await solis_api.set_storage_mode_if_needed(device_sn, "Self-Use") |
There was a problem hiding this comment.
test_solis_api now iterates devices and calls set_storage_mode_if_needed, which can write real settings to customer inverters when someone runs this CLI helper. This should be gated behind an explicit opt-in flag (e.g., --write) or kept commented out, and ideally print a clear warning before performing any writes.
| # Verify correct CID and value (Self-Use = bits 0,5 set = 33) | ||
| call = calls[0] | ||
| assert call["cid"] == SOLIS_CID_STORAGE_MODE, f"Expected CID {SOLIS_CID_STORAGE_MODE}, got {call['cid']}" | ||
| assert call["value"] == "33", f"Expected value '33' (Self-Use), got {call['value']}" | ||
| assert "storage mode to 33" in call["field_description"], "Field description should mention value 33" | ||
| assert call["value"] == "35", f"Expected value '35' (Self-Use), got {call['value']}" | ||
| assert "storage mode to 35" in call["field_description"], "Field description should mention value 35" |
There was a problem hiding this comment.
The comment and assertions here still describe Self-Use as bits 0+5 (=33), but the test now expects 35 (bits 0+1+5, i.e. TOU enabled). Update the comment (and any related wording) so it matches the new meaning/value being asserted.
| # Verify correct CID and value (Feed-in priority = bits 5,6 set = 96) | ||
| call = calls[0] | ||
| assert call["cid"] == SOLIS_CID_STORAGE_MODE, f"Expected CID {SOLIS_CID_STORAGE_MODE}, got {call['cid']}" | ||
| assert call["value"] == "96", f"Expected value '96' (Feed-in priority), got {call['value']}" | ||
| assert "storage mode to 96" in call["field_description"], "Field description should mention value 96" | ||
| assert call["value"] == "98", f"Expected value '98' (Feed-in priority), got {call['value']}" | ||
| assert "storage mode to 98" in call["field_description"], "Field description should mention value 98" |
There was a problem hiding this comment.
The comment still describes Feed-in priority as bits 5+6 (=96), but the test now expects 98 (bits 1+5+6, i.e. TOU enabled). Update the comment so it matches the new asserted value/bitset.
| # Call set_storage_mode_if_needed to change to Self-Use (bits 0,5 = 33) | ||
| await api.set_storage_mode_if_needed(inverter_sn, "Self-Use") | ||
|
|
||
| # Verify read_and_write_cid was called | ||
| calls = api.read_and_write_cid_calls | ||
| assert len(calls) == 1, f"Expected 1 call when mode changes, got {len(calls)}" | ||
|
|
||
| # Verify correct CID and value | ||
| call = calls[0] | ||
| assert call["cid"] == SOLIS_CID_STORAGE_MODE, f"Expected CID {SOLIS_CID_STORAGE_MODE}, got {call['cid']}" | ||
| assert call["value"] == "33", f"Expected value '33' (Self-Use), got {call['value']}" | ||
| assert call["value"] == "35", f"Expected value '35' (Self-Use), got {call['value']}" |
There was a problem hiding this comment.
This test still says it changes to Self-Use with bits 0+5 (=33), but the expected value is now 35 (includes TOU bit 1). Update the explanatory comment to align with the new value.
| # Setup cached value (currently Self-Use = bits 0,5 = 35) | ||
| api.cached_values[inverter_sn] = {SOLIS_CID_STORAGE_MODE: "35"} |
There was a problem hiding this comment.
The cached-value comment says “bits 0,5 = 35”, but 35 implies bits 0,1,5 are set. Update the comment to reflect the actual bit composition (TOU enabled) to avoid confusion when reading/debugging tests.
| ("Self-Use", "35"), # bits 0,1, 5 = 1+2+32 = 35 | ||
| ("Feed-in priority", "98"), # bits 5,6 = 32+64 = 98 |
There was a problem hiding this comment.
These inline explanations for the expected register values omit the TOU bit that was added (e.g., 98 includes bit 1). Update the comments so the described bits add up to the expected values.
No description provided.