fix(AdapterClient): readData timeout error is now non-fatal#45
fix(AdapterClient): readData timeout error is now non-fatal#45
Conversation
…erable When adapter.readData returns an error (e.g. timeout with code -32000), the session now recovers gracefully instead of terminating. The error handler emits readDataResult with INVALID entries matching the configured register count, allowing the poll loop to re-arm and continue data collection. This mirrors the existing non-fatal error handling for validateDataPoint and expressionHelp, keeping the session alive for transient I/O errors. Changes: - onErrorReceived: add non-fatal guard for adapter.readData in ACTIVE state - Tests: readDataErrorIsNonFatal, readDataErrorInNonActiveStateIsIgnored Fixes #43 Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 29 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdapterClient now treats Changes
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 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/ProtocolAdapter/tst_adapterclient.cpp (1)
1007-1025: Test name implies broader coverage than provided — consider adding IDLE state.At the point the error is injected,
_state == STOPPING(afterstopSession()), not generically "non-active". The absence ofsessionErrorhere is actually produced by the pre-existingpreviousState == STOPPINGguard further downonErrorReceived, not by the new readData branch (which only fires forACTIVE). The test is still valid, but a parallel case in IDLE would tighten coverage of the "ignored when not active" claim and document that a stale readData error in IDLE does not spuriously emitreadDataResult.♻️ Optional additional case
+ /* Also verify IDLE: no driveToActive, no stopSession — pure stale-error path */ + { + auto* mock2 = new MockAdapterProcess(); + AdapterClient client2(mock2); + QSignalSpy spyError2(&client2, &AdapterClient::sessionError); + QSignalSpy spyData2(&client2, &AdapterClient::readDataResult); + mock2->injectError(99, "adapter.readData", error); + QCOMPARE(spyData2.count(), 0); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ProtocolAdapter/tst_adapterclient.cpp` around lines 1007 - 1025, Test name TestAdapterClient::readDataErrorInNonActiveStateIsIgnored suggests all non-active states are covered but only STOPPING is exercised; add a parallel IDLE case by driving the client to IDLE (instead of ACTIVE) and injecting the same error to ensure AdapterClient::onErrorReceived does not emit readDataResult or sessionError when _state is IDLE; specifically reuse driveToActive/stopSession pattern for STOPPING test and create a new test that transitions to IDLE (or constructs the client without activating), then call MockAdapterProcess::injectError(5, "adapter.readData", error) and assert spyData.count() == 0 and spyError.count() == 0 to document the ignored-readData behavior for IDLE as well.src/ProtocolAdapter/adapterclient.cpp (1)
250-260: Non-fatal readData branch is correct; minor simplification possible.Behaviour matches the issue's expected outcome: state stays
ACTIVE, nosessionError, and areadDataResultof INVALID entries letsModbusPollre-arm. Sizing by_pendingExpressionsis appropriate since that is exactly what was sent toadapter.start.The manual loop can be replaced by Qt's sized constructor for clarity.
♻️ Optional simplification
- ResultDoubleList invalidResults; - invalidResults.reserve(_pendingExpressions.size()); - for (int i = 0; i < _pendingExpressions.size(); ++i) - { - invalidResults.append(ResultDouble(0.0, ResultState::State::INVALID)); - } - emit readDataResult(invalidResults); + ResultDoubleList invalidResults(_pendingExpressions.size(), ResultDouble(0.0, ResultState::State::INVALID)); + emit readDataResult(invalidResults);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ProtocolAdapter/adapterclient.cpp` around lines 250 - 260, Replace the manual loop that builds invalidResults in the adapter.readData branch with a sized constructor or fill call to create the list in one step: construct ResultDoubleList using the size _pendingExpressions.size() and a single default ResultDouble(0.0, ResultState::INVALID) (or call invalidResults.fill(...) after constructing), keeping the rest of the branch logic (method check, _state == State::ACTIVE, emit readDataResult(invalidResults), return) unchanged so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ProtocolAdapter/adapterclient.cpp`:
- Around line 250-260: Replace the manual loop that builds invalidResults in the
adapter.readData branch with a sized constructor or fill call to create the list
in one step: construct ResultDoubleList using the size
_pendingExpressions.size() and a single default ResultDouble(0.0,
ResultState::INVALID) (or call invalidResults.fill(...) after constructing),
keeping the rest of the branch logic (method check, _state == State::ACTIVE,
emit readDataResult(invalidResults), return) unchanged so behavior remains
identical.
In `@tests/ProtocolAdapter/tst_adapterclient.cpp`:
- Around line 1007-1025: Test name
TestAdapterClient::readDataErrorInNonActiveStateIsIgnored suggests all
non-active states are covered but only STOPPING is exercised; add a parallel
IDLE case by driving the client to IDLE (instead of ACTIVE) and injecting the
same error to ensure AdapterClient::onErrorReceived does not emit readDataResult
or sessionError when _state is IDLE; specifically reuse
driveToActive/stopSession pattern for STOPPING test and create a new test that
transitions to IDLE (or constructs the client without activating), then call
MockAdapterProcess::injectError(5, "adapter.readData", error) and assert
spyData.count() == 0 and spyError.count() == 0 to document the ignored-readData
behavior for IDLE as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c49cc416-dc88-46e5-a760-a3c2f5f4f943
📒 Files selected for processing (3)
src/ProtocolAdapter/adapterclient.cpptests/ProtocolAdapter/tst_adapterclient.cpptests/ProtocolAdapter/tst_adapterclient.h
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ProtocolAdapter/adapterclient.cpp (1)
250-255: Minor: consider logging the swallowed error for diagnosability.Other non-fatal branches above also don't log explicitly, but
_handshakeTimer.stop()and the warning at line 225 already cover that. Just confirming the intent: every transientreadDataerror will be visible only via the priorqCWarningand the resulting all-INVALIDResultDoubleList, which is fine but worth a brief comment explaining the rationale (mirrors the comment block at lines 227‑228 for aux requests).🪶 Optional clarifying comment
- if (method == QStringLiteral("adapter.readData") && _state == State::ACTIVE) + /* A readData failure (e.g. transient timeout, JSON-RPC -32000) is per-poll, + not session-fatal: surface an all-INVALID result so the poll loop can re-arm. */ + if (method == QStringLiteral("adapter.readData") && _state == State::ACTIVE) { ResultDoubleList invalidResults(_pendingExpressions.size(), ResultDouble(0.0, ResultState::State::INVALID)); emit readDataResult(invalidResults); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ProtocolAdapter/adapterclient.cpp` around lines 250 - 255, The branch handling adapter.readData when _state == State::ACTIVE returns an all-INVALID ResultDoubleList without logging; update the block in adapterclient.cpp (the if checking method == QStringLiteral("adapter.readData") && _state == State::ACTIVE) to either emit a brief diagnostic log (e.g., via qCWarning) including context (method, _pendingExpressions.size()) before emitting readDataResult(invalidResults) or add a short explanatory comment mirroring the existing comment used for aux requests and noting that _handshakeTimer.stop() / the earlier warning provide the primary diagnostics; ensure you reference ResultDoubleList, ResultDouble, ResultState::State::INVALID and readDataResult in the change so future readers see the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ProtocolAdapter/adapterclient.cpp`:
- Around line 250-255: The branch that creates and emits invalidResults for
"adapter.readData" currently only runs when _state == State::ACTIVE, allowing
stray readData errors in non-ACTIVE states to fall through and trigger
sessionError; change the guard so that when method ==
QStringLiteral("adapter.readData") && _state != State::ACTIVE you create
ResultDoubleList(_pendingExpressions.size(), ResultDouble(0.0,
ResultState::State::INVALID)), emit readDataResult(invalidResults) and return,
thereby preventing the code path that later captures previousState and emits
sessionError for idle/non-active sessions; update the condition around
readDataResult, leaving other active-state logic intact.
---
Nitpick comments:
In `@src/ProtocolAdapter/adapterclient.cpp`:
- Around line 250-255: The branch handling adapter.readData when _state ==
State::ACTIVE returns an all-INVALID ResultDoubleList without logging; update
the block in adapterclient.cpp (the if checking method ==
QStringLiteral("adapter.readData") && _state == State::ACTIVE) to either emit a
brief diagnostic log (e.g., via qCWarning) including context (method,
_pendingExpressions.size()) before emitting readDataResult(invalidResults) or
add a short explanatory comment mirroring the existing comment used for aux
requests and noting that _handshakeTimer.stop() / the earlier warning provide
the primary diagnostics; ensure you reference ResultDoubleList, ResultDouble,
ResultState::State::INVALID and readDataResult in the change so future readers
see the intent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6306535-b7f9-4112-998d-b2db8cce889c
📒 Files selected for processing (3)
src/ProtocolAdapter/adapterclient.cpptests/ProtocolAdapter/tst_adapterclient.cpptests/ProtocolAdapter/tst_adapterclient.h
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/ProtocolAdapter/tst_adapterclient.cpp
…e test coverage - Widen readData error guard to silently ignore errors in all non-ACTIVE states (IDLE, STOPPING, etc.), not just ACTIVE. This prevents readData errors in IDLE state from triggering sessionError. - Simplify invalidResults construction using QList sized constructor instead of reserve() + manual loop - Add TestAdapterClient::readDataErrorInIdleStateIsIgnored to verify IDLE state ignores readData errors (complements existing STOPPING state test) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
jgeudens
force-pushed
the
dev/fix_fault_handling
branch
from
fb7230a to
2d85a64
Compare
1 participant
When adapter.readData returns an error (e.g. timeout with code -32000), the session now recovers gracefully instead of terminating. The error handler emits readDataResult with INVALID entries matching the configured register count, allowing the poll loop to re-arm and continue data collection.
This mirrors the existing non-fatal error handling for validateDataPoint and expressionHelp, keeping the session alive for transient I/O errors.
Changes:
Fixes #43
Summary by CodeRabbit
Bug Fixes
Tests