Add onReadStatus/onWriteStatus callbacks so an application can reject a read/write with an ATT error

[Nicba1010]

Add onReadStatus/onWriteStatus callbacks so an application can reject a read/write with an ATT error

Addresses #428.

Short version of the problem (full detail in the issue): onWrite() returns void
and handleGattEvent hardcodes a success return on the write path, so there's no way
for an application to reject a write and have the peer receive an ATT error — even under
write-with-response, where the protocol allows it. The C host honors a non-zero return
from the access callback; the wrapper just doesn't expose that to application code.

What this does

Adds two opt-in, status-returning callbacks to NimBLECharacteristicCallbacks:

virtual int onReadStatus (NimBLECharacteristic*, NimBLEConnInfo&);
virtual int onWriteStatus(NimBLECharacteristic*, NimBLEConnInfo&);

Each returns 0 to accept or a BLE_ATT_ERR_* code to reject. Their default
implementations call the existing onRead/onWrite and return 0, so behavior is
unchanged for anyone who doesn't override them. Internally the readEvent/writeEvent
chain (NimBLELocalValueAttribute → NimBLECharacteristic/NimBLEDescriptor) now
returns int, and handleGattEvent forwards that code to the host instead of always
returning 0. On a rejected read it returns before appending the value.

Six files, +51/-15. No public signature changes to existing methods.

Backward compatibility

Nothing existing changes shape. onRead/onWrite keep their void signatures, and
every current override keeps working untouched — the new onReadStatus/onWriteStatus
default to invoking them and accepting. It's purely additive.

I went with the additive form because it drops into a minor release without touching
anyone. That said — since master is already 3.0.0-dev, if you'd rather take the cleaner
breaking version for the major (just change onRead/onWrite themselves to return int,
0 = accept, and drop the separate status methods), I'm happy to reshape this into that.
It mirrors the C stack one-to-one; the only reason I didn't lead with it is the override
break. Your call on which fits 3.0 better.

One caveat

A rejection only reaches the peer under write-with-response. A write-without-response
(ATT Write Command) is unacknowledged, so the stack drops the code and sends nothing —
that's per spec, not something this changes. Worth a mention in the docs so nobody expects
a rejection on a WRITE_NO_RSP characteristic.

Descriptors are compile-updated to the new int return but always accept; the same
onWriteStatus-style hook could be added to NimBLEDescriptorCallbacks later if there's
demand.

Testing

Builds clean for esp32s3. I also validated it end-to-end on an ESP32-S3 with a macOS
central: a characteristic returning BLE_ATT_ERR_INSUFFICIENT_RES from onWriteStatus
surfaced on the central as CBATTErrorDomain Code=17 "Resources are insufficient." under
write-with-response, and produced no error under write-without-response — matching the
caveat above. Existing writes to unmodified characteristics were unaffected.

There's an open question from the issue worth settling here too: writeEvent commits
setValue() before the callback runs, so on a rejected write the local value has already
been updated. This PR keeps that ordering; if you'd prefer the pending (data, len) be
passed to onWriteStatus so an app can validate before commit, I can add that.

---

AI usage disclosure

For transparency: I used an AI assistant while working on this — to help trace the call
path through the C host stack, cross-check the behavior against the source, and draft the
code. Flagging that so it's on the table. The changes are small and the cited code paths
point at specific locations, so they're straightforward to review against the tree.

Summary by CodeRabbit

• New Features
  • Read and write requests can now return ATT status codes to accept or reject operations.
  • Added characteristic callback hooks for read/write status codes (with default behavior that preserves existing callbacks).
• Bug Fixes
  • GATT handling now properly propagates application accept/reject results for reads and writes.
  • Descriptor and characteristic read/write handling now follows the status-based behavior, including reverting characteristic value changes when a write is rejected.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5f3a1bff-be80-4ee1-9983-ef3aba4446e0

📥 Commits

Reviewing files that changed from the base of the PR and between 5a80bb0 and 470d62d.

📒 Files selected for processing (1)

• src/NimBLECharacteristic.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• src/NimBLECharacteristic.cpp

---

📝 Walkthrough

Walkthrough

Arrr, this PR turns GATT read/write handlers into status-returning paths across NimBLELocalValueAttribute, NimBLECharacteristic, NimBLEDescriptor, and NimBLEServer, so callbacks can reject operations with BLE_ATT_ERR_* codes.

Changes

Status-returning read/write events

