fix: propagate application write rejections as ATT Error Responses - #442
fix: propagate application write rejections as ATT Error Responses#442jie-meng wants to merge 1 commit into
Conversation
…ror slot handleGattEvent's write branch always returned 0, so application-layer write rejections could never reach the client as ATT Error Responses. Add a per-write error slot on NimBLECharacteristic (set by the onWrite callback, reset before each write) and return it from handleGattEvent. No existing signatures change; writeEvent remains a void override. Vendor error range 0x80-0x9F is recommended for application codes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthrough
ChangesWrite Error Propagation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to This change lets applications return ATT errors for rejected characteristic writes. It is mergeable with owner awareness that overlapping or reentrant write callbacks could cause the shared error state to be attributed to the wrong client request. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation The changes remain within the linked issue scope. The added API, state reset, and characteristic write-path propagation directly support application-defined ATT Error Responses. No unrelated code changes are identified.
✨ 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 |
|
One question on release strategy: would you also want this backported to the |
|
@coderabbitai The Docstring Coverage check reports 62.50% (5/8 functions documented). Could you list exactly which 3 functions are counted as missing docstrings, so I can add them? Thanks! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR `#442` changed files and patch ==='
gh pr diff 442 --repo h2zero/esp-nimble-cpp -- src/NimBLECharacteristic.cpp src/NimBLECharacteristic.h src/NimBLEServer.cpp || true
echo
echo '=== Relevant source outlines ==='
ast-grep outline src/NimBLECharacteristic.h --items all --view expanded || true
ast-grep outline src/NimBLECharacteristic.cpp --items all --view expanded || true
ast-grep outline src/NimBLEServer.cpp --items all --view expanded || true
echo
echo '=== Docstring coverage configuration and workflow references ==='
rg -n -i -C 3 'docstring|documentation coverage|doxygen|doxypress' \
-g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' .github . 2>/dev/null || true
echo
echo '=== Recent check runs for PR head ==='
gh pr checks 442 --repo h2zero/esp-nimble-cpp || trueLength of output: 109655 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="h2zero/esp-nimble-cpp"
pr="442"
echo '=== PR file patches ==='
gh api "repos/$repo/pulls/$pr/files?per_page=100" \
--jq '.[] | select(.filename == "src/NimBLECharacteristic.cpp" or .filename == "src/NimBLECharacteristic.h" or .filename == "src/NimBLEServer.cpp") | "FILE: \(.filename)\n\(.patch // "(no patch returned)")\n"' \
|| true
echo
echo '=== Changed declaration and definition regions ==='
nl -ba src/NimBLECharacteristic.h | sed -n '220,290p'
nl -ba src/NimBLECharacteristic.cpp | sed -n '415,470p'
nl -ba src/NimBLEServer.cpp | sed -n '715,785p'
echo
echo '=== PR comments and reviews that mention documentation coverage ==='
{
gh api "repos/$repo/issues/$pr/comments?per_page=100" --paginate --jq '.[] | select((.body // "") | test("docstring|documentation coverage|62\\.50"; "i")) | "ISSUE COMMENT by \(.user.login):\n\(.body)\n"'
gh api "repos/$repo/pulls/$pr/comments?per_page=100" --paginate --jq '.[] | select((.body // "") | test("docstring|documentation coverage|62\\.50"; "i")) | "REVIEW COMMENT by \(.user.login):\n\(.body)\n"'
gh api "repos/$repo/pulls/$pr/reviews?per_page=100" --paginate --jq '.[] | select((.body // "") | test("docstring|documentation coverage|62\\.50"; "i")) | "REVIEW by \(.user.login):\n\(.body)\n"'
} || true
echo
echo '=== All GitHub check runs on the PR head ==='
head_sha=$(gh api "repos/$repo/pulls/$pr" --jq '.head.sha')
gh api "repos/$repo/commits/$head_sha/check-runs?per_page=100" \
--jq '.check_runs[] | {name, conclusion, details_url, output}' \
|| trueLength of output: 5372 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="h2zero/esp-nimble-cpp"
pr="442"
show_lines() {
local file="$1" start="$2" end="$3"
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
}
echo '=== Documentation near modified functions ==='
show_lines src/NimBLECharacteristic.h 55 105
show_lines src/NimBLECharacteristic.cpp 420 470
show_lines src/NimBLEServer.cpp 720 785
echo
echo '=== All PR conversation comments from CodeRabbit or mentioning coverage ==='
gh api "repos/$repo/issues/$pr/comments?per_page=100" --paginate \
--jq '.[] | select(.user.login == "coderabbitai" or ((.body // "") | test("docstring|coverage|document"; "i"))) | "ISSUE COMMENT by \(.user.login) at \(.created_at):\n\(.body)\n---"' \
|| true
echo
echo '=== All PR review comments from CodeRabbit or mentioning coverage ==='
gh api "repos/$repo/pulls/$pr/comments?per_page=100" --paginate \
--jq '.[] | select(.user.login == "coderabbitai" or ((.body // "") | test("docstring|coverage|document"; "i"))) | "REVIEW COMMENT by \(.user.login) at \(.created_at), \(.path):\n\(.body)\n---"' \
|| true
echo
echo '=== All PR reviews from CodeRabbit or mentioning coverage ==='
gh api "repos/$repo/pulls/$pr/reviews?per_page=100" --paginate \
--jq '.[] | select(.user.login == "coderabbitai" or ((.body // "") | test("docstring|coverage|document"; "i"))) | "REVIEW by \(.user.login) at \(.submitted_at):\n\(.body)\n---"' \
|| true
echo
echo '=== Check-run metadata ==='
head_sha=$(gh api "repos/$repo/pulls/$pr" --jq '.head.sha')
gh api "repos/$repo/commits/$head_sha/check-runs?per_page=100" \
--jq '.check_runs[] | "CHECK: \(.name) | conclusion=\(.conclusion) | details=\(.details_url)\nTITLE: \(.output.title // "")\nSUMMARY: \(.output.summary // "")\nTEXT: \(.output.text // "")\n---"' \
|| trueLength of output: 19202
I inspected the PR patch. These are the functions that the diff visibly touches:
The diff contains six touched declarations or definitions, not eight. Each has a preceding Doxygen block. The reported You are interacting with an AI system. |
|
Thanks for the analysis — agreed, treating the Docstring Coverage warning as a false positive. Every touched declaration/definition in this diff (setWriteError/getWriteError in both header and implementation, writeEvent, handleGattEvent) already carries a preceding Doxygen block, so I'll leave the code as is rather than add speculative docstrings. |
…ackport to 2.5) Backport of h2zero#442 to the release/2.5 branch. - NimBLECharacteristic: Added setWriteError(uint8_t) and getWriteError() const - NimBLECharacteristic::writeEvent(): Resets error slot before each write - NimBLEServer::handleGattEvent(): Returns write error for characteristic writes Co-authored-by: Jie Meng <jmengxy@gmail.com>
1 participant
Description
Closes #441.
handleGattEvent's write branch hardcodesreturn 0, so an application that rejects a write insideonWrite()(busy, validation failure, ...) can never tell the client — the client always receives a successful Write Response. The NimBLE C stack supports ATT Error Responses from the access callback, and Write Requests (opcode 0x12) support them per spec; the capability is only lost in this wrapper.Because
NimBLELocalValueAttribute::writeEventis a pure virtualvoid(overridden by bothNimBLECharacteristicandNimBLEDescriptor), the error cannot be threaded through the existing signatures without a breaking API change. This PR therefore uses a per-write error slot on the characteristic — fully backwards compatible:void NimBLECharacteristic::setWriteError(uint8_t attError)— called by the application insideonWrite();0accepts, vendor range0x80–0x9Ffor application-defined error codes.int NimBLECharacteristic::getWriteError() const— read byhandleGattEvent, which returns the slot value on theOP_WRITE_CHRpath and0on theOP_WRITE_DSCpath.writeEvent()resets the slot to0before invokingonWrite().Changes
src/NimBLECharacteristic.hsetWriteError/getWriteError, privateint m_writeError = 0src/NimBLECharacteristic.cppwriteEventbeforeonWrite; two method implementationssrc/NimBLEServer.cppNo existing signature, virtual, or behavior changes for applications that don't call
setWriteError.Client-visible behavior
GATT_SUCCESSstatus inonCharacteristicWrite; iOS as an error inperipheral:didWriteValueForCharacteristic:error:.Known pre-existing behavior (unchanged, for the record)
writeEventappliessetValue(val, len)before invokingonWrite, so a rejected write still updates the locally stored attribute value. This predates this PR; an ATT Error Response tells the client to treat the value as unchanged, and a rollback pass is left out of scope here.Validation
master(identical code context); it has not been compiled against 3.0.0-dev independently.Summary by CodeRabbit