Add Venus A (VNSA) PV input support and fix BMS scaling#309
Merged
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds Venus A (VNSA) device variant parsing with per-string PV input sensors, aggregated total PV power, and conditional BMS temperature scaling. Extends transforms with venusPvField and scaled sum, updates device registration wiring, adds types and tests, and updates CHANGELOG. ChangesVenus A (VNSA) variant with PV input and temperature scaling support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Venus A (VNSA) reports per-string PV input power (pv1-pv4 in the form power|connected) that was never parsed, so no PV sensors appeared in MQTT/Home Assistant. Add PV1-PV4 power sensors, per-input connection binary sensors and a combined Total PV Power sensor for VNSA. The BMS battery voltage and charge voltage were published as raw centivolt/decivolt values (e.g. 4328 instead of 43.28 V); scale them to volts for all Venus variants. Venus A additionally encodes the cell and MOSFET temperatures 10x too high (164 instead of 16.4 C); scale those down for VNSA only, since the other Venus variants already report whole degrees. Fixes #218 https://claude.ai/code/session_01L5QLHLxrBV8zqDip55aKow
Replace the function-based field transforms added for the Venus A PV
inputs with declarative transforms, which are introspectable and
serializable to Jinja2 (function transforms are legacy).
Add a declarative venusPvField('power' | 'connected') transform that
extracts a field from the Venus PV input string ("<power>|<connected>"),
mirroring the existing mpptPvField transform, and use sum() for the
combined Total PV Power sensor.
https://claude.ai/code/session_01L5QLHLxrBV8zqDip55aKow
Confirmed against the decompiled Marstek app (v1.6.64,
VenusPVModel in venus_realTime_controller.dart): each PV input value
"<power>|<connected>" stores parts[0] * 0.1 as pvNInverterValue, i.e.
the power is reported in deciwatts. So pv1=1076 is 107.6 W, not 1076 W.
- venusPvField('power') now divides the leading value by 10
- add an optional scale divisor to the sum() transform and use sum(10)
for the Total PV Power sensor
https://claude.ai/code/session_01L5QLHLxrBV8zqDip55aKow
tomquist
force-pushed
the
claude/adoring-dirac-cEVOb
branch
from
25de1a9 to
81bf944
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/parser.test.ts (1)
695-710: 💤 Low valueOptional: assert
b_tem(battery temperature) stays unscaled for VNSA.
b_temis the one temperature field withoutscaleTemperatures, so it diverges fromb_tp*/b_mot. Adding an explicit assertion (e.g.b_tem=16→temperature16, not 1.6) would lock in that intentional distinction and guard against an accidental future blanket-scaling change.🤖 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/parser.test.ts` around lines 695 - 710, Add an assertion to lock in that the raw b_tem field remains unscaled: after parsing with parseMessage in the 'scales Venus A (VNSA) BMS...' test, assert that the battery temperature coming from b_tem equals 16 (e.g. expect(result.bms?.temperature).toBe(16)), ensuring the b_tem value is not divided by any temperature scaling applied to b_tp*/b_mot.src/device/venus.ts (1)
234-323: 💤 Low valueConsider extracting PV field registration into a loop.
The per-string PV power and connectivity registrations repeat the same pattern for pv1–pv4. A loop similar to the cell voltage registration (lines 1418–1431) would reduce duplication and make future additions easier.
♻️ Optional refactor using a loop
// PV / solar input information if (withPvInputs) { - // Per-string PV input power (pv1..pv4 = "<power>|<connected>") - field({ key: 'pv1', path: ['pv1Power'], transform: venusPvField('power') }); - advertise( - ['pv1Power'], - sensorComponent<number>({ - id: 'pv1_power', - name: 'PV1 Power', - device_class: 'power', - unit_of_measurement: 'W', - state_class: 'measurement', - }), - ); - field({ key: 'pv2', path: ['pv2Power'], transform: venusPvField('power') }); - // ... similar for pv2, pv3, pv4 power and connected ... + // Per-string PV input power and connection status (pv1..pv4 = "<power>|<connected>") + for (let i = 1; i <= 4; i++) { + const key = `pv${i}`; + field({ key, path: [`pv${i}Power`], transform: venusPvField('power') }); + advertise( + [`pv${i}Power`], + sensorComponent<number>({ + id: `pv${i}_power`, + name: `PV${i} Power`, + device_class: 'power', + unit_of_measurement: 'W', + state_class: 'measurement', + }), + ); + field({ key, path: [`pv${i}Connected`], transform: venusPvField('connected') }); + advertise( + [`pv${i}Connected`], + binarySensorComponent({ + id: `pv${i}_connected`, + name: `PV${i} Connected`, + device_class: 'connectivity', + icon: 'mdi:solar-power', + enabled_by_default: false, + }), + ); + }🤖 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/device/venus.ts` around lines 234 - 323, The PV power and connectivity registrations are duplicated for pv1–pv4; refactor by iterating over an array of PV identifiers (e.g., ['pv1','pv2','pv3','pv4']) and perform the repeated calls to field(..., transform: venusPvField('power'/'connected')) and advertise(...) inside that loop, using sensorComponent for power (id: `${pv}_power`) and binarySensorComponent for connectivity (id: `${pv}_connected`); update references to the advertised state keys (e.g., `${pv}Power`, `${pv}Connected`) so the existing field and advertise calls (function names: field, advertise, venusPvField, sensorComponent, binarySensorComponent) are created programmatically instead of repeated.
🤖 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 `@src/device/venus.ts`:
- Around line 234-323: The PV power and connectivity registrations are
duplicated for pv1–pv4; refactor by iterating over an array of PV identifiers
(e.g., ['pv1','pv2','pv3','pv4']) and perform the repeated calls to field(...,
transform: venusPvField('power'/'connected')) and advertise(...) inside that
loop, using sensorComponent for power (id: `${pv}_power`) and
binarySensorComponent for connectivity (id: `${pv}_connected`); update
references to the advertised state keys (e.g., `${pv}Power`, `${pv}Connected`)
so the existing field and advertise calls (function names: field, advertise,
venusPvField, sensorComponent, binarySensorComponent) are created
programmatically instead of repeated.
In `@src/parser.test.ts`:
- Around line 695-710: Add an assertion to lock in that the raw b_tem field
remains unscaled: after parsing with parseMessage in the 'scales Venus A (VNSA)
BMS...' test, assert that the battery temperature coming from b_tem equals 16
(e.g. expect(result.bms?.temperature).toBe(16)), ensuring the b_tem value is not
divided by any temperature scaling applied to b_tp*/b_mot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbc7c0f8-a13d-4042-a838-4ee94e5b4054
📒 Files selected for processing (6)
CHANGELOG.mdsrc/device/venus.tssrc/parser.test.tssrc/transforms.test.tssrc/transforms.tssrc/types.ts
- Refactor the repeated pv1-pv4 power/connection field+advertise calls into a loop over a typed list of PV inputs (behaviour unchanged: same ids, names, paths and transforms). - Add an assertion to the Venus A BMS test that the battery temperature (b_tem) stays unscaled at 16, guarding against the cell/MOSFET temperature scaling being applied to it. https://claude.ai/code/session_01L5QLHLxrBV8zqDip55aKow
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 per-string PV input monitoring on Venus A (VNSA) devices and fixes incorrect scaling of BMS voltage and temperature values across all Venus variants.
Key Changes
Venus A PV Input Support: Added exposure of per-string PV input power (PV1–PV4) and their connection status, plus a combined Total PV Power sensor for VNSA devices
venusPvFieldtransform to parse Venus PV input format ("POWER|CONNECTED")sumtransform with optional scale parameter to calculate total PV power across all inputsBMS Voltage Scaling: Fixed battery voltage and charge voltage to be reported in volts instead of raw centivolt/decivolt values
Venus A BMS Temperature Scaling: Fixed cell and MOSFET temperatures for VNSA to be scaled by factor of 10 (they were previously 10× too high)
Device Definition Refactoring: Separated VNSA device registration from other Venus variants (HMG, VNSE3, VNSD) to support variant-specific field configurations
Implementation Details
registerRuntimeInfoMessage()andregisterBMSInfoMessage()to enable variant-specific field registrationSumTransforminterface to support optional scale divisorexecuteVenusPvField()andgenerateVenusPvFieldJinja2()for transform execution and template generationhttps://claude.ai/code/session_01L5QLHLxrBV8zqDip55aKow
Summary by CodeRabbit
New Features
Bug Fixes
Documentation