-
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
boards/common/nrf52: add openocd support for 'nordic_softdevice_ble' #11470
Conversation
I just give a quick try on this one and I'm having issues: if the board is previously flashed using OpenOCD with a firmware that doesn't contain SoftDevice, flashing gnrc_networking with SoftDevice using OpenOCD results in the application being broken. Then if I flash gnrc_networking using JLink, it works. If I flash again using OpenOCD, it still works. There is something weird. |
@aabadie do you have a reproducible failing case ? |
Yes, use nrf52d, which loads softdevice by default with gnrc_networking and do the following, in same order:
It seems that flashing SoftDevice with JLink puts the application back in a working state. Maybe adding erase with openocd is necessary ? |
Thank you, from my observations it was "not necessary" as said in
But I did not have a failing case to understand why it would be needed. |
When comparing the output from Instead of relying on erasing during flash, I updated to use a |
Now the |
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.
Tested and confirmed gnrc_networking is working (shell available, ifconfig returns a configured interface) when flashing with both jlink and openocd.
The python test script also works as expected but can be improved.
|
||
import sys | ||
from testrunner import run | ||
|
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 an extra blank line here, 2 are enough ;)
|
||
|
||
|
||
def _check_shell_is_working(child, tries=5): |
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.
No need for loops, just do the following:
def _check_shell_is_working(child, tries=5): | |
def _check_shell_is_working(child): | |
"""Just check that the shell is working. | |
It ensures the application did not crash. | |
""" | |
child.expect_exact('All up, running the shell now') | |
child.sendline('help') | |
child.expect_exact('Command Description', timeout=1) | |
child.expect_exact('---------------------------------------', timeout=1) | |
child.expect_exact('> ', timeout=1) |
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.
No it failed for some executions on my local board. It failed when running the test_nordic_softdevice_openocd.sh
.
Welcome to pyterm!
Type '/exit' to exit.
2019-05-27 16:23:11,315 - INFO # main(): This is RIOT! (Version: buildtest)
2019-05-27 16:23:11,316 - INFO # RIOT network stack example application
2019-05-27 16:23:11,316 - INFO # All up, running the shell now
> help
2019-05-27 16:23:13,581 - INFO # main(): This is RIOT! (Version: buildtest)
2019-05-27 16:23:13,584 - INFO # RIOT network stack example application
2019-05-27 16:23:13,587 - INFO # All up, running the shell now
> Timeout in expect script at "child.expect_exact('Command Description', timeout=1)" (examples/gnrc_networking/tests/01-run.py:20)
I should also maybe increase the timeout
.
I would split this in a dedicated pull request anyway.
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.
I just rechecked the proposed change using test_nordic_softdevice_openocd.sh
and it works like a charm. Maybe you missed the synchronization child.expect_exact('All up, running the shell now')
, at line 18 ?
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.
diff --git a/examples/gnrc_networking/tests/01-run.py b/examples/gnrc_networking/tests/01-run.py
index bd778020d..baf937c7e 100755
--- a/examples/gnrc_networking/tests/01-run.py
+++ b/examples/gnrc_networking/tests/01-run.py
@@ -15,18 +15,12 @@ def _check_shell_is_working(child, tries=5):
It ensures the application did not crash.
"""
- for i in range(0, tries):
- child.sendline('help')
- try:
- child.expect_exact('Command Description', timeout=1)
- child.expect_exact('---------------------------------------',
- timeout=1)
- child.expect_exact('> ', timeout=1)
- break
- except Exception:
- pass
- else:
- child.expect('>')
+ child.expect_exact('All up, running the shell now')
+ child.sendline('help')
+ child.expect_exact('Command Description', timeout=1)
+ child.expect_exact('---------------------------------------',
+ timeout=1)
+ child.expect_exact('> ', timeout=1)
def testfunc(child):
I found in #5863 why the I do a PR to use the modified |
The pull request for changing the |
ab2dcda
to
50426b6
Compare
Rebased on top of #11620 |
09193b9
to
fb4c927
Compare
I rebased this one to remove all the test commits as the not resetting behavior was moved to #11620 . |
fb4c927
to
a970bf5
Compare
@aabadie do you still have concerns? |
Enable the handling of flashing `softdevice.hex` when flashing the firmware for openocd. However, for flashing, only the `hexfile` and `binfile` can currently be used. The `elffile` is generated with local pages aligned to `0x10000` which makes the program starting at `0x1f000` be flashed from `0x10000` with padding bytes even if the `.text` section is indeed at `0x1f000`: readelf --sections bin/nrf52dk/gnrc_networking.elf ... [ 1] .text PROGBITS 0001f000 00f000 00f698 00 AX 0 0 16 ... readelf --segments bin/nrf52dk/gnrc_networking.elf ... LOAD 0x000000 0x00010000 0x00010000 0x1e6a0 0x1e6a0 R E 0x10000 ... The padding bytes would go through `verify_image` in `openocd` so be expected to not be overwritten but are by `softdevice.hex` Using --nmagic at link time removes the local page alignement but would need dedicated testing.
a970bf5
to
fe0b829
Compare
Rebased now that #11620 is merged. I did not cleaned that now the |
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.
With all related PRs merged, I retested flashing with OpenOCD and it now works like a charm.
I have one question regarding Murdock integration though.
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.
Let's go with this PR. ACK
Thank you for the review and for having added the openocd support in the first place :) |
I noticed one issue thank to this one. But it is not specific to this example but found out about it with this. When building with I will add |
Fix provided in #12207 |
Contribution description
Enable the handling of flashing
softdevice.hex
when flashing the firmwarefor openocd.
However, for flashing, only the
hexfile
andbinfile
can currently be used.The
elffile
is generated with local pages aligned to0x10000
which makesthe program starting at
0x1f000
be flashed from0x10000
with padding byteseven if the
.text
section is indeed at0x1f000
:The padding bytes would go through
verify_image
inopenocd
so be expectedto not be overwritten but are by
softdevice.hex
Using --nmagic at link time removes the local page alignement but would
need dedicated testing.
Testing procedure
It can be tested with
gnrc_networking
andtests/nordic_softdevice_ble
.Murdock
test execution output is not showing it is working as it does not useopenocd
. It would only show this did not break anything (even if the real changes are only in theopenocd
branch).hwrng issue fixed
Previous memory comparison that shown issues and that is now handled in #11620
Another testing procedure will be showing memory comparison with what Jlink flashes.
I added a test file to generate all the memory files to compare.
Prequisite info
softdevice.hex
is not full, bytes in[0x8bc, 0x3000[
are empty and the file is0x1eb58
bytes long.So any change in unused bytes in the output is not important.
Comparison of the output of the objcopy with
--gap-fill
Output comparison
In the openocd case, I did not erase the memory as done for Jlink as I think it is not necessary.
Comparison with the
erased
versionComparison with setting the memory to
0x00
All the changes are between
0x1000
and0x2000
so can be ignored.Comparison with flashing the memory from a previous firmware without setting memory
Here too, the difference is only between
0x1000
and0x2000
so within unused range.I provide the full output anyway if anybody is interested for reviewing without executing it themself.
diff -u jlink_flashed.hex openocd_dirty.hex
Issues/PRs references
I worked on this in the context of #10870 as my
nrf52dk
does notreset
anymore withJlink
but does withopenocd
.