Restore GAP device name after GATT reset#426
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughArr, this here change makes ChangesGATT Reset GAP Value Preservation
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/NimBLEServer.cpp`:
- Around line 894-896: The static members NimBLEDevice::m_deviceName and
NimBLEDevice::m_deviceNameSet are accessed without synchronization in the
resetGATT() method, creating a potential race condition if another thread calls
setDeviceName() concurrently. To fix this, add a static mutex member (like
m_deviceNameMutex) to the NimBLEDevice class, then protect all accesses to
m_deviceName and m_deviceNameSet with std::lock_guard in both the
setDeviceName() method and the resetGATT() method. In resetGATT(), acquire the
lock, cache the values of m_deviceNameSet and m_deviceName into local variables,
release the lock, then perform the conditional call to setDeviceName() with the
cached name outside the critical section.
🪄 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: a7ecf420-5f09-4fa4-8875-aea742681393
📒 Files selected for processing (3)
src/NimBLEDevice.cppsrc/NimBLEDevice.hsrc/NimBLEServer.cpp
| if (NimBLEDevice::m_deviceNameSet && !NimBLEDevice::setDeviceName(NimBLEDevice::m_deviceName)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Shiver me timbers! A potential race condition be lurkin' in these waters!
Arr matey, while yer logic fer reapplyin' the cached device name be sound as a Spanish doubloon, there be a concurrency concern that could make yer ship run aground:
The static members NimBLEDevice::m_deviceName and NimBLEDevice::m_deviceNameSet be accessed here without any synchronization. If some scallywag calls NimBLEDevice::setDeviceName() from another thread whilst this resetGATT() be executin', ye could have yerself a classic race condition! One thread be writin' to m_deviceName while another be readin' from it - that be a recipe fer trouble on the high seas.
Now, in most BLE applications, these operations be serialized by the nature o' the code flow. But from a library design standpoint, there be no enforcement o' this serialization, savvy?
🏴☠️ Possible solutions fer this concurrency concern
Option 1: Document that BLE operations must be serialized by the application (low effort, shifts responsibility to user)
Option 2: Add a mutex to protect access to the static members (higher effort, makes library thread-safe by design)
// In NimBLEDevice.h, add a static mutex
static std::mutex m_deviceNameMutex;
// In setDeviceName:
{
std::lock_guard<std::mutex> lock(m_deviceNameMutex);
m_deviceName = deviceName;
m_deviceNameSet = true;
}
// In resetGATT:
std::string cachedName;
bool nameWasSet;
{
std::lock_guard<std::mutex> lock(NimBLEDevice::m_deviceNameMutex);
cachedName = NimBLEDevice::m_deviceName;
nameWasSet = NimBLEDevice::m_deviceNameSet;
}
if (nameWasSet && !NimBLEDevice::setDeviceName(cachedName)) {
return false;
}🤖 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/NimBLEServer.cpp` around lines 894 - 896, The static members
NimBLEDevice::m_deviceName and NimBLEDevice::m_deviceNameSet are accessed
without synchronization in the resetGATT() method, creating a potential race
condition if another thread calls setDeviceName() concurrently. To fix this, add
a static mutex member (like m_deviceNameMutex) to the NimBLEDevice class, then
protect all accesses to m_deviceName and m_deviceNameSet with std::lock_guard in
both the setDeviceName() method and the resetGATT() method. In resetGATT(),
acquire the lock, cache the values of m_deviceNameSet and m_deviceName into
local variables, release the lock, then perform the conditional call to
setDeviceName() with the cached name outside the critical section.
|
Looks like espressif changed the behavior of this :( I think a better solution is to save the values in Personally I don't use espressifs fork anymore for this reason. |
|
If your confusion was my remark about the espressif fork, I use this instead: https://github.com/h2zero/esp-nimble-component Updated to mynewt/nimble 1.9, doesn't have much of the espressif changes and is more stable. |
|
Ok if I fix it with the espressif version? Should we detect which stack and
only apply it for espressif version?
…On Wed, Jun 24, 2026 at 4:56 PM Ryan Powell ***@***.***> wrote:
*h2zero* left a comment (h2zero/esp-nimble-cpp#426)
<#426 (comment)>
@finger563 <https://github.com/finger563>
If your confusion was my remark about the espressif fork, I use this
instead: https://github.com/h2zero/esp-nimble-component
Updated to mynewt/nimble 1.9, doesn't have much of the espressif changes
and is more stable.
—
Reply to this email directly, view it on GitHub
<#426?email_source=notifications&email_token=AAJH4ADE5AOF2BGL577L4ND5BQ565A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINZZGM2TGOJZGI32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4793539927>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJH4ABY6GTH43UFO6BKOGL5BQ565AVCNFSNUABFKJSXA33TNF2G64TZHMZDKMJRGU3DKNRYHNEXG43VMU5TINRZG4ZTSOJZGEZKC5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AAJH4AEIGOQNGOS4E55TD4T5BQ565A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINZZGM2TGOJZGI32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/AAJH4AFMLQFXF7QRTVLXSYD5BQ565A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINZZGM2TGOJZGI32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Sure, we can use this flag: |
|
Has that been updated or does it work for esp idf v6? |
|
It's a bit of work in progress still, haven't checked it against IDF v6. The same code is being used in nimble-arduino as well so it will be well tested. |
|
@finger563, updated now, idf 5 and 6 working. |
are you using that mynewt version with esp-idf or another platform? I figured there were a ton of porting changes present to support the particular ble devices so it seemed overwhelming to consider going that route. |
ESP-IDF NimBLE can reset the GAP Device Name when the standard GAP service is reinitialized during GATT server startup. NimBLEDevice::init(deviceName) calls NimBLEDevice::setDeviceName(deviceName), but that happens before the server is started. Later, starting advertising starts the server and resets/re-registers the GAP service, causing the C GAP name backing storage to be rebuilt from CONFIG_BT_NIMBLE_SVC_GAP_DEVICE_NAME. In projects where that Kconfig default remains "nimble", the advertised Local Name can be correct while the Generic Access Device Name characteristic reports "nimble".
The failing sequence is:
-> pAdvertising->start()
-> NimBLEAdvertising::start()
-> NimBLEDevice::getServer()->start()
-> NimBLEServer::resetGATT()
-> ble_gatts_reset()
-> ble_svc_gap_init()
-> ESP-IDF NimBLE resets GAP name to CONFIG_BT_NIMBLE_SVC_GAP_DEVICE_NAME
-> "nimble"
-> ble_svc_gatt_init()
Fix this by saving the device name supplied through NimBLEDevice::setDeviceName() and reapplying it immediately after ble_svc_gap_init() in NimBLEServer::resetGATT(). This preserves the C++ wrapper contract that the configured device name remains effective even when the wrapper internally resets the GATT/GAP service tables.
chmorgan
force-pushed
the
feature/restore-after-ble-svc-gap-init
branch
from
4063b55 to
975c084
Compare
3 participants
NOTE: Codex 5.5 created this patch and writeup etc but I reviewed it and it looks like what I would have done by hand, and tested it here and it does resolve the issue and explains why I've been seeing my device misnamed for a bit. Not sure how or when I broke it, or if the issue was library side or a nimble change but figured others might be hitting this issue.
ESP-IDF NimBLE can reset the GAP Device Name when the standard GAP service is reinitialized during GATT server startup. NimBLEDevice::init(deviceName) calls NimBLEDevice::setDeviceName(deviceName), but that happens before the server is started. Later, starting advertising starts the server and resets/re-registers the GAP service, causing the C GAP name backing storage to be rebuilt from CONFIG_BT_NIMBLE_SVC_GAP_DEVICE_NAME. In projects where that Kconfig default remains "nimble", the advertised Local Name can be correct while the Generic Access Device Name characteristic reports "nimble".
The failing sequence is:
-> pAdvertising->start()
-> NimBLEAdvertising::start()
-> NimBLEDevice::getServer()->start()
-> NimBLEServer::resetGATT()
-> ble_gatts_reset()
-> ble_svc_gap_init()
-> ESP-IDF NimBLE resets GAP name to CONFIG_BT_NIMBLE_SVC_GAP_DEVICE_NAME
-> "nimble"
-> ble_svc_gatt_init()
Fix this by saving the device name supplied through NimBLEDevice::setDeviceName() and reapplying it immediately after ble_svc_gap_init() in NimBLEServer::resetGATT(). This preserves the C++ wrapper contract that the configured device name remains effective even when the wrapper internally resets the GATT/GAP service tables.
Summary by CodeRabbit