Layer / File(s)	Summary
Pure virtual event contract src/NimBLELocalValueAttribute.h	readEvent/writeEvent pure virtuals change from void to int, with docs describing 0 for success and BLE_ATT_ERR_* for rejection.
Characteristic status callbacks and handlers src/NimBLECharacteristic.h, src/NimBLECharacteristic.cpp	New onReadStatus/onWriteStatus virtual callbacks forward to onRead/onWrite by default and return 0; readEvent/writeEvent now return int, with write rollback on non-zero status.
Descriptor event handlers src/NimBLEDescriptor.h, src/NimBLEDescriptor.cpp	readEvent/writeEvent overrides return int, but always return 0 after invoking the existing descriptor callbacks.
Server dispatch of status codes src/NimBLEServer.cpp	handleGattEvent now propagates readEvent return codes and returns writeEvent results directly instead of always succeeding.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant NimBLEServer
  participant NimBLECharacteristic
  participant NimBLECharacteristicCallbacks
  NimBLEServer->>NimBLECharacteristic: readEvent(connInfo)
  NimBLECharacteristic->>NimBLECharacteristicCallbacks: onReadStatus(this, connInfo)
  NimBLECharacteristicCallbacks-->>NimBLECharacteristic: status code
  NimBLECharacteristic-->>NimBLEServer: status code (reject if non-zero)
  NimBLEServer->>NimBLECharacteristic: writeEvent(val, len, connInfo)
  NimBLECharacteristic->>NimBLECharacteristic: setValue(val, len)
  NimBLECharacteristic->>NimBLECharacteristicCallbacks: onWriteStatus(this, connInfo)
  NimBLECharacteristicCallbacks-->>NimBLECharacteristic: status code
  NimBLECharacteristic-->>NimBLEServer: status code returned directly

Loading

Possibly related issues

• Allow an application to reject a write from onWrite (return an ATT error to the client) #428: This PR implements status-returning read/write callback plumbing and rejection codes for GATT operations.

Poem

Arrr, the handlers speak in code,
No more void drift on the GATT road.
Reads may sail, or reads may stall,
Writes can roll back if status calls.
A tiny int now hoists the flag —
Yarrr, the server learns to heed the tag.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Arrr, the title clearly matches the main change: new status callbacks let applications reject reads and writes with ATT errors.
Docstring Coverage	✅ Passed	Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/NimBLECharacteristic.h (1)

331-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Arr, append new virtual callbacks after the existing ones.

These new virtuals are inserted before onStatus/onSubscribe, shifting existing vtable slots for consumers using precompiled objects. If binary compatibility matters, place the new virtuals after the existing callback declarations or document the ABI break.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/NimBLECharacteristic.h` around lines 331 - 349, The new virtual callbacks
in NimBLECharacteristic are being inserted before existing callbacks and will
shift vtable layout for callers using precompiled objects. Move onReadStatus and
onWriteStatus in NimBLECharacteristic so they are declared after the existing
callback methods such as onStatus and onSubscribe, or otherwise clearly document
the ABI break in the class interface.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/NimBLECharacteristic.cpp`:
- Around line 435-437: The write path in NimBLECharacteristic::writeEvent
currently commits the new payload via setValue() before
m_pCallbacks->onWriteStatus() decides whether the write is accepted, so rejected
writes still update local state. Change writeEvent to preserve the previous
characteristic value, call onWriteStatus() first or otherwise detect a
non-success BLE_ATT_ERR_* result, and restore the prior value if the write is
rejected; use the NimBLECharacteristic::writeEvent and setValue() flow as the
place to implement the rollback.

---

Nitpick comments:
In `@src/NimBLECharacteristic.h`:
- Around line 331-349: The new virtual callbacks in NimBLECharacteristic are
being inserted before existing callbacks and will shift vtable layout for
callers using precompiled objects. Move onReadStatus and onWriteStatus in
NimBLECharacteristic so they are declared after the existing callback methods
such as onStatus and onSubscribe, or otherwise clearly document the ABI break in
the class interface.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 03dd77e4-3543-4489-8546-dee43af446dc

📥 Commits

Reviewing files that changed from the base of the PR and between 1eddc28 and 5a80bb0.

📒 Files selected for processing (6)

• src/NimBLECharacteristic.cpp
• src/NimBLECharacteristic.h
• src/NimBLEDescriptor.cpp
• src/NimBLEDescriptor.h
• src/NimBLELocalValueAttribute.h
• src/NimBLEServer.cpp