Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughIntegration updated for givenergy-modbus 2.x: removed max_batteries setting, introduced coordinator-level InverterModel type, added two device services (reboot_inverter, calibrate_battery_soc) that resolve a coordinator by Home Assistant device_id, refactored time-slot commands, and improved detection/timeout behavior. ChangesCore integration and services
sequenceDiagram
participant HA as "Home Assistant" color rgba(100,149,237,0.5)
participant DeviceReg as "Device Registry" color rgba(60,179,113,0.5)
participant Coord as "GivEnergy Coordinator" color rgba(255,165,0,0.5)
participant Modbus as "givenergy-modbus Client" color rgba(199,21,133,0.5)
participant Inverter as "Inverter" color rgba(70,130,180,0.5)
HA->>DeviceReg: service call with device_id
DeviceReg-->>HA: return device -> config_entry_id
HA->>Coord: lookup coordinator by config_entry_id
Coord->>Modbus: ensure connected
Modbus-->>Coord: connected
Coord->>Modbus: run detect()
Modbus-->>Coord: topology/capabilities
Coord->>Modbus: one_shot_command(reboot/calibrate) -> inverter
Modbus->>Inverter: execute command
Inverter-->>Modbus: ack/result
Modbus-->>Coord: return result
Coord-->>HA: return or raise HomeAssistantError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![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 updates the integration to support givenergy-modbus v2.0.0, replacing the manual max_batteries configuration with automatic topology detection via client.detect(). It also introduces new services for rebooting the inverter and calibrating battery SOC, and refines time slot management. Feedback highlights a critical issue where the _reset_client method is called but not defined in the coordinator, and suggests improving encapsulation and error handling by avoiding direct access to the private _client attribute in service handlers and entities.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pyproject.toml (1)
26-27: ⚡ Quick winPin the git source to a commit (or tag) for reproducible and explicit dependency resolution.
While
uv.lockcurrently pins the commit, the source inpyproject.tomlshould explicitly reference a commit SHA or stable tag. Relying onbranch = "main"means dependency resolution can drift day-to-day when the lock file is updated, and the manifest itself doesn't reflect the actual pinned version being used.Suggested change
[tool.uv.sources] -givenergy-modbus = { git = "https://github.com/dewet22/givenergy-modbus", branch = "main" } +givenergy-modbus = { git = "https://github.com/dewet22/givenergy-modbus", rev = "04ef415d2bb3f019aa436c86324803c0f7109713" }🤖 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 `@pyproject.toml` around lines 26 - 27, The dependency entry under [tool.uv.sources] for givenergy-modbus should be pinned to a specific commit SHA or tag instead of branch = "main"; update the givenergy-modbus source to reference a stable rev (e.g., rev = "<COMMIT_SHA>" or tag = "<vX.Y.Z>") in pyproject.toml so the manifest is explicit and reproducible, then regenerate/update uv.lock to capture the new pinned version; locate the entry named givenergy-modbus in the [tool.uv.sources] section to make this change.tests/test_coordinator.py (1)
12-15: ⚡ Quick winAdd assertion for
detect()call to regression-protect topology discovery on first connect.The test currently verifies
connect()andrefresh_plant()but omitsdetect(), which is called in the coordinator's_connect()method (line 168 of coordinator.py). Sincedetect()populatesplant.capabilitiesand gates model-aware refresh behavior required for multi-phase and other non-standard topologies, this assertion closes a regression-protection gap.Suggested test addition
async def test_first_refresh_connects_and_fetches(hass, mock_client, setup_integration): mock_client.connect.assert_called_once() + mock_client.detect.assert_called_once() mock_client.refresh_plant.assert_called_once_with(full_refresh=True)🤖 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_coordinator.py` around lines 12 - 15, The test test_first_refresh_connects_and_fetches currently asserts connect() and refresh_plant() but misses asserting that detect() was invoked during the coordinator's _connect() flow; update the test to include an assertion like mock_client.detect.assert_called_once() (or mock_client.detect.assert_called_once_with() if you expect specific args) to ensure topology discovery is exercised—look for the mock_client usage in the test and add the detect() assertion alongside the existing connect and refresh_plant assertions.
🤖 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 59-67: Both service handlers (handle_reboot_inverter and
handle_calibrate_battery_soc) currently silently no-op when
_coordinator_for_device returns None or the client's disconnected; change them
to fail fast by checking the result of _coordinator_for_device and
c._client.connected and raising a HomeAssistantError (from
homeassistant.exceptions.HomeAssistantError) when the device_id is invalid,
coordinator is missing, or client not connected, including the device_id in the
error message so the caller sees the failure instead of a silent success; keep
the existing await c._client.one_shot_command(...) path when checks pass.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 26-27: The dependency entry under [tool.uv.sources] for
givenergy-modbus should be pinned to a specific commit SHA or tag instead of
branch = "main"; update the givenergy-modbus source to reference a stable rev
(e.g., rev = "<COMMIT_SHA>" or tag = "<vX.Y.Z>") in pyproject.toml so the
manifest is explicit and reproducible, then regenerate/update uv.lock to capture
the new pinned version; locate the entry named givenergy-modbus in the
[tool.uv.sources] section to make this change.
In `@tests/test_coordinator.py`:
- Around line 12-15: The test test_first_refresh_connects_and_fetches currently
asserts connect() and refresh_plant() but misses asserting that detect() was
invoked during the coordinator's _connect() flow; update the test to include an
assertion like mock_client.detect.assert_called_once() (or
mock_client.detect.assert_called_once_with() if you expect specific args) to
ensure topology discovery is exercised—look for the mock_client usage in the
test and add the detect() assertion alongside the existing connect and
refresh_plant assertions.
🪄 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: a0f2cda3-0f20-453b-b4be-791c2b58b155
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
custom_components/givenergy_local/__init__.pycustom_components/givenergy_local/config_flow.pycustom_components/givenergy_local/const.pycustom_components/givenergy_local/coordinator.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.jsonpyproject.tomltests/conftest.pytests/test_config_flow.pytests/test_coordinator.py
💤 Files with no reviewable changes (2)
- custom_components/givenergy_local/strings.json
- custom_components/givenergy_local/translations/en.json
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.
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
Batches up the work that's been accumulating on `next` since the last merge to `main`. All seven commits land together because they sit on a single rebased line and several of them depend on each other (the library bump, the alias rename and the slot-command refactor in particular).
Test plan
Summary by CodeRabbit
New Features
Chores
Localization