Skip to content

fix: make transaction reset best effort#240

Open
zakoverflow wants to merge 1 commit into
blues:masterfrom
zakoverflow:split/reset-best-effort
Open

fix: make transaction reset best effort#240
zakoverflow wants to merge 1 commit into
blues:masterfrom
zakoverflow:split/reset-best-effort

Conversation

@zakoverflow
Copy link
Copy Markdown
Contributor

Summary

Split out the transaction reset behavior change from PR #239 so it can be reviewed independently from the new NotePing() API.

This changes _noteTransactionShouldLock() so that when resetRequired is set, the transaction path makes a best-effort call to _Reset(), clears resetRequired, and proceeds with the transaction even if reset did not report success.

Rationale

ESP32 I2C initialization can repeatedly fail in a way that prevents progress when reset is treated as a hard gate. This keeps the reset attempt, but avoids blocking the following transaction solely because the reset hook failed.

Tests

  • docker run --rm -v "$PWD":/note-c -w /note-c ghcr.io/blues/note_c_ci:latest bash -lc 'cmake --build build --target NoteTransaction_test NoteTransactionHooks_test NoteReset_test _noteHardReset_test -j2 && ctest --test-dir build -R "(NoteTransaction|NoteReset|_noteHardReset)" --output-on-failure'

Result: 8/8 related transaction/reset tests passed.

Note: a full run_unit_tests.sh pass was attempted, but this workspace's reused Docker build directory produced CMake *_NOT_BUILT placeholders unless individual test targets were built. The directly relevant reset/transaction targets were rebuilt and passed.

@zakoverflow
Copy link
Copy Markdown
Contributor Author

Potential pitfalls / review concerns for this reset-policy change:

This PR intentionally changes the meaning of resetRequired in the normal transaction path. Before, it meant "the interface is known bad until reset succeeds." After this change, it means "try reset once, then allow the next transaction to prove whether the interface is usable." That may be the right tradeoff for the ESP32 I2C initialization case, but it is a real behavioral change.

Specific concerns to evaluate:

  1. Normal NoteTransaction() may now send a user request or command on an interface that just failed to resynchronize.

    Previously, a failed reset stopped the transaction before anything user-supplied was sent. With this change, the reset failure is ignored and the transaction proceeds. That can be useful if the reset hook is overly pessimistic, but it also means the library may knowingly transact on a potentially stale or unsynchronized transport.

  2. Commands are riskier than requests.

    Requests have a response path that can prove whether the Notecard understood the transaction. Commands may not. If reset fails and a command is sent anyway, the caller may get an empty success object from the SDK even though the command may not have reached or been processed by the Notecard.

  3. Stale input / line synchronization can affect the following transaction.

    The old hard-fail behavior avoided sending a new transaction after reset failed to drain/resync the interface. The new behavior can proceed with residual bytes still buffered or with the host and Notecard not clearly aligned on request/response framing. That increases the chance of a stale response, malformed response, or parse failure being attributed to the current transaction.

  4. resetRequired is cleared even when reset fails.

    If the following transaction happens to fail with an error path that re-marks reset required, the state recovers. But if the transaction appears to succeed, or if a command path cannot observe failure, the SDK has lost the information that reset did not actually succeed.

  5. This may mask the ESP32 I2C initialization issue rather than model it precisely.

    Best-effort reset may be the pragmatic fix, but it also makes it harder to distinguish "reset hook failed but transaction is actually fine" from "transport is still unhealthy and we are pushing through anyway." Reviewers should decide whether that ambiguity belongs globally in _noteTransactionShouldLock(), or whether it should be limited to a probe/init path.

  6. Test expectations now encode a less conservative contract.

    The updated tests correctly reflect this PR's behavior, but they also document the policy change: after reset failure, transaction attempt is expected. That is worth calling out clearly because it changes the safety posture of the core transaction path.

My recommendation is to merge this only if we are comfortable with the global policy shift for normal requests and commands. If the desired behavior is specifically "ESP32 I2C init should not get stuck behind a failing reset," a narrower alternative would be to keep normal transactions reset-or-fail and apply best-effort semantics only in the initialization/probe path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant