Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #835 +/- ##
==========================================
+ Coverage 98.99% 99.05% +0.05%
==========================================
Files 11 11
Lines 1292 1370 +78
==========================================
+ Hits 1279 1357 +78
Misses 13 13 🚀 New features to boost your workflow:
|
|
@YogevBokobza I unsubscribed to avoid the notifications. Please ping when this is ready for review so I can resubscribe. Thank you. |
a4b316c to
2a9b423
Compare
|
Looks like something is wrong with the stale action, the |
|
I'm removing the Stale action because it's not working as expected. |
5a387dc to
c91d031
Compare
c91d031 to
9a94019
Compare
|
Update: |
7ef4f97 to
8669473
Compare
Reviewer's GuideAdds full support for the new token-based Switcher Heater device across the core device model, TCP/UDP protocol handling, high-level API, CLI scripts, documentation, and tests, including heater-specific state parsing and token-based control commands. Sequence diagram for CLI-based heater control and state retrievalsequenceDiagram
actor User
participant ControlScript as control_device_py
participant Api as SwitcherApi
participant Device as HeaterDevice
User->>ControlScript: invoke get_heater_state -c heater -k token -d id -l key -i ip
ControlScript->>Api: create SwitcherApi(DeviceType.HEATER, ip, id, key, token)
ControlScript->>Api: get_heater_state()
Api->>Api: _login()
Api->>Device: send GET_STATE_PACKET2_TYPE2
Device-->>Api: raw_state_response
Api->>Api: SwitcherHeaterStateResponse(raw_state_response)
Api-->>ControlScript: SwitcherHeaterStateResponse
ControlScript-->>User: print heater state, time_left, time_on, auto_shutdown, power, current
User->>ControlScript: invoke turn_on -c heater -k token -d id -l key -i ip -t 15
ControlScript->>Api: create SwitcherApi(DeviceType.HEATER, ip, id, key, token)
ControlScript->>Api: control_device(Command.ON, 15)
Api->>Api: _login()
alt token_present
Api->>Api: build GENERAL_TOKEN_COMMAND with CONTROL_DEVICE_PRECOMMAND and timer
else no_token
Api->>Api: build SEND_CONTROL_PACKET with session_id and timer
end
Api->>Device: send control packet
Device-->>Api: control_response
Api-->>ControlScript: SwitcherBaseResponse
ControlScript-->>User: print control result
Class diagram for new heater-related API and device typesclassDiagram
class DeviceCategory {
<<enumeration>>
HEATER
}
class DeviceType {
<<enumeration>>
HEATER
hex_code : str
protocol_type : int
category : DeviceCategory
token_needed : bool
}
class SwitcherBaseResponse {
+unparsed_response : bytes
+successful : bool
}
class SwitcherHeaterStateResponse {
+state : DeviceState
+time_left : str
+time_on : str
+auto_shutdown : str
+power_consumption : int
+electric_current : float
+__post_init__() void
}
class StateMessageParser {
+get_heater_state() DeviceState
+get_heater_time_left() str
+get_heater_time_on() str
+get_heater_auto_shutdown() str
+get_heater_power_consumption() int
}
class SwitcherApi {
-_device_id : str
-_device_key : str
-_device_type : DeviceType
-_token : str
+control_device(command : Command, minutes : int) SwitcherBaseResponse
+get_heater_state() SwitcherHeaterStateResponse
}
class SwitcherBase {
}
class SwitcherTimedBase {
}
class SwitcherPowerBase {
}
class SwitcherHeater {
+__post_init__() void
}
class BridgeDatagramParser {
+get_heater_state() DeviceState
+get_heater_power_consumption() int
+get_heater_remaining() str
}
class DeviceState {
<<enumeration>>
ON
OFF
}
SwitcherHeaterStateResponse --|> SwitcherBaseResponse
SwitcherApi ..> SwitcherHeaterStateResponse : returns
SwitcherApi ..> SwitcherBaseResponse : returns
SwitcherHeaterStateResponse ..> StateMessageParser : uses
StateMessageParser ..> DeviceState : returns
BridgeDatagramParser ..> DeviceState : returns
DeviceType --> DeviceCategory : has
SwitcherHeater --|> SwitcherTimedBase
SwitcherHeater --|> SwitcherPowerBase
SwitcherHeater --|> SwitcherBase
Flow diagram for UDP discovery and heater device creationflowchart LR
A["UDP datagram received"] --> B["is_switcher_originator()"];
B -->|false| Z["ignore datagram"]
B -->|true| C["parse datagram with BridgeDatagramParser"]
C --> D["get_device_type()"];
D --> E{device_type.category == HEATER};
E -->|no| F["handle other categories (boiler, breeze, runner, light)"]
F --> G["create appropriate Switcher device"]
G --> H["device_callback(device)"]
E -->|yes| I["get_heater_state()"]
I --> J{state == ON};
J -->|yes| K["get_heater_power_consumption()"]
K --> L["electric_current = watts_to_amps(power)"]
J -->|no| M["power_consumption = 0; electric_current = 0.0"]
L --> N["prepare heater timings and auto_shutdown"]
M --> N
N --> O["create SwitcherHeater(device_type, state, ids, token_needed, power, current, remaining, auto_shutdown)"]
O --> P["device_callback(heater)"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- In
SwitcherApi.control_device, you currently switch to the token-basedGENERAL_TOKEN_COMMANDwheneverself._tokenis truthy; to avoid sending a token command to non-token devices, consider gating this onself._device_type.token_needed(and possibly erroring if a token is missing for token-required types). - The new heater parsing helpers in
StateMessageParserandDatagramParserrepeat the same little‑endian hex slicing pattern; factoring out a small utility for converting 4‑byte hex segments to integers/ISO durations would reduce duplication and make the offsets easier to audit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SwitcherApi.control_device`, you currently switch to the token-based `GENERAL_TOKEN_COMMAND` whenever `self._token` is truthy; to avoid sending a token command to non-token devices, consider gating this on `self._device_type.token_needed` (and possibly erroring if a token is missing for token-required types).
- The new heater parsing helpers in `StateMessageParser` and `DatagramParser` repeat the same little‑endian hex slicing pattern; factoring out a small utility for converting 4‑byte hex segments to integers/ISO durations would reduce duplication and make the offsets easier to audit.
## Individual Comments
### Comment 1
<location> `src/aioswitcher/api/messages.py:217-221` </location>
<code_context>
+ )
+ return seconds_to_iso_time(auto_off_seconds)
+
+ def get_heater_state(self) -> DeviceState:
+ """Return the current heater device state."""
+ hex_state = self._hex_response[152:154].decode()
+ states = dict(map(lambda s: (s.value, s), DeviceState))
+ return states[hex_state]
+
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid rebuilding the DeviceState lookup dict on every get_heater_state call
Because `DeviceState` is static, this mapping doesn’t need to be rebuilt on every call. Either inline a simple comprehension (`{s.value: s for s in DeviceState}`) or, preferably, define the mapping once at module level or as a cached property and reuse it to avoid repeated allocations.
Suggested implementation:
```python
)
return seconds_to_iso_time(auto_off_seconds)
def get_heater_state(self) -> DeviceState:
"""Return the current heater device state."""
hex_state = self._hex_response[152:154].decode()
return DEVICE_STATE_BY_VALUE[hex_state]
def get_heater_power_consumption(self) -> int:
```
You also need to define the shared mapping once, near where `DeviceState` is defined in this module, for example:
```python
DEVICE_STATE_BY_VALUE: dict[str, DeviceState] = {state.value: state for state in DeviceState}
```
Place this at module level (after the `DeviceState` enum definition is available) so that `get_heater_state` can use it.
</issue_to_address>
### Comment 2
<location> `tests/test_api_tcp_client.py:203-209` </location>
<code_context>
assert_that(response.unparsed_response).is_equal_to(get_state_response_packet)
+async def test_get_heater_state_function_with_valid_packets(reader_mock, writer_write, connected_api_token_type2_2, resource_path_root):
+ three_packets = _get_dummy_packets(resource_path_root, "login_response", "login2_response", "get_heater_state_response")
+ with patch.object(reader_mock, "read", side_effect=three_packets):
+ response = await connected_api_token_type2_2.get_heater_state()
+ assert_that(writer_write.call_count).is_equal_to(3)
+ assert_that(response).is_instance_of(SwitcherHeaterStateResponse)
+ assert_that(response.unparsed_response).is_equal_to(three_packets[-1])
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend heater state test to assert parsed heater-specific fields, not just type and raw response
This only checks packet count, response type, and raw payload. It doesn’t verify the parsed heater fields introduced in `StateMessageParser` / `SwitcherHeaterStateResponse`. Please also assert the parsed values (e.g., `state`, `time_left`, `time_on`, `auto_shutdown`, `power_consumption`, `electric_current`) against the fixture-based heater response so regressions in offsets/conversion logic are caught.
Suggested implementation:
```python
async def test_get_heater_state_function_with_valid_packets(reader_mock, writer_write, connected_api_token_type2_2, resource_path_root):
three_packets = _get_dummy_packets(
resource_path_root,
"login_response",
"login2_response",
"get_heater_state_response",
)
with patch.object(reader_mock, "read", side_effect=three_packets):
response = await connected_api_token_type2_2.get_heater_state()
assert_that(writer_write.call_count).is_equal_to(3)
assert_that(response).is_instance_of(SwitcherHeaterStateResponse)
assert_that(response.unparsed_response).is_equal_to(three_packets[-1])
# Parsed heater-specific fields
assert_that(response.state).is_equal_to("on")
assert_that(response.time_left).has_minutes(44)
assert_that(response.time_on).has_minutes(16)
assert_that(response.auto_shutdown).has_hours(1)
assert_that(response.power_consumption).is_equal_to(2400)
assert_that(response.electric_current).is_close_to(10.0, within=0.1)
```
1. The helper assertions `has_minutes` / `has_hours` may not exist in your test suite:
- If they do not exist, replace them with direct comparisons to `datetime.timedelta`, for example:
- `assert_that(response.time_left).is_equal_to(timedelta(minutes=44))`
- `assert_that(response.time_on).is_equal_to(timedelta(minutes=16))`
- `assert_that(response.auto_shutdown).is_equal_to(timedelta(hours=1))`
- Ensure `from datetime import timedelta` is imported at the top of `tests/test_api_tcp_client.py` if you use `timedelta`.
2. The exact expected values (`"on"`, `44`, `16`, `1`, `2400`, `10.0`) must match the `get_heater_state_response` fixture contents. If your fixture encodes different values, adjust the literals accordingly so the test truly validates the parsed offsets/conversions for that fixture.
</issue_to_address>
### Comment 3
<location> `tests/test_api_tcp_client.py:145-148` </location>
<code_context>
assert_that(api.connected).is_true()
async def test_api_with_token_needed_but_missing_should_raise_error():
with raises(RuntimeError, match="A token is needed but is missing"):
with patch("aioswitcher.api.open_connection", return_value=b''):
- await SwitcherApi(device_type_token_api2, device_ip, device_id, device_key, token_empty)
+ await SwitcherApi(device_type_token_runner_s11, device_ip, device_id, device_key, token_empty)
</code_context>
<issue_to_address>
**suggestion (testing):** Add a similar token-missing test for the new HEATER device type
Since `DeviceType.HEATER` is also `token_needed=True` and uses the same token-based TCP flow, please add a corresponding test that instantiates `SwitcherApi` with `DeviceType.HEATER` and an empty token and asserts the same `RuntimeError`. You can either duplicate this test for the heater or parametrize it over `[DeviceType.RUNNER_S11, DeviceType.HEATER]` so both are covered.
Suggested implementation:
```python
import pytest
from pytest import raises
```
```python
@pytest.mark.parametrize(
"device_type",
[device_type_token_runner_s11, device_type_token_heater],
)
async def test_api_with_token_needed_but_missing_should_raise_error(device_type):
with raises(RuntimeError, match="A token is needed but is missing"):
with patch("aioswitcher.api.open_connection", return_value=b''):
await SwitcherApi(device_type, device_ip, device_id, device_key, token_empty)
```
If `import pytest` already exists in this file, remove the duplicate import and keep a single `import pytest` at the top. No other changes should be necessary, since `device_type_token_heater` is already defined at the bottom of the file.
</issue_to_address>
### Comment 4
<location> `docs/scripts.md:173` </location>
<code_context>
the type of the device
-k TOKEN, --token TOKEN
- the token for communicating with the new switcher devices
+ the token for communicating with switcher token based devices
-d DEVICE_ID, --device-id DEVICE_ID
the identification of the device
</code_context>
<issue_to_address>
**suggestion (typo):** Consider hyphenating "token-based" in this phrase for correct grammar.
As it’s used as a compound adjective here, it should take a hyphen; please update all occurrences in this file for consistency.
Suggested implementation:
```
-k TOKEN, --token TOKEN
the token for communicating with switcher token-based devices
```
```
-k TOKEN, --token TOKEN
the token for communicating with switcher token-based devices
```
</issue_to_address>
### Comment 5
<location> `docs/supported.md:1` </location>
<code_context>
-| Switcher Light SL02 | [product][switcher-light-sl02] | 4.3.x |
-| Switcher Light SL02 Mini | [product][switcher-light-sl02-mini] | 4.3.x |
-| Switcher Light SL03 | [product][switcher-light-sl03] | 4.4.x |
+| Name | Link | Included from version | Is require a token |
+|--------------------------|:-----------------------------------:|:---------------------:|:------------------:|
+| Switcher V2 | [product][switcher-v2] | 1.x.x | No |
</code_context>
<issue_to_address>
**issue (typo):** Column header "Is require a token" is ungrammatical; consider rephrasing.
Consider alternatives like "Requires token" or "Token required" for a clearer, grammatically correct header.
```suggestion
| Name | Link | Included from version | Requires token |
```
</issue_to_address>
### Comment 6
<location> `docs/usage_api.md:155` </location>
<code_context>
+async def control_heater(device_type, device_ip, device_id, device_key, token) :
+ # for connecting to a device we need its type, id, login key, ip address and token
+ async with SwitcherApi(device_type, device_ip, device_id, device_key, token) as api:
+ # get the device current state
+ await api.get_heater_state()
+ # turn the device on for 15 minutes
</code_context>
<issue_to_address>
**nitpick (typo):** Minor grammar tweak: "device current state" → "device's current state".
You could also adjust the verb to "get the device's current state" for smoother wording.
```suggestion
# get the device's current state
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def get_heater_state(self) -> DeviceState: | ||
| """Return the current heater device state.""" | ||
| hex_state = self._hex_response[152:154].decode() | ||
| states = dict(map(lambda s: (s.value, s), DeviceState)) | ||
| return states[hex_state] |
There was a problem hiding this comment.
suggestion (performance): Avoid rebuilding the DeviceState lookup dict on every get_heater_state call
Because DeviceState is static, this mapping doesn’t need to be rebuilt on every call. Either inline a simple comprehension ({s.value: s for s in DeviceState}) or, preferably, define the mapping once at module level or as a cached property and reuse it to avoid repeated allocations.
Suggested implementation:
)
return seconds_to_iso_time(auto_off_seconds)
def get_heater_state(self) -> DeviceState:
"""Return the current heater device state."""
hex_state = self._hex_response[152:154].decode()
return DEVICE_STATE_BY_VALUE[hex_state]
def get_heater_power_consumption(self) -> int:You also need to define the shared mapping once, near where DeviceState is defined in this module, for example:
DEVICE_STATE_BY_VALUE: dict[str, DeviceState] = {state.value: state for state in DeviceState}Place this at module level (after the DeviceState enum definition is available) so that get_heater_state can use it.
| async def test_get_heater_state_function_with_valid_packets(reader_mock, writer_write, connected_api_token_type2_2, resource_path_root): | ||
| three_packets = _get_dummy_packets(resource_path_root, "login_response", "login2_response", "get_heater_state_response") | ||
| with patch.object(reader_mock, "read", side_effect=three_packets): | ||
| response = await connected_api_token_type2_2.get_heater_state() | ||
| assert_that(writer_write.call_count).is_equal_to(3) | ||
| assert_that(response).is_instance_of(SwitcherHeaterStateResponse) | ||
| assert_that(response.unparsed_response).is_equal_to(three_packets[-1]) |
There was a problem hiding this comment.
suggestion (testing): Extend heater state test to assert parsed heater-specific fields, not just type and raw response
This only checks packet count, response type, and raw payload. It doesn’t verify the parsed heater fields introduced in StateMessageParser / SwitcherHeaterStateResponse. Please also assert the parsed values (e.g., state, time_left, time_on, auto_shutdown, power_consumption, electric_current) against the fixture-based heater response so regressions in offsets/conversion logic are caught.
Suggested implementation:
async def test_get_heater_state_function_with_valid_packets(reader_mock, writer_write, connected_api_token_type2_2, resource_path_root):
three_packets = _get_dummy_packets(
resource_path_root,
"login_response",
"login2_response",
"get_heater_state_response",
)
with patch.object(reader_mock, "read", side_effect=three_packets):
response = await connected_api_token_type2_2.get_heater_state()
assert_that(writer_write.call_count).is_equal_to(3)
assert_that(response).is_instance_of(SwitcherHeaterStateResponse)
assert_that(response.unparsed_response).is_equal_to(three_packets[-1])
# Parsed heater-specific fields
assert_that(response.state).is_equal_to("on")
assert_that(response.time_left).has_minutes(44)
assert_that(response.time_on).has_minutes(16)
assert_that(response.auto_shutdown).has_hours(1)
assert_that(response.power_consumption).is_equal_to(2400)
assert_that(response.electric_current).is_close_to(10.0, within=0.1)- The helper assertions
has_minutes/has_hoursmay not exist in your test suite:- If they do not exist, replace them with direct comparisons to
datetime.timedelta, for example:assert_that(response.time_left).is_equal_to(timedelta(minutes=44))assert_that(response.time_on).is_equal_to(timedelta(minutes=16))assert_that(response.auto_shutdown).is_equal_to(timedelta(hours=1))
- Ensure
from datetime import timedeltais imported at the top oftests/test_api_tcp_client.pyif you usetimedelta.
- If they do not exist, replace them with direct comparisons to
- The exact expected values (
"on",44,16,1,2400,10.0) must match theget_heater_state_responsefixture contents. If your fixture encodes different values, adjust the literals accordingly so the test truly validates the parsed offsets/conversions for that fixture.
There was a problem hiding this comment.
I was suggesting one more assertion layer for that new heater test, not pointing out a bug.
Right now this test only checks:
- we sent 3 packets
- we got a
SwitcherHeaterStateResponse - the raw payload matches the last dummy packet
Because heater support adds new parsed fields (state, time_left, power, etc.), it’s useful to also assert that those parsed values match what’s actually encoded in get_heater_state_response.txt. That way, if someone later breaks the parsing offsets or unit conversions, this test will fail.
Concretely, I’d recommend extending the test like this (using whatever values are actually in your fixture):
async def test_get_heater_state_function_with_valid_packets(
reader_mock,
writer_write,
connected_api_token_type2_2,
resource_path_root,
):
three_packets = _get_dummy_packets(
resource_path_root,
"login_response",
"login2_response",
"get_heater_state_response",
)
with patch.object(reader_mock, "read", side_effect=three_packets):
response = await connected_api_token_type2_2.get_heater_state()
assert_that(writer_write.call_count).is_equal_to(3)
assert_that(response).is_instance_of(SwitcherHeaterStateResponse)
assert_that(response.unparsed_response).is_equal_to(three_packets[-1])
# Also validate parsed heater values from the fixture
# NOTE: adjust these to match what get_heater_state_response.txt encodes
assert_that(response.state).is_equal_to("on")
assert_that(response.time_left).is_equal_to(timedelta(minutes=44))
assert_that(response.time_on).is_equal_to(timedelta(minutes=16))
assert_that(response.auto_shutdown).is_equal_to(timedelta(hours=1))
assert_that(response.power_consumption).is_equal_to(2400)
assert_that(response.electric_current).is_close_to(10.0, within=0.1)If the fixture encodes different values, just change the literals accordingly. If you prefer to keep the test minimal and consistent with the existing test_get_state_function_with_valid_packets, I’m fine with that too; this was just a suggestion to make heater parsing a bit more strongly covered by tests.
Happy to adjust or drop the suggestion if it doesn’t fit your testing style for this project.
| async def test_api_with_token_needed_but_missing_should_raise_error(): | ||
| with raises(RuntimeError, match="A token is needed but is missing"): | ||
| with patch("aioswitcher.api.open_connection", return_value=b''): | ||
| await SwitcherApi(device_type_token_api2, device_ip, device_id, device_key, token_empty) | ||
| await SwitcherApi(device_type_token_runner_s11, device_ip, device_id, device_key, token_empty) |
There was a problem hiding this comment.
suggestion (testing): Add a similar token-missing test for the new HEATER device type
Since DeviceType.HEATER is also token_needed=True and uses the same token-based TCP flow, please add a corresponding test that instantiates SwitcherApi with DeviceType.HEATER and an empty token and asserts the same RuntimeError. You can either duplicate this test for the heater or parametrize it over [DeviceType.RUNNER_S11, DeviceType.HEATER] so both are covered.
Suggested implementation:
import pytest
from pytest import raises@pytest.mark.parametrize(
"device_type",
[device_type_token_runner_s11, device_type_token_heater],
)
async def test_api_with_token_needed_but_missing_should_raise_error(device_type):
with raises(RuntimeError, match="A token is needed but is missing"):
with patch("aioswitcher.api.open_connection", return_value=b''):
await SwitcherApi(device_type, device_ip, device_id, device_key, token_empty)If import pytest already exists in this file, remove the duplicate import and keep a single import pytest at the top. No other changes should be necessary, since device_type_token_heater is already defined at the bottom of the file.
There was a problem hiding this comment.
My earlier comment was suggesting an extra test, not indicating that something is broken.
Right now there’s a test that checks the runner S11 token device fails when the token is missing:
async def test_api_with_token_needed_but_missing_should_raise_error():
with raises(RuntimeError, match="A token is needed but is missing"):
with patch("aioswitcher.api.open_connection", return_value=b''):
await SwitcherApi(device_type_token_runner_s11, device_ip, device_id, device_key, token_empty)The new heater device type (device_type_token_heater = DeviceType.HEATER) also requires a token, so the idea is to verify the same behavior for it. There are two options:
- Parametrize the existing test to cover both devices (preferred, less duplication):
@pytest.mark.parametrize(
"device_type",
[device_type_token_runner_s11, device_type_token_heater],
)
async def test_api_with_token_needed_but_missing_should_raise_error(device_type):
with raises(RuntimeError, match="A token is needed but is missing"):
with patch("aioswitcher.api.open_connection", return_value=b''):
await SwitcherApi(device_type, device_ip, device_id, device_key, token_empty)- Or add a second, heater-specific test that mirrors the existing one for runner S11.
If you’d prefer, I can adapt the suggestion to fit your testing style (e.g., keep tests un-parametrized, different naming, etc.).
fdee4af to
9486641
Compare
|
@YogevBokobza Looks like the AI is going crazy today, Sourcery finally reported stuff and my local Claude tried to fix on my behalf. I forced pushed, returning it to your original commit, can you please address Sourcery's comments? |
I come back in my opinion.. this AI stuff makes me work harder than actually improve.. :| |
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Signed-off-by: YogevBokobza <yogevbokobza12@gmail.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Signed-off-by: YogevBokobza <yogevbokobza12@gmail.com>
|
@TomerFi |
|
Let's merge then. |
|
@YogevBokobza @thecode Thank you both, this was included in release 6.1.0. |
Description
Adding heater support.
heater is a new device, token based that acts more or less like Switcher Boiler devices.
This will solve the issue: #844
Checklist
Additional information
Summary by Sourcery
Add first-class support for Switcher Heater devices alongside existing device types, including discovery, control, and state retrieval.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests