-
Notifications
You must be signed in to change notification settings - Fork 2k
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
pkg/nimble: version bump to NimBLE 1.5.0 RC1 #18029
pkg/nimble: version bump to NimBLE 1.5.0 RC1 #18029
Conversation
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.
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?
pkg/nrfx/include/nrfx_config.h
Outdated
#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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
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: DUMP
I 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 |
Done: apache/mynewt-nimble#1272 Will do another version update after this PR is in. |
Awesome! |
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. |
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
nrfx
headers 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:
nrfx
headers as optional package to RIOTTesting procedure
gnrc_networking
on some NRF hardwareIssues/PRs references
Depends on apache/mynewt-nimble#1249