You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
fix!: correct meter decoding, nameplate reads and TCP reconnection
Collects the work since v1.3.6: four wire-format and lifecycle faults
found against a Primo 4.0-1 and a Smart Meter TS 65A-3 sharing one RS485
bus, one breaking callback change, and the documentation and header
hygiene needed for release.
Meter, proprietary register map
-------------------------------
REG::SN was declared with NB=1 while carrying a UINT32 payload, so the
read fetched one register and the decode consumed two; the low word was
never transferred. It was also the only 32-bit read in the map not
passing word_swap, though the Carlo Gavazzi EM/ET300 protocol the map
derives from stores every 32-bit value least-significant word first. A
meter reporting [0x08EC, 0x05B1] decoded as 149684224 against a barcode
reading 95488236.
The firmware version register is not a numeric major component. The same
protocol defines it as a letter index -- 0 is "A" -- paired with a
decimal revision, so a meter reporting [1, 5] runs B5, not 1.5. Report
the raw numeric pair when the code exceeds 'Z', since a device that far
outside the documented encoding is better described by its register
contents than by a wrong letter. VR_MAJOR and VR_MINOR become FWVER and
FWREV to match: the old names asserted the numeric versioning this
removes, and the underscore form reads as the scale-factor suffix used
throughout the SunSpec models.
REG::A claimed register 270 was the total AC current. It is the phase
sequence: 0 for L1-L2-L3, 1 for L1-L3-L2. Nothing decoded it, so no
reported value was ever wrong -- getAcCurrent() already refused
Phase::TOTAL on this map -- but the declaration read like a limitation
to lift rather than the absence of a register, and lifting it would have
reported a rotation code as amperes. The map has no system current at
all; say so in the namespace documentation and expose 270 as REG::PHSEQ.
Inverter nameplate block
------------------------
validateNameplateRegisters() fetched only the two-register block header
at 40121/40122, leaving the payload from DERTYP onwards untouched, and
nothing else covers that range. Every getAcPowerRating() call therefore
indexed a region of regs_ that no transaction had written; because regs_
is zero-filled, all four outputs returned a clean 0 rather than failing.
Read header and payload in one transaction, as I160 already does. On a
Primo 4.0-1 the block holds WRtg=400 sf=1 and VARtg=400 sf=1, i.e. 4000
W and 4000 VA, where the library previously reported 0.
Bus reconnection and callbacks
-------------------------------
Pulling the inverter's ethernet cable left the bridge stuck until it was
restarted. The kernel holds a TCP connection open across a link-down, so
no ECONNRESET or EPIPE arrives; the only signal is ETIMEDOUT, classified
TRANSIENT, which never clears connected_ and so never re-enters the only
phase that calls tryConnect(). The timed-out request also stayed
outstanding, so the late reply answered the next request and every read
after it was one frame behind. Escalate ETIMEDOUT to RECONNECT in
executeTransaction() when the transport is TCP, and only there:
tryConnect() sees the same code from an unanswered SYN, where it is a
genuine transient that Phase 1 already retries.
onBusConnect_ fired when the state became connected while
onBusDisconnect_ fired when an attempt to restore it failed, so the names
implied a pair the fire sites did not form. An outage recovering on its
first attempt left no trace beyond an errno, and the bus offered no
analogue of setDeviceRetryCallback() despite having exactly that
behaviour. Split the two meanings: onBusDisconnect_ becomes void() and
fires once from the Phase 2 exit, where connected_ actually changed;
onBusRetry_ takes over the old signature and fire site. The delay stays
off the disconnect callback deliberately -- naming one at drop time is a
prediction, since the attempt has not run and may never wait at all.
Headers and comments
---------------------
modbus_utils.h used std::reverse without <algorithm> and Register
without register_base.h, compiling only because every consumer happened
to include both first; on its own, or through registers.h, it failed
outright. Every public header now compiles standalone.
Comments that grew during the shared-bus and reconnect debugging into
records of how a conclusion was reached are shortened to the operative
rule and its reason, and those restating the statement below them are
dropped. Where a rule is stated in full on the public API the
implementation now refers to it rather than repeating it. Also drop the
"[--] guard done, queue free" trace, which has announced the completion
of a guard that no longer runs at that point since 634d926, and correct
two stale declarations: getFwVersion()'s documented return format, and
cbMutex_ guarding four callback vectors rather than three.
BREAKING CHANGE: addBusDisconnectCallback() takes std::function<void()>
instead of std::function<void(int)> and fires on the transition rather
than per failed attempt. Consumers wanting the old per-attempt delay must
register addBusRetryCallback() instead. REG::VR_MAJOR and REG::VR_MINOR
are renamed to REG::FWVER and REG::FWREV, and REG::A is removed in favour
of REG::PHSEQ. Consumers persisting the meter serial number or firmware
version must migrate stored values: both differ from every previously
reported one.
fix: read the meter serial number as a full 32-bit value
REG::SN was declared with NB=1 while carrying a UINT32 payload, so the
read fetched one register and the decode consumed two. The low word was
never transferred and the zero-initialised buffer was used instead.
The word order was wrong as well. Every 32-bit value in this map is
stored least-significant word first -- the Carlo Gavazzi EM/ET300
protocol the map derives from states this explicitly -- but the serial
was the only 32-bit read not passing word_swap.
On a Smart Meter TS 65A-3 reporting registers [0x08EC, 0x05B1] the old
code produced 149684224; the meter's barcode reads 95488236.
Consumers persisting the serial number need to migrate stored values:
the corrected result differs from every previously reported one.
fix: format the meter firmware version as a Carlo Gavazzi code
The version register is not a numeric major component. The Carlo Gavazzi
EM/ET300 protocol this register map derives from defines it as a letter
index -- 0 is "A", 1 is "B" -- paired with a decimal revision code, so a
meter reporting [1, 5] runs firmware B5, not 1.5.
Report the raw numeric pair when the version code exceeds 'Z', since a
device that far outside the documented encoding is better described by
its actual register contents than by a wrong letter.
Rename VR_MAJOR and VR_MINOR to FWVER and FWREV to match: the old names
asserted the numeric versioning this commit removes, and the underscore
form reads as the scale-factor suffix used throughout the SunSpec models
(A_SF, W_SF).
Consumers persisting the firmware version will see the string change on
the next device read.
fix: reconnect a TCP bus when a read on an established socket times out
Pulling the inverter's ethernet cable left the bridge stuck until it was
restarted. The kernel holds a TCP connection open across a link-down, so
no ECONNRESET or EPIPE ever arrives; the only signal is ETIMEDOUT, which
deduceSeverity() classifies as TRANSIENT. TRANSIENT does not clear
connected_, so busLoop() stayed in Phase 2 and never re-entered Phase 1 --
the only place that calls tryConnect(). The socket was never closed.
Worse, the timed-out request stayed outstanding on it. Once the link
returned, the inverter's late reply was read as the answer to the next
request and every read after that was one frame behind, reported as
EMBBADDATA. That is also TRANSIENT, so the device retried the same dead
socket with growing backoff indefinitely.
Escalate ETIMEDOUT to RECONNECT in executeTransaction() when the transport
is TCP. The check sits there rather than in deduceSeverity() because the
rule is narrower than "TCP": tryConnect() also sees ETIMEDOUT on TCP, from
a connect() whose SYN went unanswered, and that is a genuine transient
Phase 1 already retries with backoff. modbus_read_registers() is the only
call in the library that can produce the code on an established transport,
so keeping the rule at that one site makes the wrong classification
unreachable from the wrong one, and leaves modbus_error.h transport-blind.
EMBBADDATA is deliberately not escalated. It is set in
check_confirmation(), which is transport-agnostic and reached as readily by
RTU, where a mismatched frame is what an RS485 collision produces;
libmodbus recovers from it with modbus_flush() under
MODBUS_ERROR_RECOVERY_PROTOCOL and reserves modbus_connect() for link
errors. Escalating it would apply link-class recovery to a protocol-class
fault and, on a shared serial bus, drop every other device for one bad
frame. It also cannot arise here once the timeout escalates, because the
socket is closed before a desync can accumulate.
Also corrects the deduceSeverity() comment, which asserted that a dropped
peer follows ETIMEDOUT with ECONNRESET/EPIPE. That holds for a serial
hangup, not for a TCP link-down.
The missing "Inverter 'primo' disconnected, trying to reconnect in ..."
line needs no separate fix. onBusDisconnect_ fires only from Phase 1 when
tryConnect() fails, so it reappears once the bus actually reconnects.
refactor!: make the bus callbacks mirror the device callbacks
BREAKING CHANGE: addBusDisconnectCallback() takes std::function<void()>
instead of std::function<void(int)> and fires on the transition rather
than per failed attempt. Consumers wanting the old per-attempt delay must
register addBusRetryCallback() instead.
onBusConnect_ fired when the state became connected; onBusDisconnect_
fired when an attempt to restore it failed. The names implied a pair the
fire sites did not form, and the asymmetry had two consequences. An outage
that recovered on its first attempt reported nothing but an errno through
onBusError_, conventionally logged at debug, so a brief drop left no trace.
And the bus offered no analogue of setDeviceRetryCallback() even though it
had exactly that behaviour, which made a bus retry and a device retry
indistinguishable in a consumer's callback set despite meaning different
things: the transport is gone, versus the transport is fine but this slave
will not validate.
Split the two meanings. onBusDisconnect_ becomes void() and fires once
from the Phase 2 exit, where connected_ actually changed -- reached by a
RECONNECT error from executeTransaction() and by triggerReconnect() alike.
onBusRetry_ takes over the old signature and fire site. The bus now
mirrors the device: a state pair, an error channel, a retry notice.
The reconnect delay stays off the disconnect callback deliberately. Naming
one at drop time would be a prediction -- the attempt has not run, and if
it succeeds the wait never happens -- which was the objection in af4010b
that kept onBusDisconnect_ out of this site in the first place. It applies
to the delay, not to the notification, so a callback without one can fire
here without reviving the double reporting af4010b removed.
Shutdown does not fire onBusDisconnect_ even though it is a transition.
Consumers log their own teardown, and firing then would race with the
removeBusCallback() calls their destructors are making.
fix: read the nameplate payload, not just its block header
validateNameplateRegisters() fetched I120::ID.NB + I120::L.NB, which is the
two-register block header at 40121/40122. It checked the model ID and the
block length and returned, leaving the payload from DERTYP (40123) onwards
untouched.
Nothing else covers that range: fetchInverterRegisters() polls the active
state code, the I10X/I11X inverter block and the I160 MPPT extension, and
none of them reach I120. So every getAcPowerRating() call indexed a region
of regs_ that no transaction had ever written. regs_ is resized to 0xFFFF
and zero-filled, so all four outputs - WRtg, VARtg and both VArRtg
quadrants - returned a clean 0 rather than failing, which is why this read
as a plausible value instead of an error.
Read the header and the payload in one transaction, matching what
validateMultiMpptRegisters() already does for I160. The nameplate block is
static, so the single read at validation time serves every later call.
Verified against a Primo 4.0-1 on the integer map: registers 40124-40127
hold WRtg=400 sf=1 and VARtg=400 sf=1, i.e. 4000 W and 4000 VA, where the
library previously reported 0 for both.
fix: 270 is the phase sequence, not a total current
REG::A claimed register 270 was the meter's total AC current. In the
Carlo Gavazzi EM/ET300 map this driver's proprietary path derives from,
270 is the phase sequence: 0 for L1-L2-L3, 1 for L1-L3-L2. An mbpoll
capture of a TS 65A-3 sub-meter confirms it, reading 0 while L1 drew
0.167 A and L2 drew 5.325 A.
Nothing decoded it, so no reported value was ever wrong: getAcCurrent()
already refused Phase::TOTAL on the proprietary map and returned before
reaching the register. The declaration was still a trap - the guard
reads like a limitation to lift rather than the absence of a register,
and lifting it would have silently reported a rotation code as amperes.
That map has no system current at all: three phase currents, a neutral
current on some variants, nothing equivalent to M20X::A. Say so in the
namespace documentation, and expose 270 as REG::PHSEQ, which is what it
is. It sits inside the 16-register system block the driver already
reads, so surfacing it later costs no bus traffic.
getAcCurrent() keeps its guard and its structure; only the now-nameless
proprietary argument changes, to a default Register. That argument is
unreachable behind the guard, and were it ever reached it decodes to
EINVAL rather than to whatever sits at the address.
Verified against the capture: the phase-block base and 14-register
stride, and A/W/VA/var/PF/V for L1 and L2 (V*I, sqrt(P^2+Q^2) and the VA
register agree to 0.1 % on L2). Still unverified: 320-326, the tail of
the L3 block, which read zero throughout because nothing was connected
to L3, and 258-268, which fell outside the captured window.
docs: trim implementation comments to release length
Several comments grew during the shared-bus and reconnect debugging into
multi-paragraph narratives that recorded how a conclusion was reached
rather than what a reader needs to keep the code correct.
Shorten them to the operative rule and its reason, and drop comments that
only restate the statement below them. The load-bearing rationale is kept
in every case: the ETIMEDOUT transport dependence, the RTU pre-send flush,
the EPROTO-over-EINVAL severity choice, the no-join-under-mtx_ deadlock,
and the RetryEntry lifetime invariant.
Where a rule is already stated in full on the public API, the
implementation now refers to it instead of repeating it, so the two cannot
drift apart.
Also fix two inaccuracies found while reading: cbMutex_ guards four
callback vectors, not three, and drop the duplicated callback banner
comment in fronius_bus.h.
No functional change.
docs: describe the meter firmware version as a Carlo Gavazzi code
getFwVersion() has returned a letter-plus-revision code such as "B5" for
the proprietary map since cd9e73d, but the declaration still promised
"major.minor" format, e.g. "1.5". Correct the Doxygen comment to match.
fix: make modbus_utils.h self-contained and drop a stale trace line
modbus_utils.h used std::reverse without <algorithm> and Register without
register_base.h. It compiled only because every consumer happened to
include both first; including it on its own, or through registers.h, fails
outright. Add the two includes.
The "[--] guard done, queue free" trace dates from 469ce57, when a guard
delay followed each transaction. 634d926 removed the post-failure sleep and
moved the remaining guard ahead of the send, so the line now announces the
completion of something that no longer happens there. The [tx]/[rx] pair
already brackets the transaction. Remove it.
The 500 ms slave-switch guard is left alone: 634d926 raised it from 100 ms
deliberately, and it is the same on every branch.
clang-format corrections