shelly, pytest - #3730
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors Shelly device/consumer implementations to share a common HTTP status/generation handler, and expands/adjusts pytest fixtures and expectations around consumer energy accounting and scheduled consumer behavior.
Changes:
- Introduces
status_handler.pyto centralize Shelly generation detection, status requests, and payload parsing across gen1/gen2+ device types. - Updates Shelly inverter/counter/battery components and Shelly EM/PM consumers to use the shared status handler instead of duplicated parsing (and removes the old
constants.py). - Extends measurement logging and control-layer tests/fixtures to include
consumeraggregates and to cover additional consumer control flows.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/modules/devices/shelly/shelly/status_handler.py | New shared Shelly generation/status/parsing utilities used by multiple Shelly modules. |
| packages/modules/devices/shelly/shelly/inverter.py | Uses shared Shelly status/parsing helpers instead of local parsing. |
| packages/modules/devices/shelly/shelly/device.py | Uses shared get_generation() during device initialization. |
| packages/modules/devices/shelly/shelly/counter.py | Uses shared Shelly status/parsing helpers and simplifies optional field assignment. |
| packages/modules/devices/shelly/shelly/bat.py | Uses shared Shelly status/parsing helpers instead of local parsing. |
| packages/modules/devices/shelly/shelly/constants.py | Removes now-redundant constant after moving it into the shared handler. |
| packages/modules/consumers/shelly/shelly_pm/consumer.py | Replaces Modbus-based PM control with Shelly HTTP switching + shared status parsing. |
| packages/modules/consumers/shelly/shelly_pm/config.py | Updates Shelly PM configuration schema to match the new HTTP-based consumer. |
| packages/modules/consumers/shelly/shelly_em/consumer.py | Switches Shelly EM consumer to shared status parsing (no longer wraps Shelly device). |
| packages/modules/consumers/shelly/shelly_em/config.py | Updates Shelly EM configuration schema to align with the new consumer implementation. |
| packages/helpermodules/measurement_logging/test_data_analyse_percentage_totals.json | Adds expected consumer and cp.all aggregates to percentage totals test data. |
| packages/helpermodules/measurement_logging/process_log_unit_test.py | Extends assertions to cover cp.all and consumer aggregates. |
| packages/helpermodules/measurement_logging/process_log_testdata.py | Updates/extends processed/unprocessed log test data to include consumer and revised PV totals. |
| packages/helpermodules/measurement_logging/conftest.py | Updates fixtures to include consumer entries and adjust sh fixture structure. |
| packages/helpermodules/command_test_data.py | Updates legacy-converted processed entry test data for PV and adds consumer aggregates. |
| packages/control/consumer/consumer.py | Adjusts scheduled-plan selection logic and minor messaging/formatting. |
| packages/control/consumer/consumer_test.py | Expands unit tests for consumer control logic (wait-for-start, time charging, scheduling, tariff hours). |
| packages/control/chargelog/chargelog_test.py | Extends charge log fixtures to include an empty consumer section. |
| packages/control/algorithm/filter_chargepoints_test.py | Updates expected priority order in an algorithm unit test. |
Comments suppressed due to low confidence (3)
packages/modules/devices/shelly/shelly/status_handler.py:95
- Direct indexing
meters['freq']can raiseKeyErrorfor devices/firmware that omit frequency, which then gets converted into a generic "unsupported" error. Treatfreqas optional (similar to theswitch:0branch) and use.get().
power = meters['apower'] * factor
frequency = meters['freq']
packages/modules/devices/shelly/shelly/status_handler.py:122
- In the
em1:0branch, per-phasepowers[...]is not multiplied byfactorwhilepoweris. This makespowersinconsistent withpower(and with other branches) wheneverfactor != 1, and also raisesKeyErrorfor missing fields likepf. Applyfactorconsistently and use.get()defaults to avoid incorrectly treating a supported device as unsupported.
powers[phase-1] = meters['act_power']
voltages[phase-1] = meters['voltage']
currents[phase-1] = meters['current'] * factor
power_factors[phase-1] = meters['pf']
power = meters['act_power'] * factor # shelly Pro EM Gen 2
packages/modules/devices/shelly/shelly/status_handler.py:123
meters['freq']can be absent on someem1:0payloads; raisingKeyErrorhere will misclassify a supported device as unsupported. Use.get()and keepfrequencyoptional.
frequency = meters['freq']
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if 'model' in device_info: | ||
| model = str(device_info['model']) | ||
| elif 'type' in device_info: | ||
| model = str(device_info['type']) | ||
| log.debug(f"Device {model} at {address} is generation {generation}") |
| filtered_plans = [d for d in plans_diff_end_date if list(d.values())[0] > (self.BUFFER_AFTER_END_TIME) * -1] | ||
| if filtered_plans: | ||
| sorted_plans = sorted(plans_diff_end_date, key=lambda x: list(x.values())[0]) | ||
| for p in sorted_plans: | ||
| if self.BUFFER_AFTER_END_TIME < list(p.values())[0]: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (12)
packages/modules/devices/shelly/shelly/status_handler.py:31
request_status()fetches the status endpoint without authentication. For Shelly configurations that includeusername/passwordthis will return 401 and break updates; consider adding an optionalauthparameter and passing it from callers.
def request_status(address: str, generation: Optional[int]) -> Dict:
if generation == 1:
status_url = "http://" + address + "/status"
else:
status_url = "http://" + address + "/rpc/Shelly.GetStatus"
return req.get_http_session().get(status_url, timeout=3).json()
packages/modules/devices/shelly/shelly/status_handler.py:13
- Shelly devices can be password-protected, but
get_generation()always calls/shellywithout auth. Since consumers now haveusername/passwordfields, generation detection will fail for protected devices unless these helpers accept/pass auth credentials.
This issue also appears on line 26 of the same file.
def get_generation(address: str) -> Tuple[Optional[int], str]:
device_info = req.get_http_session().get(f"http://{address}/shelly", timeout=3).json()
packages/modules/devices/shelly/shelly/status_handler.py:123
- In the
em1:0(Shelly Pro EM Gen2) branch,poweris multiplied byfactorbut the per-phasepowers[...]entry is not. This makespowersinconsistent withpower(e.g., sign inversion with factor=-1).
meters = status['em1:0']
powers[phase-1] = meters['act_power']
voltages[phase-1] = meters['voltage']
currents[phase-1] = meters['current'] * factor
power_factors[phase-1] = meters['pf']
power = meters['act_power'] * factor # shelly Pro EM Gen 2
packages/modules/devices/shelly/shelly/status_handler.py:128
- Catching
KeyErrorand raising a genericExceptiondrops the original error context/stack, which makes debugging malformed/partial payloads harder. Preserve the original exception via exception chaining and use a more specific exception type.
except KeyError:
raise Exception("unsupported shelly device?")
packages/modules/devices/shelly/shelly/inverter.py:55
update()no longer catches/parses errors from status retrieval/parsing. WithIndependentComponentUpdaterswallowing exceptions, this can fail silently without the previous log message. Consider restoring a try/except around the status handling and logging the exception.
def update(self) -> None:
power = 0
status = request_status(self.address, self.generation)
_, _, currents, _, power, _ = parse_data(self.phase, self.factor, status)
self.peak_filter.check_values(power)
_, exported = self.sim_counter.sim_count(power)
inverter_state = InverterState(
power=power,
currents=currents,
exported=exported
)
self.store.set(inverter_state)
packages/modules/devices/shelly/shelly/counter.py:46
update()includes an unusedpower = 0and no longer logs parsing/network errors (exceptions are swallowed by the device updater). Wrapping status retrieval/parsing in try/except and logging restores debuggability and avoids dead code.
def update(self) -> None:
power = 0
status = request_status(self.address, self.generation)
powers, voltages, currents, power_factors, power, frequency = parse_data(self.phase, self.factor, status)
self.peak_filter.check_values(power)
packages/modules/devices/shelly/shelly/bat.py:54
update()no longer handles/logs parsing/network errors. Since the device updater swallows exceptions, failures can become silent. Consider restoring a try/except and logging (as before) so unsupported devices/payload changes are visible.
def update(self) -> None:
status = request_status(self.address, self.generation)
_, _, currents, _, power, _ = parse_data(self.phase, self.factor, status)
self.peak_filter.check_values(power)
imported, exported = self.sim_counter.sim_count(power)
bat_state = BatState(
power=power,
currents=currents,
imported=imported,
exported=exported
)
self.store.set(bat_state)
packages/modules/consumers/shelly/shelly_pm/config.py:17
- This consumer config declares
factorasOptional[int], but it is used as a numeric multiplier (and may need non-integer values). Using a non-optionalfloatavoidsNone/type coercion issues during config parsing and when multiplying power/current.
def __init__(self,
ip_address: Optional[str] = None,
factor: Optional[int] = -1,
phase: Optional[int] = 1,
channel: int = 0,
packages/modules/consumers/shelly/shelly_em/config.py:16
- This consumer config declares
factorasOptional[int], but it is used as a numeric multiplier (and may need non-integer values). Using a non-optionalfloatavoidsNone/type coercion issues during config parsing and when multiplying power/current.
def __init__(self,
ip_address: Optional[str] = None,
factor: Optional[int] = -1,
phase: Optional[int] = 1,
username: Optional[str] = None,
password: Optional[str] = None) -> None:
packages/modules/consumers/shelly/shelly_pm/consumer.py:20
username/passwordare part of the configuration and are used for switching, but status reads (request_status) and generation detection (get_generation) are called without auth. This will break metering/control for password-protected devices.
def initializer():
nonlocal sim_counter, generation, model
sim_counter = SimCounterConsumer(config.id, ComponentType.CONSUMER)
generation, model = get_generation(config.configuration.ip_address)
packages/modules/consumers/shelly/shelly_em/consumer.py:27
username/passwordexist in the configuration but are not used for status reads or generation detection. For password-protected Shelly devices, this consumer will fail with 401 unless auth is threaded through the status/generation helpers.
def initializer():
nonlocal sim_counter, generation
sim_counter = SimCounterConsumer(config.id, ComponentType.CONSUMER)
generation, _ = get_generation(config.configuration.ip_address)
def error_handler() -> None:
initializer()
def update() -> ConsumerState:
status = request_status(config.configuration.ip_address, generation)
powers, voltages, currents, _, power, _ = parse_data(
config.configuration.phase, config.configuration.factor, status)
imported, exported = sim_counter.sim_count(power)
packages/control/consumer/consumer_test.py:138
TimeCharging.plansis aList[TimeChargingPlanConsumer](seecontrol/consumer/consumer_data.py), but this test uses dicts (e.g.{ "0": ... }). Using lists here better reflects production behavior and prevents accidental reliance on dict truthiness/iteration.
@pytest.mark.parametrize(
"plans, plan_found, expected",
[
pytest.param({}, None, (0,
Consumer.TIME_CHARGING_NO_PLAN_CONFIGURED, Chargemode.STOP), id="no plan defined"),
* add shelly, flake8, pytest consumer chargemodes * fix shelly * existing pytests running * review
No description provided.