Skip to content
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/qn908x: Compute the image checksum #15546

Merged
merged 1 commit into from Dec 3, 2020

Conversation

iosabi
Copy link
Contributor

@iosabi iosabi commented Dec 2, 2020

Contribution description

QN908X CPUs require the image to have a valid checksum. The checksum is
a simple addition of the first 7 uint32_t values stored in the 8th
position of the image header. This position is a reseved entry of the
Cortex-M Vector Table and its value depends on other fields that are
computed at link time. Performing this checksum at link time seems
hard to do, so instead this patch uses a python script to patch the
checksum from the ELF file. This redefines the value of FLASHFILE
to the new .elf file with the checksum fixed.

With this patch, OpenOCD can program and verify QN908X images since
now they have a valid checksum value.

Testing procedure

make BOARD=qn9080dk flash

Issues/PRs references

This is part of the FR #13852 .

@benpicco benpicco added Area: boards Area: Board ports Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Dec 2, 2020
@benpicco benpicco requested a review from aabadie December 2, 2020 15:56
@aabadie
Copy link
Contributor

aabadie commented Dec 2, 2020

Does this means that the port of qn9080dk as it is merged now in master cannot be flashed ?
I would be very interested by the results of the following command (be careful, it takes a lot of a time):

./dist/tools/compile_and_test_for_board/compile_and_test_for_board.py . qn9080dk --jobs=4 --with-test-only

@iosabi
Copy link
Contributor Author

iosabi commented Dec 2, 2020

Does this means that the port of qn9080dk as it is merged now in master cannot be flashed ?

It can still be flashed with OpenOCD but verification step in openocd fails. OpenOCD with the patch linked from this comment has code to compute and fix the checksum before flashing; however flashing and verifying are different processes so when you re-read the flash OpenOCD will tell you that the verification against he file without the correct checksum failed, and then it takes a while to re-read the whole flash to know which bytes are different (verification is done computing the hash by running code in the device, so the bytes are not read back to the host; if the hash fails verification falls back to re-read back to the host and compare byte-by-byte to report which bytes are different). I believe that the segger tool handles this without problem.

The NXP SDK with the mcuxpresso will produce an .elf that has the right checksum (it is a post-process in their makefiles like here but done with some other tool, the checksum algorithm is described in the manual and pretty simple) and I think it is safer to always produce a file with the correct checksum. I don't know if there is a way to do this as a post-process on the same .elf file instead of creating a new one with the RIOT makefiles.

I would be very interested by the results of the following command (be careful, it takes a lot of a time):

./dist/tools/compile_and_test_for_board/compile_and_test_for_board.py . qn9080dk --jobs=4 --with-test-only

I can try this, but at the moment this CPU doesn't have the UART module implemented (will be sending that patch next), how would this verify that the flash worked? just by the exit code of openocd? (a device that doesn't boot is about the same as one that boots but doesn't print anything in UART). I've been doing manual verification running gdb on the target after boot.

@aabadie
Copy link
Contributor

aabadie commented Dec 2, 2020

Indeed, without the UART driver, the compile_and_test_for_board is pretty useless. If you have a driver for UART ready, then please submit a PR quickly. Having UART support allows a lot of our tests to run on a platform, especially the threading and irq ones. Without it, it's difficult to know if the thread/irq support is correctly implemented.

If you say that the checksum improves things with OpenOCD, then the solution proposed by this PR is fine. But I would rather merge #15545 first (as it already had a CI run). The changes you proposed here would be the same anyway. Maybe you could simply rebased this PR on top of #15545. Thanks!

@aabadie
Copy link
Contributor

aabadie commented Dec 3, 2020

#15545 was just merged, please rebase your PR @iosabi !

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python functions names should use snake case style. Other than that everything is fine with this PR. I tested it locally and $(EFLFILE)-checksum.elf is used as flash file.

You can squash the suggestions below right away.

boards/common/qn908x/dist/nxp_checksum.py Outdated Show resolved Hide resolved
boards/common/qn908x/dist/nxp_checksum.py Outdated Show resolved Hide resolved
@iosabi
Copy link
Contributor Author

iosabi commented Dec 3, 2020

./dist/tools/compile_and_test_for_board/compile_and_test_for_board.py . qn9080dk --jobs=4 --with-test-only

With this patch plus the UART PR I just uploaded this is running. Had to install a bunch of packages, is there a place/script that would install all the dev dependencies for RIOT?

I got the following errors:

ERROR:qn9080dk:Tests failed: 6
Failures during compilation:
- [tests/pkg_cifra](tests/pkg_cifra/compilation.failed)
- [tests/pkg_cn-cbor](tests/pkg_cn-cbor/compilation.failed)

Failures during test:
- [tests/heap_cmd](tests/heap_cmd/test.failed)
- [tests/mpu_noexec_ram](tests/mpu_noexec_ram/test.failed)
- [tests/pkg_libfixmath_unittests](tests/pkg_libfixmath_unittests/test.failed)
- [tests/rmutex_cpp](tests/rmutex_cpp/test.failed)

pkg_cifra failis with:

build/pkg/cifra/src/tassert.h:27: error: "assert" redefined [-Werror]

pkg_cn-cbor fails with:

build/pkg/cn-cbor/src/cn-cbor.c: In function 'cn_cbor_decode':
cc1: error: function may return address of local variable [-Werror=return-local-addr]
build/pkg/cn-cbor/src/cn-cbor.c:267:11: note: declared here
  267 |   cn_cbor catcher = {CN_CBOR_INVALID, 0, {0}, 0, NULL, NULL, NULL, NULL};
      |           ^~~~~~~
cc1: all warnings being treated as errors

Note: I'm using 10-2020-q2-preview gcc.

heap_cmd, rmutex_cpp and mpu_noexec_arm worked on retry after unplugging/plugging my USB-UART. I think that my cheapo USB-UART probe is to blame here, I will try a better quality one I have here to check on the reliability. Manually testing those with the terminal worked fine too. The failure was the /dev/ttyUSB0 would be reading again and again the same text and not clearing it from the buffer.

pkg_libfixmath_unittests fails on the fix16 tests (about 20 or 30 failures in the fix16, the other are fine):

-0.600037 / -32768.000000 = 0.000015
-0.600037 / -32768.000000 = 0.000018
FAILED: fix16_unittests.c:194 failures == 0

QN908X CPUs require the image to have a valid checksum. The checksum is
a simple addition of the first 7 uint32_t values stored in the 8th
position of the image header. This position is a reseved entry of the
Cortex-M Vector Table and its value depends on other fields that are
computed at link time. Performing this checksum at link time seems
hard to do, so instead this patch uses a python script to patch the
checksum from the ELF file. This redefines the value of FLASHFILE
to the new .elf file with the checksum fixed.

With this patch, OpenOCD can program and verify QN908X images since
now they have a valid checksum value.
@aabadie
Copy link
Contributor

aabadie commented Dec 3, 2020

is there a place/script that would install all the dev dependencies for RIOT?

You can add BUILD_IN_DOCKER=1 to the command line. This will build using the same toolchains used on the CI.

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 3, 2020
@benpicco benpicco merged commit 0427e28 into RIOT-OS:master Dec 3, 2020
@iosabi iosabi deleted the qn908x_checksum branch December 4, 2020 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants