pkg/nimble: version bump to NimBLE 1.5.0 RC1 - #18029
Conversation
haukepetersen
added
State: waiting for other PR
github-actions
Bot
added
Area: build system
fjmolinas
left a comment
fjmolinas
left a comment
There was a problem hiding this comment.
Nice this at least still compiles with mynewt-core as well... will test it out once dependencies are in... I trust you will test the BLE side thoroughly... one question on the config files though, where do they come from?
| #if defined(CPU_FAM_NRF51) | ||
| #include <nrfx_config_nrf51.h> | ||
| #elif defined(NRF52805_XXAA) | ||
| #include <nrfx_config_nrf52805.h> | ||
| #elif defined(NRF52810_XXAA) | ||
| #include <nrfx_config_nrf52810.h> | ||
| #elif defined(NRF52811_XXAA) | ||
| #include <nrfx_config_nrf52811.h> | ||
| #elif defined(NRF52820_XXAA) | ||
| #include <nrfx_config_nrf52820.h> | ||
| #elif defined(CPU_MODEL_NRF52832XXAA) || defined (CPU_MODEL_NRF52832_XXAB) | ||
| #include <nrfx_config_nrf52832.h> | ||
| #elif defined(NRF52833_XXAA) | ||
| #include <nrfx_config_nrf52833.h> | ||
| #elif defined(NRF52840_XXAA) | ||
| #include <nrfx_config_nrf52840.h> | ||
| #elif defined(NRF5340_XXAA_APPLICATION) | ||
| #include <nrfx_config_nrf5340_application.h> | ||
| #elif defined(NRF5340_XXAA_NETWORK) | ||
| #include <nrfx_config_nrf5340_network.h> | ||
| #elif defined(NRF9160_XXAA) | ||
| #include <nrfx_config_nrf9160.h> | ||
| #else | ||
| #error "Unknown device." | ||
| #endif |
There was a problem hiding this comment.
Are these not all part of the nrfx package? Where do they come from? Could we use them similarly as it's done for stm32?
|
The nrfx headers in |
|
Did look at those imported config headers a bit closer: turns out in our current setup where we only use headers from the nrfx package we do not need most of them, so I removed them again from the package and just left the needed essentials. However, I just discovered that NimBLE is not compiling anymore for any nrf51-based board. There seem to be some define clashes in the nrf51 configuration which I can't figure out how to solve so far. Will need a little more digging I suppose. But the nrf52 and the nrf52840 both seem to work as expected when running some NimBLE examples in RIOT. |
fjmolinas
left a comment
fjmolinas
left a comment
There was a problem hiding this comment.
Nice to see the amount of headers reduced did you modify the ones that are still here? Can we copy them from the package in the makefile?
Can you show the logs? |
The
Of course: DUMPI see two major problems:
|
|
Finally, fixed the |
IMO this seems like a more maintainable approach, and avoid including vendor files in the tree
With the current CI times I'm fine with having it here. |
|
Just did some integration test using |
|
Regarding the I don't know what the issue is on the NimBLE part that it does not work. Depending on how much time you want to spend on this I would suggest:
|
|
If you agree with the suggested approach (at least the first two points), then you can go ahead and squash IMO, would be nice if you ran your benchmarks as well. |
5f5dea3 to
842698e
Compare
|
Sorry for the delay... Actually the Did update it now and I sucessfully build (without However, if I build and flash the same application using So there is actually rather a problem using |
842698e to
4d77265
Compare
|
updated the nimble commit hash once more (after squashing the Nimble upstream PR) and reabsed |
|
On my side this will have to wait for next week, I'm sadly on a paper deadline :( |
|
Actually there is something missing, can you apply this patch and squash: diff --git a/pkg/Kconfig b/pkg/Kconfig
index f6bd78aad4..8c0a894325 100644
--- a/pkg/Kconfig
+++ b/pkg/Kconfig
@@ -54,6 +54,7 @@ rsource "mynewt-core/Kconfig"
rsource "nanocbor/Kconfig"
rsource "nanopb/Kconfig"
rsource "nanors/Kconfig"
+rsource "nrfx/Kconfig"
rsource "qcbor/Kconfig"
rsource "qdsa/Kconfig"
rsource "qr-code-generator/Kconfig"
diff --git a/pkg/mynewt-core/Kconfig b/pkg/mynewt-core/Kconfig
index 11fc88a87c..53e822bca2 100644
--- a/pkg/mynewt-core/Kconfig
+++ b/pkg/mynewt-core/Kconfig
@@ -37,5 +37,6 @@ config MODULE_MYNEWT-CORE_UTIL
config MODULE_MYNEWT-CORE_NRF5X_HAL
bool "mynewt-core nrf52 and nrf51 timer hal"
+ select PACKAGE_NRFX
endif # PACKAGE_MYNEWT-CORE
diff --git a/pkg/mynewt-core/Makefile.dep b/pkg/mynewt-core/Makefile.dep
index 8131c03595..9555b3d619 100644
--- a/pkg/mynewt-core/Makefile.dep
+++ b/pkg/mynewt-core/Makefile.dep
@@ -5,5 +5,9 @@ USEMODULE += mynewt-core
DEFAULT_MODULE += auto_init_mynewt-core
+ifneq (,$(filter mynewt-core_nrf5x_hal,$(USEMODULE)))
+ USEPKG += nrfx
+endif
+
# esp frequency is not pow2 so is incompatible with os_cputime
FEATURES_BLACKLIST += arch_esp |
|
Pushed the patch |
|
thanks! There is one other issue I am looking into: Murdock is not building some examples for the nrf51 due to not enough RAM. Seems like the they trip just very lightly over the RAM limit... But with the help of the mynewt devs I think I found an optimization that should give us 1-2kb of free RAM and fix those issues for good. I in the progress of testing them. |
Should we leave it as a follow-up? If not please stop the CI from running before those optimizations. |
|
Done. Finished the testing and everything works as expected, also did some backward testing with nrf52 boards to make sure extended advertisings are too working as they should. Hopefully all Murdock issues should be resolved now. |
|
ACK still stands |
These boards have insufficient RAM to build this test.
|
The |
maribu
added
CI: ready for build
benpicco
added
CI: ready for build
Done: apache/mynewt-nimble#1272 Will do another version update after this PR is in. |
Awesome! |
benpicco
added
CI: ready for build
|
Good to see this in! |
@haukepetersen While testing PR #18439 I noticed that since this PR all |
|
@haukepetersen I figured out the problem. If Advertising Extension is used, the event queue before they are processed. With only 2 elements in that queue, the I will provide a fix which increases the queue size if |
|
PR #18467 fixes the problem. |
6 participants
Contribution description
NimBLE is about to be released in version 1.5.0. In order to update the NimBLE version in RIOT, too, there are some changes needed to the NimBLE port to cater for internal changes in NimBLE. These are most notably the use of Nordic
nrfxheaders in the NimBLE radio drivers and an overhaul of the BLE transport implementation in NimBLE.This PR contains all the needed changes to RIOT to allow for updating NimBLE to its newest version:
nrfxheaders as optional package to RIOTTesting procedure
gnrc_networkingon some NRF hardwareIssues/PRs references
Depends on apache/mynewt-nimble#1249