Add a flag to to use arduino header paths - fixes PIO builds#420
Merged
Add a flag to to use arduino header paths - fixes PIO builds#420
Conversation
📝 WalkthroughWalkthroughArrr — replaced compile-time macro Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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: 1
🧹 Nitpick comments (1)
src/NimBLEExtAdvertising.cpp (1)
21-25: Arr, the cargo be mostly shipshape, but I spy a wee barnacle!The header path logic be correct, matey. However, I notice the
# endifon line 25 has inconsistent spacing compared to the#ifdefand#elseabove it. The preprocessor be tolerant of such things, but for a clean deck:🧹 Minor style cleanup for consistent preprocessor indentation
`#ifdef` USING_NIMBLE_ARDUINO_HEADERS # include "nimble/nimble/host/services/gap/include/services/gap/ble_svc_gap.h" `#else` # include "services/gap/ble_svc_gap.h" -# endif +#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEExtAdvertising.cpp` around lines 21 - 25, The preprocessor block using `#ifdef` USING_NIMBLE_ARDUINO_HEADERS / `#else` is fine but the closing directive is written as "# endif" with a space; change it to "#endif" to match the style of the other directives and keep consistent indentation for the block that includes ble_svc_gap.h (references: the `#ifdef` USING_NIMBLE_ARDUINO_HEADERS, `#else`, and the closing `#endif`).
🤖 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/NimBLEDevice.h`:
- Around line 32-35: The build never defines the macro
USING_NIMBLE_ARDUINO_HEADERS so all `#ifdef` USING_NIMBLE_ARDUINO_HEADERS include
branches (e.g., in src/NimBLEDevice.h) never trigger; add a compile definition
for this macro in the build system: update library.json to add a build.flags
entry (e.g., "-DUSING_NIMBLE_ARDUINO_HEADERS") for PlatformIO/Arduino builds,
update CMakeLists.txt to add target_compile_definitions or
add_compile_definitions when the Arduino framework is detected, and optionally
expose a Kconfig boolean (e.g., CONFIG_USING_NIMBLE_ARDUINO_HEADERS) that when
enabled adds the -D define so developers can toggle the Arduino header path
behavior.
---
Nitpick comments:
In `@src/NimBLEExtAdvertising.cpp`:
- Around line 21-25: The preprocessor block using `#ifdef`
USING_NIMBLE_ARDUINO_HEADERS / `#else` is fine but the closing directive is
written as "# endif" with a space; change it to "#endif" to match the style of
the other directives and keep consistent indentation for the block that includes
ble_svc_gap.h (references: the `#ifdef` USING_NIMBLE_ARDUINO_HEADERS, `#else`, and
the closing `#endif`).
🪄 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: 8a3ecef6-3727-45a8-99b5-af06401696aa
📒 Files selected for processing (27)
src/NimBLEAddress.hsrc/NimBLEAdvertisedDevice.hsrc/NimBLEAdvertisementData.cppsrc/NimBLEAdvertising.cppsrc/NimBLEAdvertising.hsrc/NimBLEAttValue.cppsrc/NimBLECharacteristic.cppsrc/NimBLEClient.cppsrc/NimBLEClient.hsrc/NimBLEConnInfo.hsrc/NimBLEDevice.cppsrc/NimBLEDevice.hsrc/NimBLEExtAdvertising.cppsrc/NimBLEExtAdvertising.hsrc/NimBLEL2CAPChannel.cppsrc/NimBLEL2CAPChannel.hsrc/NimBLELocalValueAttribute.hsrc/NimBLELog.hsrc/NimBLERemoteValueAttribute.hsrc/NimBLEScan.cppsrc/NimBLEScan.hsrc/NimBLEServer.cppsrc/NimBLEServer.hsrc/NimBLEStream.cppsrc/NimBLEStream.hsrc/NimBLEUUID.hsrc/NimBLEUtils.cpp
Comment on lines
+32
to
+35
| # ifdef USING_NIMBLE_ARDUINO_HEADERS | ||
| # include "nimble/nimble/host/include/host/ble_gap.h" | ||
| # else | ||
| # include <nimble/nimble/host/include/host/ble_gap.h> | ||
| # include "host/ble_gap.h" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Macro usage / definitions =="
rg -n --no-heading '\bUSING_NIMBLE_ARDUINO_HEADERS\b'
echo
echo "== Old vs new build-flag references =="
rg -n --no-heading 'CONFIG_NIMBLE_CPP_IDF|USING_NIMBLE_ARDUINO_HEADERS'
echo
echo "== Include gates for ble_gap.h after migration =="
rg -n --no-heading -C2 'ble_gap\.h|USING_NIMBLE_ARDUINO_HEADERS'Repository: h2zero/esp-nimble-cpp
Length of output: 16255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for macro definition in build config files ==="
echo
echo "Checking library.json for build flags:"
if [ -f library.json ]; then
cat -n library.json | head -30
fi
echo
echo "Checking for CMakeLists.txt files:"
find . -name "CMakeLists.txt" -type f | head -5
echo
echo "Checking platformio.ini:"
if [ -f platformio.ini ]; then
cat -n platformio.ini
fi
echo
echo "=== Searching for -DUSING_NIMBLE_ARDUINO_HEADERS ==="
rg -n 'USING_NIMBLE_ARDUINO_HEADERS' --type-list | grep -i config
rg -r '-DUSING_NIMBLE_ARDUINO_HEADERS' .
echo
echo "=== Check for Arduino framework integration ==="
fd -e 'h' -e 'hpp' -e 'cmake' | xargs rg -l 'Arduino\|ARDUINO' 2>/dev/null | head -5Repository: h2zero/esp-nimble-cpp
Length of output: 1543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full library.json content ==="
wc -l library.json
cat library.json
echo
echo "=== CMakeLists.txt for macro definitions ==="
cat CMakeLists.txt
echo
echo "=== Search for USING_NIMBLE_ARDUINO_HEADERS definition ==="
rg -n 'define.*USING_NIMBLE_ARDUINO_HEADERS|add_compile_definitions.*USING_NIMBLE_ARDUINO_HEADERS|-DUSING_NIMBLE_ARDUINO_HEADERS'Repository: h2zero/esp-nimble-cpp
Length of output: 3083
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for `#define` USING_NIMBLE_ARDUINO_HEADERS in any file ==="
rg -n '#define.*USING_NIMBLE_ARDUINO_HEADERS' --type-all
echo
echo "=== Check if it's conditionally defined in any header ==="
rg -n -A3 'ifdef.*ARDUINO|if defined.*ARDUINO' src/ | head -50
echo
echo "=== Search library.json in git history or PR changes ==="
git log --oneline -n 20 -- library.json 2>/dev/null || echo "No git history available"
echo
echo "=== Check if Arduino framework itself would define this macro ==="
echo "Searching for any references to Arduino framework macros or build system variables:"
rg -n 'ARDUINO|__PLATFORMIO__|ESP_PLATFORM' src/ | head -20Repository: h2zero/esp-nimble-cpp
Length of output: 588
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for `#define` USING_NIMBLE_ARDUINO_HEADERS ==="
rg -n '#define USING_NIMBLE_ARDUINO_HEADERS'
echo
echo "=== Check if macro is conditionally set based on build environment ==="
rg -n -B5 -A5 'USING_NIMBLE_ARDUINO_HEADERS' src/ | grep -E '(`#define`|#if|Arduino|PLATFORMIO)' | head -40
echo
echo "=== Look for build configuration that sets this macro ==="
find . -name '*.cmake' -o -name '*.py' -o -name 'setup.py' | xargs rg -l 'USING_NIMBLE_ARDUINO_HEADERS' 2>/dev/null || echo "No cmake/python build files set this macro"
echo
echo "=== Check if this should come from Arduino ESP32 framework ==="
rg -n 'framework-arduinoespressif32|arduino-esp32' CMakeLists.txt
echo
echo "=== Search for build system or preprocessor integration ==="
fd -e 'idf_component.yml' -o -e 'build.gradle' -o -e 'platformio.ini'Repository: h2zero/esp-nimble-cpp
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check PR changes for library.json or build flag additions ==="
git diff HEAD~1 library.json 2>/dev/null || echo "No git diff available, checking if library.json was modified"
echo
echo "=== Look for any build flag or compile definition changes in this PR ==="
git show --stat 2>/dev/null | head -30
echo
echo "=== Verify if USING_NIMBLE_ARDUINO_HEADERS might come from Arduino core ==="
echo "Checking for Arduino framework detection patterns:"
rg -n 'ARDUINO\b' src/ | head -20
echo
echo "=== Check if macro should be set as a compiler flag in build system ==="
echo "Looking for similar macro patterns (CONFIG_*, USING_*, etc):"
rg -n '#ifdef.*CONFIG_|#ifdef.*USING_' src/ | head -20
echo
echo "=== Summary: Is USING_NIMBLE_ARDUINO_HEADERS actually defined? ==="
if rg -q 'USING_NIMBLE_ARDUINO_HEADERS' . && ! rg -q '#define USING_NIMBLE_ARDUINO_HEADERS'; then
echo "MACRO USED: Yes (44+ locations)"
echo "MACRO DEFINED: No"
echo "RESULT: Macro is referenced but never defined in the repository"
fiRepository: h2zero/esp-nimble-cpp
Length of output: 1956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read Kconfig file for USING_NIMBLE_ARDUINO_HEADERS ==="
cat Kconfig | head -100
echo
echo "=== Search for where macro is actually DEFINED/SET in CMakeLists ==="
rg -n -A5 -B5 'add_compile_definitions|idf_component_set_property|target_compile_definitions' CMakeLists.txt
echo
echo "=== Check idf_component.yml if it exists ==="
if [ -f idf_component.yml ]; then
cat idf_component.yml
fi
echo
echo "=== Final check: Where is this macro defined in the build system? ==="
rg -n 'USING_NIMBLE_ARDUINO_HEADERS' CMakeLists.txt Kconfig 2>/dev/null || echo "Not found in CMakeLists or Kconfig"Repository: h2zero/esp-nimble-cpp
Length of output: 3650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search entire Kconfig for USING_NIMBLE_ARDUINO_HEADERS ==="
rg -n 'USING_NIMBLE_ARDUINO_HEADERS' Kconfig
echo
echo "=== Get full CMakeLists.txt and check for compile definitions ==="
wc -l CMakeLists.txt
echo "---"
cat CMakeLists.txt
echo
echo "=== Check if Arduino framework sets this macro externally ==="
echo "Searching for any mechanism that might auto-define this:"
rg -n 'idf_component_register' CMakeLists.txt -A50 | grep -i 'compile\|flag\|define'
echo
echo "=== Final diagnosis: Is USING_NIMBLE_ARDUINO_HEADERS supposed to be set? ==="
git log --all --oneline --grep="arduino" 2>/dev/null | head -10 || echo "No git history"Repository: h2zero/esp-nimble-cpp
Length of output: 127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Display complete CMakeLists.txt again with line count ==="
cat -n CMakeLists.txt | tail -50
echo
echo "=== Check if USING_NIMBLE_ARDUINO_HEADERS is set via CMake variable ==="
rg -n 'set\(.*ARDUINO\|add_compile_options.*ARDUINO' CMakeLists.txt
echo
echo "=== Comprehensive search: Where should this macro be set? ==="
echo "Checking for any preprocessor flag definition pattern:"
rg -rn '\-DUSING_NIMBLE_ARDUINO_HEADERS|\-DARDUINO' . --include='*.txt' --include='*.cmake' --include='*.json' --include='*.yml'
echo
echo "=== Conclusion: Is the macro actually defined anywhere? ==="
if ! rg -q 'USING_NIMBLE_ARDUINO_HEADERS.*=' .; then
echo "NO DEFINITION FOUND for USING_NIMBLE_ARDUINO_HEADERS"
echo "This macro is used in 44+ places but is never set in:"
echo " - CMakeLists.txt"
echo " - Kconfig"
echo " - library.json"
echo " - Any source files"
fiRepository: h2zero/esp-nimble-cpp
Length of output: 1666
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show FIRST 31 lines of CMakeLists.txt (Arduino detection) ==="
head -31 CMakeLists.txt
echo
echo "=== Look for any conditional compile definitions based on Arduino detection ==="
rg -n -A10 '__COMPONENT_TARGETS.*arduino' CMakeLists.txt
echo
echo "=== Search for idf_component_set_property or target_compile_definitions ==="
rg -n 'idf_component_set_property\|target_compile_definitions\|idf_build_set_property' . --include='*.txt' --include='*.cmake'
echo
echo "=== Check if this PR is incomplete - search commit files ==="
git show --name-only 2>/dev/null | grep -i 'cmake\|kconfig\|library' || echo "Checking file modifications..."
echo
echo "=== CRITICAL CHECK: Does any part of the code SET this macro? ==="
echo "Exact search for where macro gets defined:"
git show HEAD 2>/dev/null | grep -i 'USING_NIMBLE_ARDUINO_HEADERS' | head -20 || rg -n 'USING_NIMBLE_ARDUINO_HEADERS.*=' . --include='*.cpp' --include='*.h'Repository: h2zero/esp-nimble-cpp
Length of output: 2313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Does CMAKE actually SET the macro anywhere? ==="
rg -n 'idf_component_set_property\|target_compile_definitions'
echo
echo "=== Verify CMakeLists.txt is COMPLETE (check file size) ==="
wc -l CMakeLists.txt
tail -5 CMakeLists.txt
echo
echo "=== FINAL DIAGNOSIS: Search for USING_NIMBLE_ARDUINO_HEADERS definition ==="
echo "In CMakeLists: "
rg 'USING_NIMBLE_ARDUINO_HEADERS' CMakeLists.txt || echo " NOT FOUND"
echo "In Kconfig: "
rg 'USING_NIMBLE_ARDUINO_HEADERS' Kconfig || echo " NOT FOUND"
echo "In library.json: "
rg 'USING_NIMBLE_ARDUINO_HEADERS' library.json || echo " NOT FOUND"
echo "In ANY source file: "
rg 'USING_NIMBLE_ARDUINO_HEADERS' src/ | wc -l
echo " (matches found above)"
echo
echo "=== CONCLUSION ==="
echo "Macro USED: 44+ times in code"
echo "Macro DEFINED: 0 times in build system"
echo "RESULT: Macro is referenced but NEVER SET"Repository: h2zero/esp-nimble-cpp
Length of output: 116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Final verification: library.json for build.flags ==="
cat library.json
echo
echo "=== Does library.json have build.flags section? ==="
rg 'build.flags' library.json || echo "NO build.flags section found"
echo
echo "=== COMPREHENSIVE FINAL RESULT ==="
echo "1. Macro Usage: 44+ locations use USING_NIMBLE_ARDUINO_HEADERS"
echo "2. Macro Definition: ZERO locations define it"
echo
echo "Files that USE the macro:"
rg -l 'USING_NIMBLE_ARDUINO_HEADERS' src/ | wc -l
echo
echo "Files that DEFINE the macro:"
rg -l '#define USING_NIMBLE_ARDUINO_HEADERS' . || echo "NONE"
echo
echo "Build system files that SET it:"
rg 'USING_NIMBLE_ARDUINO_HEADERS' CMakeLists.txt Kconfig library.json 2>/dev/null || echo "NONE FOUND"Repository: h2zero/esp-nimble-cpp
Length of output: 943
Arrr, the USING_NIMBLE_ARDUINO_HEADERS macro be used in 44+ places but ne'er actually defined in the build system, ye scallywag!
The macro be referenced across 27 source/header files in conditional include gates (lines like 32-35 in src/NimBLEDevice.h), but there be no mechanism to set it:
library.jsonlacks abuild.flagssection to define-DUSING_NIMBLE_ARDUINO_HEADERSCMakeLists.txtdetects Arduino frameworks but don't add any compile definitionsKconfigcontains no flag fer this macro
Without this macro being defined, all #ifdef USING_NIMBLE_ARDUINO_HEADERS blocks will always evaluate false, causin' the code to always use the fallback host/ble_gap.h paths instead o' the Arduino header layouts. The commit message says "Add a flag to use arduino header paths" but the flag was ne'er added to the build configuration. Set this macro explicitly in the build system so Arduino-oriented PlatformIO builds actually use the correct header paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NimBLEDevice.h` around lines 32 - 35, The build never defines the macro
USING_NIMBLE_ARDUINO_HEADERS so all `#ifdef` USING_NIMBLE_ARDUINO_HEADERS include
branches (e.g., in src/NimBLEDevice.h) never trigger; add a compile definition
for this macro in the build system: update library.json to add a build.flags
entry (e.g., "-DUSING_NIMBLE_ARDUINO_HEADERS") for PlatformIO/Arduino builds,
update CMakeLists.txt to add target_compile_definitions or
add_compile_definitions when the Arduino framework is detected, and optionally
expose a Kconfig boolean (e.g., CONFIG_USING_NIMBLE_ARDUINO_HEADERS) that when
enabled adds the -D define so developers can toggle the Arduino header path
behavior.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/NimBLEDevice.h (1)
32-35:⚠️ Potential issue | 🔴 CriticalBlocker: ye switched the gate, but the new macro still be unhooked in build config.
Line 32 now depends on
USING_NIMBLE_ARDUINO_HEADERS, but the providedCMakeLists.txtsnippet does not define it. That leaves Line 35’s fallback include active, so the missing-header failure this PR targets can still persist.🏴☠️ Proposed fix (in
CMakeLists.txt)idf_component_register( REQUIRED_IDF_TARGETS "esp32" @@ PRIV_REQUIRES ${ESP_NIMBLE_PRIV_REQUIRES} ) + +# Enable Arduino-oriented NimBLE include layout when Arduino framework component is present. +if(__COMPONENT_TARGETS MATCHES "___idf_espressif__arduino-esp32|__idf_arduino|___idf_framework-arduinoespressif32") + target_compile_definitions(${COMPONENT_LIB} PUBLIC USING_NIMBLE_ARDUINO_HEADERS=1) +endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEDevice.h` around lines 32 - 35, NimBLEDevice.h now conditions on the macro USING_NIMBLE_ARDUINO_HEADERS but your build doesn't define it, so the fallback include stays active; update the build config to define USING_NIMBLE_ARDUINO_HEADERS for the target that builds this header (e.g., add a target_compile_definitions or equivalent definition for the library/executable target) so the branch including "nimble/nimble/host/include/host/ble_gap.h" is used; ensure the definition is applied to the same target that compiles sources including NimBLEDevice.h and verify the preprocessor symbol name exactly matches USING_NIMBLE_ARDUINO_HEADERS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/NimBLEDevice.h`:
- Around line 32-35: NimBLEDevice.h now conditions on the macro
USING_NIMBLE_ARDUINO_HEADERS but your build doesn't define it, so the fallback
include stays active; update the build config to define
USING_NIMBLE_ARDUINO_HEADERS for the target that builds this header (e.g., add a
target_compile_definitions or equivalent definition for the library/executable
target) so the branch including "nimble/nimble/host/include/host/ble_gap.h" is
used; ensure the definition is applied to the same target that compiles sources
including NimBLEDevice.h and verify the preprocessor symbol name exactly matches
USING_NIMBLE_ARDUINO_HEADERS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f712f8ca-c97f-4de3-9d1f-99c5291f7566
📒 Files selected for processing (28)
Kconfigsrc/NimBLEAddress.hsrc/NimBLEAdvertisedDevice.hsrc/NimBLEAdvertisementData.cppsrc/NimBLEAdvertising.cppsrc/NimBLEAdvertising.hsrc/NimBLEAttValue.cppsrc/NimBLECharacteristic.cppsrc/NimBLEClient.cppsrc/NimBLEClient.hsrc/NimBLEConnInfo.hsrc/NimBLEDevice.cppsrc/NimBLEDevice.hsrc/NimBLEExtAdvertising.cppsrc/NimBLEExtAdvertising.hsrc/NimBLEL2CAPChannel.cppsrc/NimBLEL2CAPChannel.hsrc/NimBLELocalValueAttribute.hsrc/NimBLELog.hsrc/NimBLERemoteValueAttribute.hsrc/NimBLEScan.cppsrc/NimBLEScan.hsrc/NimBLEServer.cppsrc/NimBLEServer.hsrc/NimBLEStream.cppsrc/NimBLEStream.hsrc/NimBLEUUID.hsrc/NimBLEUtils.cpp
💤 Files with no reviewable changes (1)
- Kconfig
✅ Files skipped from review due to trivial changes (11)
- src/NimBLEL2CAPChannel.cpp
- src/NimBLEServer.h
- src/NimBLEExtAdvertising.h
- src/NimBLELocalValueAttribute.h
- src/NimBLEClient.h
- src/NimBLEAdvertising.h
- src/NimBLEAdvertisedDevice.h
- src/NimBLEStream.h
- src/NimBLEServer.cpp
- src/NimBLEClient.cpp
- src/NimBLEUUID.h
🚧 Files skipped from review as they are similar to previous changes (8)
- src/NimBLEUtils.cpp
- src/NimBLECharacteristic.cpp
- src/NimBLEAdvertisementData.cpp
- src/NimBLEStream.cpp
- src/NimBLELog.h
- src/NimBLEDevice.cpp
- src/NimBLEAddress.h
- src/NimBLEL2CAPChannel.h
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NimBLEDevice.cpp (1)
1028-1035:⚠️ Potential issue | 🔴 CriticalFix asymmetry in deinit path — Arduino builds leak controller resources!
Avast! The deinit path be NOT mirroring the init logic, matey. Thar be a resource leak lurkin' in these waters!
On init (line 914), the guard be:
# if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0) || defined(USING_NIMBLE_ARDUINO_HEADERS)This means: execute if (IDF < 5.0.0) OR usin' Arduino headers.
On deinit (lines 1028-1029), the guard be:
# ifndef USING_NIMBLE_ARDUINO_HEADERS # if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)This means: execute if NOT usin' Arduino AND IDF < 5.0.0.
When USING_NIMBLE_ARDUINO_HEADERS be defined with IDF < 5.0.0:
- Init: Executes
esp_bt_controller_init()✓- Deinit: Skips
esp_nimble_hci_and_controller_deinit()✗That be a resource leak fer Arduino builds! The controller be initialized but never properly deinitialized.
Change the deinit guard to match init:
# if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0) || defined(USING_NIMBLE_ARDUINO_HEADERS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEDevice.cpp` around lines 1028 - 1035, The deinit preprocessor guards around esp_nimble_hci_and_controller_deinit() are asymmetric with the init guards used around esp_bt_controller_init(), causing Arduino builds to skip controller deinitialization and leak resources; modify the deinit guards so they match the init condition (i.e., run the deinit when ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5,0,0) OR USING_NIMBLE_ARDUINO_HEADERS is defined) so esp_nimble_hci_and_controller_deinit() is invoked in the same cases where esp_bt_controller_init() was called.
🧹 Nitpick comments (1)
src/NimBLEStream.cpp (1)
23-29: Arr, this flag flip be shipshape and seaworthy!The switch from
CONFIG_NIMBLE_CPP_IDFtoUSING_NIMBLE_ARDUINO_HEADERSbe a clever refactorin', matey! Rather than inferrin' Arduino paths by the absence of the IDF flag, ye now explicitly check fer Arduino headers—a cleaner sail into the harbor. This pattern flies consistent across the entire fleet of source files (27+ sightings from bow to stern), showin' this be no rogue flag.The
USING_NIMBLE_ARDUINO_HEADERSmacro be injected by the PlatformIO framework build system, not explicitly defined inlibrary.jsonorCMakeLists.txt, which be the intended design fer this build environment. The header paths be correctly hoisted fer all ports through this mechanism.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEStream.cpp` around lines 23 - 29, The conditional include in NimBLEStream.cpp should use the new macro consistently across the codebase: replace any remaining checks for CONFIG_NIMBLE_CPP_IDF with USING_NIMBLE_ARDUINO_HEADERS so all files (including NimBLEStream.cpp) detect Arduino header paths the same way; verify the build system (PlatformIO) injects USING_NIMBLE_ARDUINO_HEADERS (no change to CMakeLists.txt/library.json) and update any files referencing os_mbuf.h or nimble_port.h to follow the same `#ifdef` USING_NIMBLE_ARDUINO_HEADERS pattern to avoid divergent include paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/NimBLEDevice.cpp`:
- Around line 1028-1035: The deinit preprocessor guards around
esp_nimble_hci_and_controller_deinit() are asymmetric with the init guards used
around esp_bt_controller_init(), causing Arduino builds to skip controller
deinitialization and leak resources; modify the deinit guards so they match the
init condition (i.e., run the deinit when ESP_IDF_VERSION <
ESP_IDF_VERSION_VAL(5,0,0) OR USING_NIMBLE_ARDUINO_HEADERS is defined) so
esp_nimble_hci_and_controller_deinit() is invoked in the same cases where
esp_bt_controller_init() was called.
---
Nitpick comments:
In `@src/NimBLEStream.cpp`:
- Around line 23-29: The conditional include in NimBLEStream.cpp should use the
new macro consistently across the codebase: replace any remaining checks for
CONFIG_NIMBLE_CPP_IDF with USING_NIMBLE_ARDUINO_HEADERS so all files (including
NimBLEStream.cpp) detect Arduino header paths the same way; verify the build
system (PlatformIO) injects USING_NIMBLE_ARDUINO_HEADERS (no change to
CMakeLists.txt/library.json) and update any files referencing os_mbuf.h or
nimble_port.h to follow the same `#ifdef` USING_NIMBLE_ARDUINO_HEADERS pattern to
avoid divergent include paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37ddafd4-d30e-4c8e-82fb-a501ac913a6d
📒 Files selected for processing (28)
Kconfigsrc/NimBLEAddress.hsrc/NimBLEAdvertisedDevice.hsrc/NimBLEAdvertisementData.cppsrc/NimBLEAdvertising.cppsrc/NimBLEAdvertising.hsrc/NimBLEAttValue.cppsrc/NimBLECharacteristic.cppsrc/NimBLEClient.cppsrc/NimBLEClient.hsrc/NimBLEConnInfo.hsrc/NimBLEDevice.cppsrc/NimBLEDevice.hsrc/NimBLEExtAdvertising.cppsrc/NimBLEExtAdvertising.hsrc/NimBLEL2CAPChannel.cppsrc/NimBLEL2CAPChannel.hsrc/NimBLELocalValueAttribute.hsrc/NimBLELog.hsrc/NimBLERemoteValueAttribute.hsrc/NimBLEScan.cppsrc/NimBLEScan.hsrc/NimBLEServer.cppsrc/NimBLEServer.hsrc/NimBLEStream.cppsrc/NimBLEStream.hsrc/NimBLEUUID.hsrc/NimBLEUtils.cpp
💤 Files with no reviewable changes (1)
- Kconfig
✅ Files skipped from review due to trivial changes (6)
- src/NimBLEScan.h
- src/NimBLEDevice.h
- src/NimBLEAdvertisementData.cpp
- src/NimBLEScan.cpp
- src/NimBLEAttValue.cpp
- src/NimBLEL2CAPChannel.h
🚧 Files skipped from review as they are similar to previous changes (11)
- src/NimBLEClient.h
- src/NimBLEServer.h
- src/NimBLEExtAdvertising.h
- src/NimBLEAddress.h
- src/NimBLEUtils.cpp
- src/NimBLEStream.h
- src/NimBLELog.h
- src/NimBLEUUID.h
- src/NimBLEServer.cpp
- src/NimBLECharacteristic.cpp
- src/NimBLEClient.cpp
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.
Fixes #407
Summary by CodeRabbit
Refactor
Chores