Conversation
📝 WalkthroughWalkthroughReplaced the compile-time NVS namespace with a runtime-configurable namespace and added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nimble/nimble/host/store/config/src/ble_store_nvs.c`:
- Around line 65-69: The current branch treats a NULL ns the same as an overly
long string and logs "Namespace string is too long", which is misleading; update
the conditional in the setter so you first check if ns == NULL and in that case
quietly assign NIMBLE_NVS_NAMESPACE = NIMBLE_NVS_DEFAULT_NAMESPACE (or log at
debug/verbose level), otherwise validate strlen(ns) against
NIMBLE_NVS_STR_NAME_MAX_LEN and only log an ESP_LOGE via LOG_TAG when the length
check fails before assigning the default; reference the symbols ns,
NIMBLE_NVS_NAMESPACE, NIMBLE_NVS_DEFAULT_NAMESPACE, NIMBLE_NVS_STR_NAME_MAX_LEN
and LOG_TAG when making this change.
- Around line 62-71: set_nvs_namespace currently stores the caller's pointer
into NIMBLE_NVS_NAMESPACE which can lead to dangling pointers; change it to copy
the incoming string into a static buffer and point NIMBLE_NVS_NAMESPACE at that
buffer (ensure the buffer size uses NIMBLE_NVS_STR_NAME_MAX_LEN, always
null-terminate, and fall back to NIMBLE_NVS_DEFAULT_NAMESPACE on overflow);
validate ns != NULL and strlen(ns) < NIMBLE_NVS_STR_NAME_MAX_LEN before copying,
log via ESP_LOGE(LOG_TAG) on overflow and use the static buffer contents
thereafter so callers may pass stack or heap strings safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bece196-cfc6-4b74-8f9b-673b941bd580
📒 Files selected for processing (2)
src/nimble/nimble/host/store/config/src/ble_store_nvs.csrc/nimconfig.h
This was referenced Mar 25, 2026
h2zero
force-pushed
the
nvs-custom-namespace
branch
2 times, most recently
from
1e0f0f3 to
8e60f7c
Compare
h2zero
force-pushed
the
nvs-custom-namespace
branch
from
8e60f7c to
6d4b2e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nimble/nimble/host/store/config/src/ble_store_nvs.c (1)
160-164:⚠️ Potential issue | 🔴 CriticalShiver me timbers!
TAGbe undefined and will scuttle yer compilation, cap'n!Arr, ye renamed
TAGtoLOG_TAGat line 54, but several functions still be usin' the oldTAGidentifier that now be walkin' the plank! This will cause compilation errors faster than ye can say "Davy Jones!"Affected locations using undefined
TAG:
- Line 162:
get_nvs_peer_record()- Lines 459, 465:
ble_store_nvs_peer_records()- Lines 677, 682:
ble_nvs_restore_peer_records()- Lines 761, 767, 775, 778:
ble_store_config_persist_eads()Replace all occurrences of
TAGwithLOG_TAGin these functions to fix the compilation errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nimble/nimble/host/store/config/src/ble_store_nvs.c` around lines 160 - 164, Several logging calls still use the old undefined symbol TAG (causing compile errors); update all uses of TAG to LOG_TAG in the affected functions — specifically replace TAG with LOG_TAG in get_nvs_peer_record(), ble_store_nvs_peer_records(), ble_nvs_restore_peer_records(), and ble_store_config_persist_eads() so the ESP_LOGx calls use the existing LOG_TAG identifier used at the top of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nimble/nimble/host/store/config/src/ble_store_nvs.c`:
- Around line 62-78: The function set_nimble_nvs_namespace currently accepts
empty strings because it only checks strlen(ns) < NIMBLE_NVS_STR_NAME_MAX_LEN;
update it to reject empty strings by ensuring ns is non-NULL and not empty
(e.g., check ns[0] != '\0' or strlen(ns) > 0) before copying into
nimble_nvs_namespace_buf; if the string is empty or too long, log the error and
set NIMBLE_NVS_NAMESPACE to NIMBLE_NVS_DEFAULT_NAMESPACE (references:
set_nimble_nvs_namespace, nimble_nvs_namespace_buf, NIMBLE_NVS_NAMESPACE,
NIMBLE_NVS_DEFAULT_NAMESPACE, NIMBLE_NVS_STR_NAME_MAX_LEN).
---
Outside diff comments:
In `@src/nimble/nimble/host/store/config/src/ble_store_nvs.c`:
- Around line 160-164: Several logging calls still use the old undefined symbol
TAG (causing compile errors); update all uses of TAG to LOG_TAG in the affected
functions — specifically replace TAG with LOG_TAG in get_nvs_peer_record(),
ble_store_nvs_peer_records(), ble_nvs_restore_peer_records(), and
ble_store_config_persist_eads() so the ESP_LOGx calls use the existing LOG_TAG
identifier used at the top of the file.
🪄 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: 834bca33-1904-485e-8b30-1efcfe92924e
📒 Files selected for processing (1)
src/nimble/nimble/host/store/config/src/ble_store_nvs.c
* Adds the function `set_nimble_nvs_namespace(const char*)` for users to switch bond data stores.
h2zero
force-pushed
the
nvs-custom-namespace
branch
from
6d4b2e8 to
5c45baa
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/nimble/nimble/host/store/config/src/ble_store_nvs.c (1)
62-78: Avast! A word o' caution about when to hoist this flag, captain!Changin' the namespace at runtime while NVS operations be in progress could lead to unpredictable results - there be no synchronization guardin' the
NIMBLE_NVS_NAMESPACEpointer. Consider documentatin' that this function should only be called before NimBLE initialization or after a full deinit, when no NVS operations be underway.📜 Suggested documentation addition
+/** + * Set the NVS namespace for storing BLE bond data. + * + * `@note` This function should be called before NimBLE initialization + * or after NimBLE_deinit(). Calling while NimBLE is active may + * result in undefined behavior. + * + * `@param` ns The namespace string (max 15 chars), or NULL to reset to default. + */ void set_nimble_nvs_namespace(const char *ns)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nimble/nimble/host/store/config/src/ble_store_nvs.c` around lines 62 - 78, The set_nimble_nvs_namespace function mutates the global NIMBLE_NVS_NAMESPACE pointer without synchronization which can cause races if called while NVS operations run; update the code and/or docs to prevent misuse: either document clearly in the function comment and public API that set_nimble_nvs_namespace must only be called before NimBLE initialization or after full deinitialization (no NVS activity), or add proper synchronization (e.g., a mutex or atomic swap) around writes to nimble_nvs_namespace_buf and updates to NIMBLE_NVS_NAMESPACE so runtime changes are safe; reference the symbols set_nimble_nvs_namespace, NIMBLE_NVS_NAMESPACE, and nimble_nvs_namespace_buf when applying the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/nimble/nimble/host/store/config/src/ble_store_nvs.c`:
- Around line 62-78: The set_nimble_nvs_namespace function mutates the global
NIMBLE_NVS_NAMESPACE pointer without synchronization which can cause races if
called while NVS operations run; update the code and/or docs to prevent misuse:
either document clearly in the function comment and public API that
set_nimble_nvs_namespace must only be called before NimBLE initialization or
after full deinitialization (no NVS activity), or add proper synchronization
(e.g., a mutex or atomic swap) around writes to nimble_nvs_namespace_buf and
updates to NIMBLE_NVS_NAMESPACE so runtime changes are safe; reference the
symbols set_nimble_nvs_namespace, NIMBLE_NVS_NAMESPACE, and
nimble_nvs_namespace_buf when applying the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d4ee851-e510-4fac-acbc-61581af97bca
📒 Files selected for processing (2)
src/nimble/nimble/host/store/config/src/ble_store_nvs.csrc/nimconfig.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nimconfig.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the function
set_nimble_nvs_namespace(const char*)for users to switch bond data stores.Fixes #959
Fixes #1102
Summary by CodeRabbit
New Features
Bug Fixes