Add Depth of Discharge configuration support for Venus devices#307
Merged
Conversation
Venus devices report a depth-of-discharge (dod) value in their runtime info but it was neither published nor configurable. Add a Depth of Discharge number entity and a discharge-depth command (cd=56,dod=%d), matching the Jupiter implementation. The entity is only advertised on devices that actually report the value and is limited to the 30-88% range exposed by the Marstek app. Fixes #306
Sniffing the app traffic (issue #306) confirmed the command is cd=56,dod=%d with a minimum of 30, but the maximum of 88% is sent to the device as dod=0 rather than dod=88. Translate the sentinel in both directions: send 0 when the requested value is the maximum, and report a received dod=0 back as 88%.
Return undefined (defer) instead of false from the enabled predicate when the dod field is absent, so the entity is only published once the device actually reports a depth of discharge value, matching the HMI inverter PV3/PV4 gating convention. Returning false would have actively cleared the entity instead.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR adds Venus depth-of-discharge control by introducing a ChangesVenus Depth-of-Discharge Support
Sequence DiagramsequenceDiagram
participant Device as Battery Device
participant Parser as MQTT Parser
participant VenusImpl as Venus Implementation
participant HA as Home Assistant
participant User as Command Source
Device->>Parser: publishes "dod=0" or "dod=88"
Parser->>VenusImpl: parsed message (dod -> depthOfDischarge)
VenusImpl->>HA: publish dod sensor (depthOfDischarge%)
User->>VenusImpl: discharge-depth command (e.g. 75)
VenusImpl->>VenusImpl: validate 30≤value≤88
VenusImpl->>VenusImpl: update depthOfDischarge
VenusImpl->>Device: publish "cd=56,dod=75" (or "dod=0" if 88)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (1)
src/generateDiscoveryConfigs.test.ts (1)
357-363: ⚡ Quick winAssert the advertised DoD range contract too.
This verifies gating, but not the
30..88/step: 1number metadata that Home Assistant uses to constrain the control. A drift there would let the UI offer values that the Venus handler rejects.Small assertion add-on
const withDod = dodConfigs({ depthOfDischarge: 88 }); expect(withDod).toHaveLength(1); expect(withDod[0].config).not.toBeNull(); + expect(withDod[0].config).toMatchObject({ + min: 30, + max: 88, + step: 1, + }); });🤖 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 `@src/generateDiscoveryConfigs.test.ts` around lines 357 - 363, Add an assertion that the advertised DoD range contract carries the expected numeric bounds and step: after creating withDod = dodConfigs({ depthOfDischarge: 88 }), inspect withDod[0].config (the advertised DoD control/contract) and assert its metadata contains minimum 30, maximum 88 and step 1 (e.g., contract.range or properties.range fields that describe the 30..88 step:1 constraint) so the test verifies both presence and the exact numeric constraints used by Home Assistant.
🤖 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 `@docs/venus.md`:
- Around line 423-430: Add language identifiers to the two fenced code blocks
containing the MQTT topic "hame_energy/{type}/App/{uid or mac}/ctrl" and the
payload "cd=56,dod=%d" in docs/venus.md to satisfy markdownlint MD040; update
both opening backtick fences to include a language tag (e.g., "text") so they
become fenced code blocks like ```text.
In `@src/device/venus.ts`:
- Around line 1226-1235: The transform for the 'dod' field currently maps the
sentinel 0 to DOD_MAX but otherwise accepts any integer; update the transform
for the field with key 'dod' (path ['depthOfDischarge']) to first parse the
value and map 0 to DOD_MAX, then validate that the resulting dod lies within
DOD_MIN..DOD_MAX and return undefined for parse failures or out-of-range values;
in short: decode sentinel 0, then drop (return undefined) any dod < DOD_MIN or >
DOD_MAX.
In `@src/deviceCommands.test.ts`:
- Around line 744-787: Add test cases to src/deviceCommands.test.ts for
malformed discharge-depth payloads (e.g., "88foo", "30.5", "", " 70 ") and
update the Venus handler in src/device/venus.ts to reject any non-exact-numeric
payload rather than using parseInt(message, 10). In the Venus handler function
that processes the "discharge-depth" command, validate the message with a strict
numeric check (e.g., match full string to an integer pattern), then enforce the
30–88 range and still map 88 to 0 before publishing; return null/ignore when
validation fails so the new tests pass.
---
Nitpick comments:
In `@src/generateDiscoveryConfigs.test.ts`:
- Around line 357-363: Add an assertion that the advertised DoD range contract
carries the expected numeric bounds and step: after creating withDod =
dodConfigs({ depthOfDischarge: 88 }), inspect withDod[0].config (the advertised
DoD control/contract) and assert its metadata contains minimum 30, maximum 88
and step 1 (e.g., contract.range or properties.range fields that describe the
30..88 step:1 constraint) so the test verifies both presence and the exact
numeric constraints used by Home Assistant.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbce137b-0638-4417-82bc-708cf010a74f
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mddocs/venus.mdsrc/device/venus.tssrc/deviceCommands.test.tssrc/generateDiscoveryConfigs.test.tssrc/parser.test.tssrc/types.ts
Comment on lines
+423
to
+430
| ``` | ||
| hame_energy/{type}/App/{uid or mac}/ctrl | ||
| ``` | ||
|
|
||
| Payload: | ||
| ``` | ||
| cd=56,dod=%d | ||
| ``` |
There was a problem hiding this comment.
Add fenced code block language identifiers to satisfy markdownlint (MD040).
Line 423 and Line 428 use fenced blocks without language tags; this triggers current static-analysis warnings.
Suggested doc-only fix
Topic:
-```
+```text
hame_energy/{type}/App/{uid or mac}/ctrlPayload:
- +text
cd=56,dod=%d
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 423-423: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 428-428: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/venus.md` around lines 423 - 430, Add language identifiers to the two
fenced code blocks containing the MQTT topic "hame_energy/{type}/App/{uid or
mac}/ctrl" and the payload "cd=56,dod=%d" in docs/venus.md to satisfy
markdownlint MD040; update both opening backtick fences to include a language
tag (e.g., "text") so they become fenced code blocks like ```text.
- Reject out-of-range values when decoding the dod field (after mapping the 0 sentinel to the maximum), returning undefined instead of publishing bogus percentages. - Require an exact integer payload for the discharge-depth command so malformed payloads (e.g. "88foo", "30.5", "", " 70 ") are ignored rather than silently coerced by parseInt. - Extend tests to cover the malformed payloads and assert the advertised number entity carries the 30..88 step:1 range.
Replace the function-based dod transform with the declarative chain(number(), inRange(...)), matching the existing mcp_w field. The device reports the actual percentage in its runtime data, so the read path only needs numeric parsing and range validation; the 0 sentinel for the maximum applies only to the write direction, which stays in the discharge-depth command handler. Declarative transforms are also introspectable for Home Assistant Jinja2 template generation.
Extract the function form of TransformSpec into a named TransformFn type annotated with @deprecated, so the compiler and IDEs flag any function-based field transforms. Declarative transforms should be used instead since they can be introspected for Home Assistant Jinja2 template generation and serialized.
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
2 participants
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
This PR adds support for reading and configuring the Depth of Discharge (DoD) setting on Venus devices through a new
discharge-depthcommand entity. The feature allows users to adjust the battery's maximum depth of discharge between 30-88% via Home Assistant.Key Changes
DEPTH_OF_DISCHARGE = 56command type to the Venus device command enumDOD_MIN = 30andDOD_MAX = 88based on Marstek app limits with a note that these may change over timedepthOfDischargefield toVenusDeviceDatatypedischarge-depthcommand handler with input validationImplementation Details
depthOfDischargeis present in device state, preventing unnecessary Home Assistant entities for devices that don't support this featurehttps://claude.ai/code/session_01TWXXbfjrDmatHtpZKUfktK
Summary by CodeRabbit
New Features
Documentation
Tests