-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix bt rpc gatt deinit #25879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix bt rpc gatt deinit #25879
Conversation
…oved fixed memory leak on multiple bt init/disable calls. Signed-off-by: Robert Gałat <robert.galat@nordicsemi.no>
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 0cd7729dafed8671839af522bf77c9d1ca978371 more detailssdk-nrf:
Github labels
List of changed files detected by CI (9)Outputs:ToolchainVersion: 43683a87ea Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
98212f4 to
bf9e1c0
Compare
7ab29c1 to
81962e6
Compare
|
|
||
| FILE(GLOB app_sources src/*.c) | ||
|
|
||
| # Remove log_stub.c from glob since we include it explicitly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we exclude it to include it later? What's the goal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this does not make sense, removed it.
| # Remove log_stub.c from glob since we include it explicitly | ||
| list(FILTER app_sources EXCLUDE REGEX ".*log_stub\\.c$") | ||
|
|
||
| # Add src directory for test_config.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's included by an absolute path anyway, so this is not really needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment was misleeding, but I still need to include the /src dir.
|
|
||
| # Minimal BT configuration - just enough to use the real gatt.h header | ||
| # We don't need full BT stack functionality, just the type definitions | ||
| CONFIG_BT=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gatt.h is in the public include directory, so you don't really need CONFIG_BT to be able to include <zephyr/bluetooth/gatt.h> do you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is true, removed unnecessary config
| #include <stddef.h> | ||
| #include <stdint.h> | ||
|
|
||
| /* Include the header - it will use our stub zephyr/bluetooth/gatt.h */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no stub gatt.h, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in deed there is none. it seems like the comment is a leftover from previous iteration of the test implementation. removed it.
| /* Forward declaration - full definition not needed for unit testing */ | ||
| struct nrf_rpc_cbor_ctx; | ||
|
|
||
| /* Typedef for compatibility */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original nRF RPC doesn't use typedef so there's no point in adding it in stub either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is true, removed it.
Memory footprint analysis revealed the following potential issuesapplications.nrf_desktop.zdebug_nrf21540ek_multicore[nrf5340dk/nrf5340/cpuapp]: ROM size increased by 784[B] in comparison to the main[29bfe34] branch. - link (cc: @nrfconnect/ncs-si-bluebagel) Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-25879/7) |
a453ff8 to
0cd7729
Compare
check if gatt services are correctly clean after remove Signed-off-by: Robert Gałat <robert.galat@nordicsemi.no>
No description provided.