Conversation
Pins the upper bound to <2.0.0a1 so that any 2.x alpha/beta/rc builds published during v2 development don't get pulled into installations on the stable branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When consecutive timeouts exceeded the tolerance threshold the client was left open, causing the reconnect check to see a live socket and never attempt a new connection. Reset the client before raising UpdateFailed so the next tick triggers a clean reconnect. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Points uv at the local givenergy-modbus checkout while v2.0.0 is in development and not yet published to PyPI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Switches from a local editable path to tracking the upstream git repo so the next branch always reflects the latest givenergy-modbus main. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
givenergy-modbus renamed Inverter to SinglePhaseInverter; the alias still works but emits a deprecation warning and will be removed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Dangerous one-shot commands are registered as domain services rather than button entities so they only appear in Developer Tools and cannot be accidentally triggered from a dashboard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Prevents accidental multi-inverter firing by resolving a required device_id field to a single coordinator via the device registry. Services.yaml gains a device selector filtered to this integration. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Call client.detect() once on connect (coordinator + config flow) so refresh_plant() dispatches via the new model-aware load_config()/refresh() path. This unlocks support for three-phase, AIO-HV, EMS and other non-default topologies. - Replace the deprecated Inverter import with a shared InverterModel = SinglePhaseInverter | ThreePhaseInverter alias defined in coordinator.py. - Rebuild the time platform around commands.set_charge_slot / set_discharge_slot using plant.inverter.slot_map, replacing the eight deprecated set_charge_slot_N / set_discharge_slot_N wrappers. - Remove the max_batteries config option entirely — capabilities.lv_battery_addresses drives battery enumeration now, so the user-facing field, schema entry, coordinator parameter and translations are all dead weight.
Both reboot_inverter and calibrate_battery_soc previously silently no-op'd if the device_id was invalid, the coordinator was missing, or the Modbus client was disconnected — the call appeared to succeed while no command was actually sent. Also add a detect() assertion to the first-refresh test so the topology-discovery step doesn't quietly regress.
The git override pointed dev installs at the library's main branch while HACS users got whatever was pinned in manifest.json. Now that 2.0.0a1 is published to PyPI, drop the override so `uv sync` resolves the alpha from PyPI — dev/CI exercise the exact artifact end users install. Each subsequent library alpha (2.0.0a2, …) will arrive via the bump-givenergy-modbus workflow rather than `branch = main` drift.
Add 20 new inverter sensors: battery_calibration_stage (enum), inverter_fault_messages (decoded string), i_grid_port, v_p_bus, v_n_bus, e_solar_diverter, second-stack battery energy (×4), battery_pause_mode (upgraded to enum), battery_maintenance_mode (enum, three-phase only), usb_device_inserted (enum). Add 10 new per-battery sensors: cap_calibrated, BMS status bytes 1–7, BMS warning bytes 1–2. Add translations for all new enum sensors (battery_pause_mode, battery_maintenance_mode, battery_calibration_stage, usb_device_inserted) in en.json. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Switch from manual release creation to automatic: pushing a v* tag now creates the GitHub release, marks it as pre-release when the tag contains a/b/rc, patches manifest.json, and attaches the zip — all without a separate gh release create step. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the battery view with a sections layout so each battery appears as a side-by-side column: SOC gauge → pack details → cell voltages → cell temperatures. Template includes two battery sections; add/remove as needed. Also fix the three energy bar charts: remove chart_type: bar (not a valid apexcharts-card ChartType) and add type: column per series. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Generates a complete Lovelace dashboard YAML with serial numbers pre-filled. Battery view dynamically builds one column per battery serial passed, using HA's sections layout for side-by-side comparison. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Registers a givenergy_local.generate_dashboard service that auto-detects all connected inverters and batteries, generates a Lovelace dashboard YAML using the actual serial numbers, and writes it to the HA config directory. A persistent notification shows the file path on completion. Extracts the generation logic into custom_components/givenergy_local/lovelace.py so it is shared by both the service and the standalone lovelace/generate.py script. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
HA deprecated the "Lovelace" branding in favour of "Dashboards".
Renames the lovelace/ directory to dashboard/, dashboard.yaml to
template.yaml, and lovelace.py to dashboard.py. Updates all internal
references and user-facing strings accordingly. Also writes generated
files to www/ as dashboard_givenergy_{serial}.yaml so they are
accessible via /local/ without filesystem access.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The library's "Connection established" line is emitted from givenergy_modbus.client.client at INFO, which gets filtered when only the integration's own logger is enabled — making the close/reconnect cycle hard to trace from HA logs. Emit matching messages from the coordinator itself so visibility doesn't depend on enabling a second logger. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The tolerance threshold used strict-greater (`>`) semantics, so `timeout_tolerance=N` actually allowed N tolerated failures and reset on the N+1th — surprising both as a config knob (the number doesn't match the user's mental model) and in practice (with the previous default of 5 and a 30s scan interval, recovery only began after 3+ minutes of timeouts, when on most setups it succeeds on the next tick). Switch to `>=` so the config value means "reset on the Nth consecutive failure", and drop the default from 5 to 3. Existing entries keep their saved value untouched — only new installs and entries that haven't been reconfigured pick up the new default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
uv invokes the build backend during dependency resolution even when [tool.uv] package = false. Without an explicit [build-system], that falls through to the PEP 517 legacy default of setuptools, whose flat-layout auto-discovery sees both custom_components/ and the new dashboard/ directory as candidate top-level packages and refuses to proceed — failing uv lock on any pyproject.toml dependency change with "Multiple top-level packages discovered in a flat-layout". The project isn't distributed as a pip package; it's a HACS-installed HA custom component. Pin the build backend explicitly to uv_build, which is uv's own first-party backend and a natural fit for projects that exist solely so uv can resolve a dev venv. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a retries option to the config flow that's forwarded to refresh_plant() so a single dropped Modbus request can be retried within the same tick rather than counting toward the tolerance window and triggering a connection reset. With a fast refresh being just a handful of requests and each retry costing at most ~1s, the latency impact is negligible while single-blip recovery becomes invisible to HA entities. Bump the manifest pin to givenergy-modbus>=2.0.0a2 so the option only ships once the library actually forwards timeout/retries through load_config()/refresh() (fixed on library main in 260af42). The pyproject pin stays on a1 until 2.0.0a2 lands on PyPI; manifest.json is the runtime-effective pin for HACS installs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brings the dev pin in line with manifest.json now that 2.0.0a2 is on PyPI, picking up the timeout/retries param-forwarding fix (260af42) and the bounds-violation log noise fix (a4501d7). uv_build requires a static project version even with package = false, so replace the dynamic version with a 0.0.0 stub and note that the authoritative version still lives in manifest.json for HACS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Records the read/write interleaving model (HA lock serialises ticks, one_shot_command writes can land between them, shape-hash demux keeps them apart) and the one invariant that holds it all together: detect() must not run while other requests can be in flight. Documents both why the invariant holds today (detect only runs at connect, under the coordinator lock, before entities exist) and what would break it (moving detect onto a hot path), so future refactors don't have to re-derive this from first principles. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously the workflow always opened the bump PR against main. With v1.0 development now tracking modbus 2.x on its own branch while main stays on modbus 1.x, the right target depends on the incoming version. Resolve the base from the major version up-front (1 → main, 2 → v1.0, otherwise error), check out that branch, and pass it through to the PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captures the baseline ruleset (no force-push, no deletion, require validate.yml checks) for main + v1.0 as a re-runnable shell script rather than a one-time gh api invocation. Looks up the ruleset by name and PUTs to update if it exists, POSTs to create otherwise — so future tweaks (adding a new branch, changing required checks) are just edits to the script. Owner is bypass-listed so deliberate maintenance still works. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GITHUB_TOKEN-authored actions don't fire downstream workflows (anti-loop protection by design), which left auto-bump PRs without validate.yml checks running — meaning they couldn't satisfy the required-status-checks rule and needed a manual close/reopen to kick CI. Pull the PR creation credential from a repo PAT secret instead so push and pr-create events trigger pull_request workflows like a human-authored PR would. Requires a fine-grained PAT to be stored as the BUMP_PAT repo secret with contents:read+write and pull-requests:read+write on this repo. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The library now ships a calibrated retry_delay default and the in-tick recovery story is mostly handled there. The two knobs we exposed (retries in v1.0.0a6, timeout_tolerance since v0.4.1) were weakly named — both invite users to set higher values that actually make their integration worse, not better — and the failure mode that motivated them is now better mitigated upstream. Remove both fields from the config-flow form and bump ConfigFlow.VERSION to 2. async_migrate_entry strips the legacy fields from any existing entry so storage stays clean, and the coordinator falls back to its own constructor defaults (timeout_tolerance=3, retries=1). Anyone who'd customised the values gets reset to the new defaults on next load; if those don't fit a specific installation, the right path is an issue rather than a config knob. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Promotes the read-only battery_pause_mode sensor to a writable select (matching the existing battery_power_mode pattern) and adds a pair of time entities for the pause slot's start and end, mirroring how charge/discharge slots are exposed. Library has the corresponding write commands (set_battery_pause_mode and set_pause_slot) and the registers (318-320) are in the WRITE_SAFE_REGISTERS allowlist. Refactors GivEnergyTimeEntityDescription to take a single setter_fn callable instead of the previous is_charge/slot_index combo. The existing charge/discharge entries now bind set_charge_slot or set_discharge_slot directly, and the new pause entries bind set_pause_slot. Uniform shape positions the file cleanly for the upcoming library migration to single-endpoint slot writes (one register per user edit, no read-modify-write race on the other end). Note for upgraders: the entity ID changes platform — the old sensor.givenergy_battery_pause_mode will be orphaned and a new select.givenergy_battery_pause_mode appears. Anyone with dashboards or automations referencing the sensor will need to repoint them to the select. Pre-release window is the right time for the break. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The new single-endpoint setters in givenergy-modbus 2.0.0a5 (set_charge_slot_start, set_charge_slot_end, set_pause_slot_start, etc.) write just the register being edited, removing the read-modify-write race that lived in async_set_value. Previously, editing one endpoint round-tripped the other endpoint's value from the last coordinator refresh — with a 30s scan interval, rapid edits to start and end could clobber each other. Drops async_set_value's reconstruction of the whole TimeSlot and binds each entity's setter_fn directly to the relevant start/end-specific library setter. setter_fn signature changes from (TimeSlot, InverterModel) -> list to (dt_time, InverterModel) -> list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpgrade to givenergy-modbus v2: refactor coordinator and entity types, add config-entry migration, new HA services and dashboard generation/repair flows, expand diagnostics and time/select behavior, update tests, and adjust CI/workflows and tooling. Changesgivenergy-modbus v2 Integration & Service Enhancements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom_components/givenergy_local/time.py (1)
149-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard writes when the slot is unavailable on the current inverter model.
async_set_valuenow sends a command unconditionally. If the slot is unsupported (slot_fn(...)returnsNone), this still attempts a write via service/API calls.Proposed fix
async def async_set_value(self, value: dt_time) -> None: client = self.coordinator._client if client is None or not client.connected: return inverter = self.coordinator.data.inverter + if self.entity_description.slot_fn(inverter) is None: + return await client.one_shot_command(self.entity_description.setter_fn(value, inverter)) await self.coordinator.async_request_refresh()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/givenergy_local/time.py` around lines 149 - 155, async_set_value currently calls client.one_shot_command with the result of self.entity_description.setter_fn(value, inverter) without checking for None; if the slot is unsupported setter_fn may return None and you end up issuing an invalid write. Update async_set_value to call self.entity_description.setter_fn(value, inverter), store the result in a local (e.g., cmd), check if cmd is None and if so simply return (or log/debug and return) instead of calling client.one_shot_command, otherwise proceed to await client.one_shot_command(cmd) and then await self.coordinator.async_request_refresh(); reference async_set_value, self.entity_description.setter_fn, client.one_shot_command, and self.coordinator.async_request_refresh in your change.
🧹 Nitpick comments (1)
tests/test_init.py (1)
73-75: ⚡ Quick winAssert the migration-error state explicitly in the future-version test.
Line 73’s comment says HA marks the entry as migration error, but the test doesn’t assert that state. Adding it makes the contract explicit.
Proposed fix
+from homeassistant import config_entries from pytest_homeassistant_custom_component.common import MockConfigEntry @@ # Setup is expected to fail; HA marks the entry as migration_error. assert not await hass.config_entries.async_setup(entry.entry_id) + assert entry.state is config_entries.ConfigEntryState.MIGRATION_ERROR🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_init.py` around lines 73 - 75, The test should explicitly assert that the config entry is marked as a migration error after setup fails; after calling await hass.config_entries.async_setup(entry.entry_id) (or immediately after the failing assert), retrieve the entry via hass.config_entries.async_get_entry(entry.entry_id) and assert its .state equals config_entries.ConfigEntryState.MIGRATION_ERROR (or the equivalent enum used in the test) so the test documents the expected migration_error state for the future-version entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@custom_components/givenergy_local/dashboard.py`:
- Around line 71-72: The dashboard is using the sensor entity for
battery_pause_mode (entity: {_i(inv, "battery_pause_mode")}) but the UI should
target the writable select entity; update the entries that create the "Pause
Mode" controls/status (the occurrences around the existing entity: {_i(inv,
"battery_pause_mode")} / name: Pause Mode) to reference the select domain's
canonical entity id (i.e. the select.givenergy_inverter_*_battery_pause_mode
canonical id) instead of the sensor one so the controls card targets the
writable select and the status card uses the same canonical id; locate the two
occurrences (around the shown block and the similar block at lines 266-267) and
change the entity references to the select entity id generation used elsewhere
in the file.
- Around line 215-218: The string literals for the battery cell ranges ("Cells
1–4", "Cells 5–8", "Cells 9–12", "Cells 13–16") contain EN DASH characters
(U+2013) that trigger the ambiguous-unicode lint; replace each EN DASH with a
plain ASCII hyphen-minus ("-") so they read "Cells 1-4", "Cells 5-8", "Cells
9-12", "Cells 13-16" (these appear alongside the keys "cells_1_4_temperature",
"cells_5_8_temperature", "cells_9_12_temperature", "cells_13_16_temperature");
after updating, run ruff fix/format (uv run ruff check --fix && uv run ruff
format) to ensure the lint is cleared.
In `@dashboard/template.yaml`:
- Around line 49-50: The template incorrectly uses
sensor.givenergy_inverter_INVERTER_SERIAL_battery_pause_mode (a read-only
sensor) — change it to the writable select domain by replacing occurrences with
select.givenergy_inverter_INVERTER_SERIAL_battery_pause_mode in both the
overview and controls blocks (also update the other occurrence referenced in the
file), ensuring the entity id uses the select prefix and the same name "Pause
Mode".
In `@tests/test_config_flow.py`:
- Around line 97-103: The current filter on
mock_client.refresh_plant.call_args_list compares kwargs for exact equality
which misses calls that include full_refresh=False plus extra kwargs; change the
comprehension that builds test_connection_calls to select calls where
c.kwargs.get("full_refresh") is False (or where "full_refresh" in c.kwargs and
c.kwargs["full_refresh"] is False) so the test detects any _test_connection
invocation regardless of additional kwargs—update the variable that constructs
test_connection_calls (the list comprehension referencing
mock_client.refresh_plant.call_args_list) accordingly.
---
Outside diff comments:
In `@custom_components/givenergy_local/time.py`:
- Around line 149-155: async_set_value currently calls client.one_shot_command
with the result of self.entity_description.setter_fn(value, inverter) without
checking for None; if the slot is unsupported setter_fn may return None and you
end up issuing an invalid write. Update async_set_value to call
self.entity_description.setter_fn(value, inverter), store the result in a local
(e.g., cmd), check if cmd is None and if so simply return (or log/debug and
return) instead of calling client.one_shot_command, otherwise proceed to await
client.one_shot_command(cmd) and then await
self.coordinator.async_request_refresh(); reference async_set_value,
self.entity_description.setter_fn, client.one_shot_command, and
self.coordinator.async_request_refresh in your change.
---
Nitpick comments:
In `@tests/test_init.py`:
- Around line 73-75: The test should explicitly assert that the config entry is
marked as a migration error after setup fails; after calling await
hass.config_entries.async_setup(entry.entry_id) (or immediately after the
failing assert), retrieve the entry via
hass.config_entries.async_get_entry(entry.entry_id) and assert its .state equals
config_entries.ConfigEntryState.MIGRATION_ERROR (or the equivalent enum used in
the test) so the test documents the expected migration_error state for the
future-version entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 645924f6-c732-4db5-9da5-cd9d1d45919d
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.github/workflows/bump-givenergy-modbus.yml.github/workflows/release.yml.gitignorecustom_components/givenergy_local/__init__.pycustom_components/givenergy_local/config_flow.pycustom_components/givenergy_local/const.pycustom_components/givenergy_local/coordinator.pycustom_components/givenergy_local/dashboard.pycustom_components/givenergy_local/manifest.jsoncustom_components/givenergy_local/number.pycustom_components/givenergy_local/select.pycustom_components/givenergy_local/sensor.pycustom_components/givenergy_local/services.yamlcustom_components/givenergy_local/strings.jsoncustom_components/givenergy_local/switch.pycustom_components/givenergy_local/time.pycustom_components/givenergy_local/translations/en.jsondashboard/generate.pydashboard/template.yamlpyproject.tomlscripts/apply-branch-protection.shtests/conftest.pytests/test_config_flow.pytests/test_coordinator.pytests/test_init.pytests/test_select.pytests/test_sensor.pytests/test_time.py
💤 Files with no reviewable changes (2)
- custom_components/givenergy_local/strings.json
- tests/test_sensor.py
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades the integration to support givenergy-modbus v2.0, introducing automatic topology detection and support for three-phase inverters. It adds new services for inverter rebooting, battery SOC calibration, and automated Lovelace dashboard generation. Config entries are migrated to version 2, removing manual battery count and timeout settings in favor of library-managed defaults. Feedback includes correcting outdated entity references in the dashboard generator, fixing a NameError in the sensor definitions, and moving a blocking I/O operation to the executor to avoid stalling the event loop.
| name=f"BMS Status {i}", | ||
| state_class=SensorStateClass.MEASUREMENT, | ||
| entity_category=EntityCategory.DIAGNOSTIC, | ||
| value_fn=_battery_attr(f"status_{i}"), |
There was a problem hiding this comment.
The helper function _battery_attr does not appear to be defined in this file or imported. This will cause a NameError at runtime. Consider using a lambda with a default argument to capture the loop variable and avoid the closure trap, consistent with the pattern used for cell voltages below.
Suggested change
| value_fn=_battery_attr(f"status_{i}"), | |
| value_fn=lambda bat, i=i: getattr(bat, f"status_{i}"), |
The earlier sensor→select move for battery_pause_mode left stale references in the dashboard generator and template, which would have shipped broken entity links to anyone generating a dashboard against v1.0. Repoint both call sites at the new select.* entity. Other inline review fixes: - dashboard.py: replace EN DASH characters in "Cells N-M" labels with plain ASCII hyphen-minus (Ruff RUF001 hygiene, even though we don't enable that rule in our config — these are noise either way). - __init__.py: move the synchronous www_dir.mkdir() call into the executor so it doesn't block the event loop. Sibling write_text was already in the executor; this brings the two calls in line. - tests/test_config_flow.py: relax the test_connection_calls filter to match on full_refresh=False instead of exact kwargs equality, so the test isn't falsely-pass-prone if refresh_plant gains additional kwargs (it now passes retries=1 in the active update path). - tests/test_init.py: assert ConfigEntryState.MIGRATION_ERROR on the future-version test rather than relying on the implicit boolean from async_setup; makes the contract explicit. Skipped: - Gemini flagged _battery_attr as undefined at sensor.py:781 — false positive, it's defined at sensor.py:52. - CodeRabbit suggested guarding async_set_value when slot_fn returns None. With single-endpoint writes, the setter writes one register directly; the read-from-current-slot pattern that motivated the guard is gone. Adding it would silently swallow user input rather than letting the inverter NAK an invalid register write. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
custom_components/givenergy_local/__init__.py (1)
113-113: ⚡ Quick winCapture
www_direxplicitly to avoid late-binding in the lambda.While this code is safe in practice because the
awaitensures the executor job completes before the next loop iteration, the lambda captureswww_dirby reference. This triggers ruff B023 and is better fixed for clarity and maintainability.♻️ Proposed fix using default argument capture
- await hass.async_add_executor_job(lambda: www_dir.mkdir(exist_ok=True)) + await hass.async_add_executor_job(lambda d=www_dir: d.mkdir(exist_ok=True))As per coding guidelines, run
uv run ruff check --fix && uv run ruff formatbefore committing code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/givenergy_local/__init__.py` at line 113, Replace the late-bound lambda passed to hass.async_add_executor_job so www_dir is captured explicitly (use a default-argument capture) before calling www_dir.mkdir; update the call that currently wraps www_dir.mkdir in a lambda to capture www_dir in the lambda signature (referencing hass.async_add_executor_job and www_dir.mkdir) and then run uv run ruff check --fix && uv run ruff format before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@custom_components/givenergy_local/__init__.py`:
- Line 113: Replace the late-bound lambda passed to hass.async_add_executor_job
so www_dir is captured explicitly (use a default-argument capture) before
calling www_dir.mkdir; update the call that currently wraps www_dir.mkdir in a
lambda to capture www_dir in the lambda signature (referencing
hass.async_add_executor_job and www_dir.mkdir) and then run uv run ruff check
--fix && uv run ruff format before committing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0467ade-9ff9-48d3-bb8c-3a0b3d3f6d2e
📒 Files selected for processing (5)
custom_components/givenergy_local/__init__.pycustom_components/givenergy_local/dashboard.pydashboard/template.yamltests/test_config_flow.pytests/test_init.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_config_flow.py
- tests/test_init.py
- custom_components/givenergy_local/dashboard.py
- dashboard/template.yaml
Trivial nitpick from CodeRabbit. Our ruff config doesn't enable flake8-bugbear so this wasn't actually flagged in CI, but the default-arg capture is clearer about the intent (lambda binds the current www_dir, not whatever the name happens to refer to when the executor runs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@custom_components/givenergy_local/manifest.json`:
- Line 12: The manifest dependency string is referencing a non-existent release
of the archived package; replace the current "givenergy-modbus>=2.0.0a6,<3.0.0"
entry with the active successor package name and a stable lower bound, e.g.
change it to "givenergy-modbus-async>=2.0.0,<3.0.0" in the manifest.json to
reference the maintained package (verify compatibility with any code that
imports givenergy-modbus and update imports/calls if necessary).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 056fc8b7-a54c-4fa4-8e13-72d28725fc1b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
custom_components/givenergy_local/__init__.pycustom_components/givenergy_local/manifest.jsonpyproject.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- custom_components/givenergy_local/init.py
Spurious readings from the inverter occasionally expand the apexcharts auto-scale to ±20 kW, compressing a full day of sane data against the chart floor. Adding a yaxis min/max envelope keeps rendering within the system's physical limits regardless of outliers. Defaults to 10 kW (single-phase systems). Exposed as an optional max_power_kw field on the generate_dashboard service so 3-phase users can raise it without regenerating with code changes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
statistics: { type: max, period: day } is unreliable for
total_increasing sensors — period: day may not be supported by all
apexcharts-card versions, and silently returns nothing.
Switch to statistics: { type: state, period: hour } + group_by: { func:
max, duration: 1d } which queries the hourly LTS (universally supported)
and aggregates into daily buckets. The daily max of hourly end-of-period
state values equals the end-of-day reading, which is the daily total for
sensors that reset at midnight.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…HA Repairs
Introduces DASHBOARD_VERSION (starting at 1) in dashboard.py. When the
integration loads and detects a previously-generated dashboard is behind
the current schema, it raises a fixable HA Repairs issue
(dashboard_outdated_v{N}) with old/new version placeholders. The issue
ID is version-specific so ignoring a v1→v2 prompt does not suppress the
v2→v3 one.
Clicking Fix in Settings → Repairs calls generate_dashboard
automatically, preserving the stored max_power_kw setting. Completing
the service (via Fix or the new Regenerate button on the Diagnostics tab)
saves the current schema version to HA's persistent Store and clears the
issue.
Also adds an Integration card at the bottom of the Diagnostics view with
a Regenerate Dashboard button and a commented-out stub for the upcoming
capture_frames service (issue #45).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Raises minimum givenergy-modbus from 2.0.0a6 to 2.0.0rc1 - Bumps requires-python to >=3.14 to match rc1's Python requirement - Renames four dual-stack battery sensor keys/value_fns from _2 to _alt to match the rc1 attribute rename in the modbus library Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Exposes givenergy-modbus rc1's Client.capture_frames() as a HA service. The service tees redacted Modbus wire frames to a text file in /local/ for a configurable duration (10–300 s, default 60 s) and posts a persistent notification with a download link when complete. - device_id is optional; omitting it targets the first connected inverter - Diagnostics dashboard tab now includes a "Capture Debug Frames" button Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…them The _alt energy register bank exists on some models only. Previously all four sensors were always created, showing as unknown/unavailable on hardware that doesn't have them. Now they are only registered if their value is non-None at first refresh (skip_if_none=True). Also corrects the section comment and friendly names — these registers are an alternate bank on the same battery, not a second battery stack. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@custom_components/givenergy_local/__init__.py`:
- Around line 214-217: The nested callback _sink closes over the per-iteration
list frames which triggers Ruff B023; fix this by binding frames as a default
parameter in the callback signature (e.g., def _sink(direction: str, data:
bytes, frames=frames)) so each loop iteration captures its own list explicitly
before calling coordinator._client.capture_frames; update the _sink definition
used with coordinator._client.capture_frames to use that default-argument
binding.
In `@custom_components/givenergy_local/strings.json`:
- Around line 37-40: The dashboard_outdated issue object currently has both a
top-level "description" and a "fix_flow" child which violates hassfest (must
have exactly one); remove the top-level "description" property from the
"dashboard_outdated" object in custom_components/givenergy_local/strings.json so
only "fix_flow" remains, and make the same removal for the corresponding
"dashboard_outdated" entry in
custom_components/givenergy_local/translations/en.json to keep translations
consistent.
In `@custom_components/givenergy_local/translations/en.json`:
- Around line 35-38: Remove the "description" property from the
"dashboard_outdated" issue object so only "fix_flow" remains (Home Assistant
requires exactly one of description or fix_flow for an issue); update both
custom_components/givenergy_local/translations/en.json ("dashboard_outdated"
object) and the corresponding strings.json entry to delete the description key
and keep the fix_flow block as-is.
In `@pyproject.toml`:
- Line 10: Update the Ruff and Mypy target settings to match the project's
Python requirement (>=3.14): change the Ruff configuration entry target-version
from "py313" to "py314" (under the tools.ruff section / target-version key) and
change the Mypy configuration python_version from "3.13" to "3.14" (under the
[tool.mypy] / python_version key) so linting/type-checking target the declared
Python runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02811654-e82b-4042-af10-0feec2fd0e1a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
custom_components/givenergy_local/__init__.pycustom_components/givenergy_local/const.pycustom_components/givenergy_local/dashboard.pycustom_components/givenergy_local/manifest.jsoncustom_components/givenergy_local/repairs.pycustom_components/givenergy_local/sensor.pycustom_components/givenergy_local/services.yamlcustom_components/givenergy_local/strings.jsoncustom_components/givenergy_local/translations/en.jsonpyproject.toml
✅ Files skipped from review due to trivial changes (1)
- custom_components/givenergy_local/manifest.json
🚧 Files skipped from review as they are similar to previous changes (2)
- custom_components/givenergy_local/const.py
- custom_components/givenergy_local/dashboard.py
- Remove top-level `description` from the `dashboard_outdated` issue in strings.json and translations/en.json; `description` and `fix_flow` are mutually exclusive in the hassfest issue schema (fixes CI failure) - Add `slot_fn` None guard in `time.py` `async_set_value` before writing to the inverter, so pause-slot entities on models without that slot are silently skipped rather than sending a broken command - Bind `frames` as an explicit default arg in the `_sink` closure inside `handle_capture_frames` to satisfy Ruff B023 - Update ruff `target-version` and mypy `python_version` from py313 → py314 to match the project's `requires-python = ">=3.14"` declaration Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Bump minimum HA version to 2026.5 (Python 3.14 requirement); update hacs.json homeassistant field to match - Remove Timeout Tolerance and Number of Batteries from config table (both removed in v1.0 — battery count is now auto-discovered) - Move Battery Pause Mode from diagnostic sensor to Controls table as a Select entity; add Battery Pause Slot Start/End time controls - Add Services section documenting generate_dashboard, capture_frames, reboot_inverter, and calibrate_battery_soc, including Repairs auto-fix behaviour for dashboard schema updates - Expand Supported inverters section to reflect givenergy-modbus v2.0 multi-model coverage, with confirmed vs. awaiting-validation split - Replace register-dump instructions with capture_frames as the primary debug tool; givenergy-cli noted as fallback for pre-install use - Add screenshot placeholders throughout; wire in existing dashboard-notification.png via new docs/ folder - Add troubleshooting note for auto-discovered battery count, linking to issue #48 for persistent capability caching Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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.
Summary
Promotes the v1.0 development line to stable. This release migrates to
givenergy-modbus2.x, ships a dashboard-generator service, simplifies the config surface, and lands substantial reliability work that came out of debugging #32. 33 commits, ~1.8k lines across 27 files.Library and reliability
The largest underlying change is the migration to
givenergy-modbus2.x. The library now does its own per-request retry with a 0.5sretry_delaydefault — calibrated against GivTCP's empirical value for the same hardware family — which catches the multi-second silent windows that some inverters (particularly Gen 1 AC) exhibit. With retries firing immediately as they did before, both attempts landed inside the silent window and accomplished nothing; the delay puts the retry past the typical recovery point.The integration picked up a matching set of changes. Time-slot edits now use single-endpoint writes — editing one end of a charge/discharge/pause slot writes just that register, rather than round-tripping a stale value for the other end. The coordinator's reset-after-N-timeouts semantics flipped to reset-on-the-Nth-timeout (so
timeout_tolerance=3does what its name suggests), and the default dropped from 5 to 3, roughly halving recovery time. INFO log lines on close/reconnect now appear under the integration's own logger so the cycle is visible from HA logs without enabling the library logger separately.Services
Three new services landed:
reboot_inverter— restarts the invertercalibrate_battery_soc— kicks off battery SOC recalibrationgenerate_dashboard— generates a Lovelace dashboard YAML tailored to the discovered topology, writes it to/config/www/, and notifies via a persistent notificationreboot_inverterandcalibrate_battery_socnow require an explicitdevice_idand raiseHomeAssistantErrorif the target inverter is unreachable. Previously they fell back to whichever inverter the integration found first, which got confusing in multi-inverter setups.Entities
Battery pause is now fully writable: a select for the mode (Disabled / Pause Charge / Pause Discharge / Pause Both) and a pair of time entities for the pause slot's start and end. Previously the mode was exposed as a read-only sensor, which surfaced state but didn't let users actually configure pause behaviour from HA.
Sensor coverage expanded with new diagnostic and enum sensors for inverter status, system mode, charge status, per-MPPT voltage and current, firmware versions, and various battery health indicators.
Configuration
The
retriesandtimeout_toleranceconfig-flow fields are removed. The library now ships calibrated defaults that handle the failure modes those knobs were intended to address, and in practice both invited "more is better" intuitions that traded recovery time for nothing useful. Anasync_migrate_entryruns automatically on first load, stripping the legacy fields from existing entries; everyone ends up on the calibrated defaults.The
max_batteriessetting is gone — battery count is auto-detected viadetect()at connect time.Build / CI infrastructure
pyproject.tomldeclaresuv_buildas the explicit build backend, avoiding setuptools' flat-layout auto-discovery getting confused by the coexistingcustom_components/anddashboard/directories. The auto-bump workflow now routes its PR to whichever branch matches the modbus major version (1.x → main, 2.x → v1.0), and runs under aBUMP_PATrepo secret so the resulting PR triggers downstreamvalidate.ymlchecks properly.scripts/apply-branch-protection.shcaptures the baseline branch-protection ruleset as a re-runnable shell script.Breaking changes
max_batteriessetting removed — battery count is auto-detectedretriesandtimeout_toleranceconfig-flow fields removed; migration runs on first loadbattery_pause_modemoves sensor → select platform —sensor.givenergy_battery_pause_modeis orphaned; dashboards or automations should be repointed atselect.givenergy_battery_pause_modereboot_inverterandcalibrate_battery_socservices now requiredevice_idTest plan
Summary by CodeRabbit
New Features
Chores
Misc