pkg/nimble: migrate to ztimer #16317
Merged
Conversation
This file contains 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
haukepetersen
added
Type: enhancement
haukepetersen
requested review from
aabadie,
fjmolinas,
leandrolanzieri,
MichelRottleuthner and
smlng
as code owners
fjmolinas
reviewed
Apr 12, 2021
| @@ -32,6 +32,7 @@ | |||
| #include "thread.h" | |||
| #include "thread_flags.h" | |||
| #include "net/bluetil/ad.h" | |||
| #include "xtimer.h" | |||
There was a problem hiding this comment.
Seems like a shame to keep xtimer here... but I understand that this is unrelated
There was a problem hiding this comment.
Exactly. Also for this use case it does not really matter, as energy consumption is not really a the focus of this test :-)
This was referenced Apr 13, 2021
haukepetersen
force-pushed
the
opt_nimble_ztimer
branch
from
591d107 to
398c386
Compare
|
Removed the Although apache/mynewt-nimble#967 is ready to be merged, we have the problem that merging it prior to this PR it will make NimBLEs CI fail, as the RIOT build will not succeed. So my strategy is the following:
|
examples/gnrc-networking OK
examples/nimble_gatt OKCan connect on phone as well |
|
With |
I can't reliably reproduce yet... |
tests/l2cap_% OK
|
Makes sense, @kaspar030 is this OK to merge in soft feature freeze? |
I seem to be getting it quite reliably after a fresh flash. |
I have not notices this before, but I will try some boards to see if I can reproduce this. |
|
Seems like there was an issue with the shell command still using xtimer. I simply pulled in the commit from #16324 that migrates the nimble_netif shell command to use |
|
fjmolinas
reviewed
Apr 14, 2021
pkg/nimble/Makefile
Outdated
| @@ -1,6 +1,6 @@ | |||
| PKG_NAME = nimble | |||
| PKG_URL = https://github.com/apache/mynewt-nimble.git | |||
There was a problem hiding this comment.
If we are temporarily switching the upstream needs to be changed
There was a problem hiding this comment.
I don't think so, as the fork is PRed, it seems like git is fine in finding the correct branch by the provided hash
There was a problem hiding this comment.
Not for me:
make -C examples/nimble_scanner/ distclean clean flash term
rm -rf /home/francisco/workspace/RIOT/examples/nimble_scanner/bin/nrf52dk/pkg-build/nimble
rm -rf /home/francisco/workspace/RIOT/build/pkg/nimble
rm -rf /home/francisco/workspace/RIOT/examples/nimble_scanner/bin/nrf52dk/pkg-build/nimble
Building application "nimble_scanner" for "nrf52dk" with MCU "nrf52".
[INFO] cloning nimble
Cloning into '/home/francisco/workspace/RIOT/build/pkg/nimble'...
remote: Enumerating objects: 140, done.
remote: Counting objects: 100% (140/140), done.
remote: Compressing objects: 100% (105/105), done.
remote: Total 41218 (delta 54), reused 66 (delta 29), pack-reused 41078
Receiving objects: 100% (41218/41218), 12.28 MiB | 10.42 MiB/s, done.
Resolving deltas: 100% (25153/25153), done.
fatal: reference is not a tree: 7a2bf2420a1a2bcd8bbb5bf8f5927882b3c499c0
make[1]: *** [/home/francisco/workspace/RIOT/pkg/pkg.mk:123: /home/francisco/workspace/RIOT/build/pkg/nimble/.git] Error 128
make: [/home/francisco/workspace/RIOT/examples/nimble_scanner/../../Makefile.include:696: pkg-prepare] Error 2 (ignored)
There was a problem hiding this comment.
This still needs addressing.
There was a problem hiding this comment.
Once this is addressed please trigger the ci.
I'm still getting it: |
|
Yes, just noticed this myself. Also I noticed, that the newly added commit is not even related to this example application, as the nimble_netif shell command is not used here... Will dig a little deeper. |
|
Havn't found the reason, but it seems some changes in RIOT could be the cause: A malloc problem would also explain, why the hard-fault is only visible right after system reboot. Will keep digging. |
|
Findings: |
If this is reproducible in master, then if an issue is opened I won't consider it a blocker for this PR. |
|
It is :-) I will open the issue so we can progress with the ztimer migration for NimBLE. Now after 2 hours of debugging I did not close in on the malloc issue, so it seems to be something more complicated... |
But it seems it was introduced after #15606 since I can't seem to reproduce from that commit. |
|
I had the same idea about this just minutes ago :-) In the middle of bisecting it right as we speak. |
|
Took a while but I think I found the issue:
So much for the observed hard fault, now towards verifying and fixing it :-) |
nice catch! |
Nice catch indeed. |
|
Fix is on its way :-) Will propose something to NimBLE upstream so the fix is not tied to the RIOT specific initialization... |
Yep, offline @kaspar030 mentioned he is OK with merging during soft-feature freeze. So let's go ahead! |
|
Fix is PRed, lets see if my approach is accepted... @fjmolinas Awesome, I just need to remove the last commit and this PR should be good to go, right?! |
haukepetersen
force-pushed
the
opt_nimble_ztimer
branch
from
22d6444 to
0d1c09c
Compare
haukepetersen
added
the
CI: ready for build
haukepetersen
force-pushed
the
opt_nimble_ztimer
branch
from
0d1c09c to
9bd65ef
Compare
|
@fjmolinas you might be right with the upstream nimble repo: seems Murdocks git cache can't find the needed commit... So for now I changed the upstream repo to link directly to my nimble fork. |
|
Also included the ztimer migration for the |
|
Alright, Murdock is happy now. @fjmolinas do you want to give this one more try/final review? Just verified that gnrc_networking (ping between two nodes) and the GATT example are still working as expected, besides the known hard-fault on scan issue that is... |
Other then the |
|
All good |
|
Very nice, thanks! |
This was referenced Apr 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Contribution description
With #15782 merged all prerequisites for migrating NimBLE from
xtimertoztimerare in. For a next step, I PRed the required NimBLE NPL changes to NimBLE (apache/mynewt-nimble#967). These are needed in conjunction with the changes of this PR and finally allow forxtimer-free RIOT/NimBLE builds :-)As stated in apache/mynewt-nimble#967, appart from simplifying the NPL code in NImBLE, this change allows for significant energy savings. E.g. when running the
examples/nimble_gattapplication without STDIO (USEMODULE+=stdio_null) and the node is in connected state with a peer, the average current consumption drops from ~420µA to 72µA.Testing procedure
Verify that NimBLE is behaving as before. I suggest to run at least the following on any nrf52x board of you choice:
examples/nimble_gatt-> connect with a smartphone (e.g. using thenrf connectAPP on Android) and verify GATT services are workingexamples/nimble_scanner-> use thescanshell command and it should list some devices that it foundtests/nimble_l2cap/tests/nimble_l2cap_server-> connect two boards and make sure flooding and inctest are running as expectedexamples/gnrc_networking-> connect two IP-over-BLE devices using thebleshell command and make sure they can ping each otherIssues/PRs references
depends on apache/mynewt-nimble#967