Skip to content

feat: NotePing() allows for testing to see if a notecard is responding sans delays/retry#239

Open
rayozzie wants to merge 2 commits into
masterfrom
ray/autobaud
Open

feat: NotePing() allows for testing to see if a notecard is responding sans delays/retry#239
rayozzie wants to merge 2 commits into
masterfrom
ray/autobaud

Conversation

@rayozzie
Copy link
Copy Markdown
Contributor

The new NotePing method issues an echo transaction without delays or retries so that one can quickly determine whether a notecard is connected and communicating successfully. This is used by the note-arduino notecard.ping() to do automatic baud rate recovery during init.

…g sans delays/retry

The new NotePing method issues an echo transaction *without delays or retries* so that one can quickly determine whether a notecard is connected and communicating successfully.  This is used by the note-arduino notecard.ping() to do automatic baud rate recovery during init.
@rayozzie rayozzie requested a review from zfields April 10, 2026 20:56
Copy link
Copy Markdown
Contributor

@zakoverflow zakoverflow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes.

The intent of a fast single-attempt connectivity probe for autobaud makes sense, but this PR currently changes the behavior of the normal transaction path in a way that breaks existing note-c architecture.

Blocking findings:

  1. _noteTransactionShouldLock() now ignores reset failure for all normal transactions.

In n_request.c, the reset-required path changed from "attempt reset, abort if reset failed" to "call _Reset(), ignore the result, clear resetRequired, and continue." That is why CI is failing in NoteTransaction_test.cpp: the tests expect no _noteJSONTransaction() call after a failed reset, but the PR now proceeds into the transaction anyway.

This should not be fixed by changing those tests. The old behavior is the right contract for NoteTransaction(): if the transport is known to require reset/re-sync and reset fails, the library should not attempt the user request/command on a known-bad interface. If NotePing() needs to bypass reset state for autobaud probing, that exception should stay inside NotePing() only.

Suggested fix: restore the previous reset failure handling in _noteTransactionShouldLock(), and keep the "do not honor resetRequired" behavior local to NotePing().

  1. NotePing() bypasses the existing I2C locking discipline while draining input.

_notePingDrainInput() calls _I2CReceive() directly. Existing I2C transaction/reset flows lock around I2C operations via _i2cNoteTransaction(), _i2cNoteReset(), or the I2C mutex helpers. This direct receive path can race with other I2C users and does not fit the existing transport abstraction.

Suggested fix: either use an existing locked abstraction for the drain or explicitly take/release the I2C mutex around this drain path.

  1. API naming/docs need to reflect that this is a transport probe, not a general health check.

NotePing() is acceptable if we want a simple public name, but the implementation does a very specific thing: it sends one echo, skips reset, skips CRC, skips retries, suppresses normal transaction logs, uses a short fixed timeout, and drains residual serial/I2C input. That is closer to a transport/autobaud probe than a normal "is the Notecard healthy?" API.

I would consider NoteProbe() or NoteProbeConnection() if this is primarily for autobaud/transport detection. If we keep NotePing(), the docs should explicitly say it is a single-attempt transport probe and not a substitute for the normal request path.

Test notes:

  • The unit-test failures are explained by finding #1: NoteTransaction_test.cpp reset-failure sections now see _noteJSONTransaction() called after failed reset.
  • The failing notecard-binary-test appears unrelated to this PR. The job failed before firmware tests ran because the Notestation reservation client exited with Connect call failed ('100.87.163.107', 65432).

I would be comfortable with this API after the normal transaction reset semantics are restored, the I2C drain follows the established locking model, and focused tests are added for NotePing() success/failure/no-retry/resetRequired/drain behavior.

@zakoverflow
Copy link
Copy Markdown
Contributor

I prepared the requested follow-up commit, but I cannot push directly to this PR branch: GitHub rejected zakoverflow pushing to blues:ray/autobaud with 403.

I opened the commit as a companion PR targeting this branch:

#241

That PR:

  • restores the original _noteTransactionShouldLock() reset-or-fail behavior, removing the reset-policy change from this PR
  • adds focused NotePing() unit coverage for success, nonce mismatch, error response, invalid JSON, transaction error, missing response, no retry, no reset when resetRequired is set, transaction-start failure cleanup, no CRC, 500 ms timeout, newline termination, and serial drain before transmit

Verification in the note-c CI container:

  • NotePing_test: 30 assertions passed
  • NoteTransaction_test: 124 assertions passed

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.

2 participants