The three-phase Sofar, with its new half unverified and labelled as such - #189
Merged
Conversation
sofar_hyd_3ph, sibling of the EP profile shipped yesterday. Same vendor document, same address space, two more phases. THE SOURCING IS DELIBERATELY UNEVEN AND THE PROFILE SAYS SO TWICE. Everything the two profiles share -- frequency, output power, phase R, both PV strings, the whole battery block, inverter temperature -- carries the EP's two-source backing. The three-phase part does not: the hardware behind the second source is a single-phase HYD-3600-EP, which has no S or T phase to confirm. So the registers that make this profile three-phase are exactly the registers nothing has checked. Writing that down matters more than usual here, because the shared rows would otherwise lend their credibility to the new ones -- a profile is read as one document, and sixteen confirmed rows make the four unconfirmed ones look confirmed by proximity. Included anyway rather than withheld: for a three-phase owner an unsourced channel and an absent one are both wrong, and only one of them is testable. ONE TRAP RECORDED FOR WHOEVER EXTENDS THIS. R, S and T are NOT evenly spaced -- 0x048D, 0x0498, 0x04A3, eleven registers apart with other grid values between them. Assuming a stride reads the wrong register and returns a number that looks fine. The block is sized to span all three in one read rather than three reads of four. Two strings mapped, not four. The vendor list carries PV1-PV6 and gives mask bits only to PV1-PV4, which suggests four is this family's ceiling -- but it never says which MODELS populate three or four, and the range spans 5 kW to 20 kW. A unit with more strings under-reports, which is visible and fixable; mapping registers a smaller unit does not populate would publish zeroes as readings, which is not. 10 profiles valid, 1022 host tests, all gates, board builds clean.
Review again found a summary sentence flattening an exception its own detail records. The README row said "what it shares is two-sourced", but the two energy counters it shares with the EP profile are vendor-document only -- stated correctly in both profile headers and in the protocol doc, and lost in the one-line summary a reader actually scans. That is the third instance in two days: the SolarMax note, the EP profile, and now this. Same shape every time -- the exception is documented where the work is, and the sentence at the top promises more than the rows beneath it. It is not carelessness about the facts; it is carelessness about which sentence gets read. Also fixed: the header said "three registers" make this profile three-phase where there are four (voltage and current on both S and T), and the docs index still described the protocol page as covering only the single-phase EP after this branch extended it to cover both.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new Modbus profile for the Sofar HYD 5…20K-3PH family, extending the existing Sofar HYD EP map to three-phase (L2/L3) while explicitly documenting which rows are vendor-only/unverified. This fits the “profile-driven Modbus support” approach by expanding device coverage without changing firmware code.
Changes:
- Introduces
sofar_hyd_3phprofile with 3-phase AC measurements plus shared EP measurements (PV, battery, temp, energy). - Updates Sofar protocol documentation and top-level docs/README to reflect the new three-phase profile and its sourcing caveats.
- Updates coverage tables to include the new profile.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds the new Sofar HYD 5…20K-3PH entry to the supported inverters table with sourcing notes. |
| profiles/sofar/hyd_3ph.toml | New three-phase Sofar HYD profile (blocks + measurement mappings) with explicit sourcing warnings. |
| docs/sofar-hyd-protocol.md | Documents that the three-phase sibling now has its own profile and calls out the uneven phase spacing trap. |
| docs/README.md | Updates the Sofar entry to cover both EP and 3PH profiles and summarizes sourcing differences. |
| docs/drivers/coverage.md | Adds sofar_hyd_3ph to generated coverage tables. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The header talked about phases S and T while every row below maps ac.phase_l2 and ac.phase_l3, and nothing in the file said those were the same thing. A reader holding this profile next to the vendor document had to carry that mapping in their head -- which is how somebody eventually maps T onto L2. The naming is now stated once at the top, and every phase row names the vendor's letter beside our id. The long line left over from the previous fix is rewrapped; nothing in the file exceeds 100 characters now.
Review re-derived this profile from the vendor PDF and found the reason for its biggest omission was false. 0x0488 ActivePower_PCC_Total was left unmapped because "the vendor gives no sign convention". It does. THE REMARK CELL WRAPS ONTO A SECOND LINE, and the text extraction I worked from showed only the first half -- "Totall [sic] PCC active power,". The continuation reads "positive to fed [sic] into the grid, negative to draw from the grid". I had seen that sentence on the neighbouring apparent-power row and concluded it belonged only there. Re-reading the page with the table layout preserved shows it belongs to both. So the grid figure is now mapped, in BOTH Sofar profiles, with scale = -10: 0.01 kW to watts, and the sign flip, because the vendor counts export positive where grid.power counts import positive. That fills the channel added three days ago, which until now had no producer at all. The same re-read turned up 0x0504 ActivePower_Load_Total, which the vendor states plainly -- "Consumed by load is positive" -- already matching load.power. Mapped too. Twenty registers in the EP profile now, twenty-four in the three-phase one. WHAT I AM KEEPING IN THE DOCS is not the corrected fact but the mistake. The failure was not misreading a number; it was treating a truncated extraction as if it were the document, and then reasoning carefully on top of it. Every argument I built was sound -- don't infer a convention from a neighbouring register, don't guess a grid sign, a backwards grid figure is the worst kind of wrong -- and all of it rested on a sentence I had not read. Careful reasoning on an unverified input produces confident, wrong output, and it looks exactly like diligence. The class generalises past this file: any wrapped cell in any vendor PDF extracts the same way, and nothing warns you.
Second review pass, on the state the first one produced. It confirmed the sign arithmetic (raw 250 exporting -> -2500 W, raw -250 importing -> +2500 W) and the block coverage, and then found five claims left stale by my own change. The worst is docs/README.md, which still said "the grid register is left out of both because nobody states its sign" -- describing a decision I had reversed two commits earlier, in the very index line meant to tell a reader what the page says. The rest are counts and lists that stopped matching when two registers were added: the EP header still said sixteen-of-eighteen with two exceptions where it is now sixteen-of-twenty with four, its block comment still said eighteen values, the three-phase header still counted six vendor-only rows where there are eight, and the README row for the three-phase profile listed the exceptions the EP row above it had already been corrected to list in full. The pattern is now unmistakable and worth stating plainly: EVERY TIME I CHANGE WHAT A FILE DOES, THE SENTENCES SUMMARISING IT GO STALE, AND I DO NOT GO LOOKING. The detail is right because I was editing it; the summary is wrong because I was not. Counting the registers with a script rather than trusting the prose is what caught four of these five.
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
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.
sofar_hyd_3ph, sibling of the EP profile shipped yesterday. Same vendor document, same addressspace, two more phases.
The sourcing is deliberately uneven, and the profile says so twice
Everything the two profiles share — frequency, output power, phase R, both PV strings, the
whole battery block, inverter temperature — carries the EP's two-source backing.
The three-phase part does not. The hardware behind the second source is a single-phase
HYD-3600-EP, which has no S or T phase to confirm. So the registers that make this profile
three-phase are exactly the registers nothing has checked.
Writing that down matters more than usual here: a profile is read as one document, and sixteen
confirmed rows make the four unconfirmed ones look confirmed by proximity. Included anyway rather
than withheld — for a three-phase owner an unsourced channel and an absent one are both wrong, and
only one of them is testable.
One trap recorded for whoever extends this
R, S and T are not evenly spaced:
0x048D,0x0498,0x04A3— eleven registers apart, withother grid values in between. Assuming a stride reads the wrong register and returns a number that
looks entirely reasonable. The block is sized to span all three in one read rather than three reads
of four.
Two strings mapped, not four
The vendor list carries PV1–PV6 and gives mask bits only to PV1–PV4, which suggests four is this
family's ceiling — but it never says which models populate three or four, and the range spans
5 kW to 20 kW.
A unit with more strings under-reports, which is visible and fixable. Mapping registers a smaller
unit does not populate would publish zeroes as readings, which is not.
Verification
10 profiles valid, 1022 host tests, all gates, board builds clean. Block coverage was checked
independently of the generator: every register, including both words of each
u32, falls inside adeclared block.