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

Remove patch #67

Merged
merged 9 commits into from
Mar 6, 2020
Merged

Remove patch #67

merged 9 commits into from
Mar 6, 2020

Conversation

gendx
Copy link
Collaborator

@gendx gendx commented Mar 5, 2020

Fixes #58. This synchronizes with upstream Tock to remove the patch added in #60.

  • Tests pass
  • Appropriate changes to README are included in PR

ia0
ia0 previously approved these changes Mar 5, 2020
run_desktop_tests.sh Outdated Show resolved Hide resolved
.github/workflows/boards_build.yml Outdated Show resolved Hide resolved
.github/workflows/boards_build.yml Show resolved Hide resolved
@gendx
Copy link
Collaborator Author

gendx commented Mar 5, 2020

There's something weird happening on my desktop.

My guess is that because my build folder is longer than on Travis, the binary is bigger. Indeed, due to Rust including debug paths in binaries, the longer the folder, the larger the binary.

$ strings boards/nordic/nrf52840dk/target/thumbv7em-none-eabi/release/nrf52840dk | grep <my build folder> | wc -l
579

This means that I reach the limit of 128K currently set in the layout.ld, but not Travis and GitHub workflows (yet).

We should be able to increase the limit to 192K (the prog section starts at address 0x00030000), but I want to make sure that regressions are caught by our CI.


On Travis:

   Compiling nrf52dk_base v0.1.0 (/home/travis/build/google/OpenSK/third_party/tock/boards/nordic/nrf52dk_base)
    Finished release [optimized + debuginfo] target(s) in 26.07s
   text	   data	    bss	    dec	    hex	filename
 128512	   1720	 260424	 390656	  5f600	target/thumbv7em-none-eabi/release/nrf52840dk

=> text + data = 130232 = 0x1FCB8

For me the text section takes 1024 more bytes:

    Finished release [optimized + debuginfo] target(s) in 0.02s
   text	   data	    bss	    dec	    hex	filename
 129536	   1720	 260424	 391680	  5fa00	target/thumbv7em-none-eabi/release/nrf52840dk

=> text + data = 131256 = 0x200B8

Edit: on GitHub workflow with a long directory name:

   Compiling nrf52dk_base v0.1.0 (/home/runner/work/OpenSK/OpenSK/this-is-a-long-build-directory-0123456789abcdefghijklmnopqrstuvwxyz/third_party/tock/boards/nordic/nrf52dk_base)
    Finished release [optimized + debuginfo] target(s) in 20.30s
   text	   data	    bss	    dec	    hex	filename
 131072	   1720	 260424	 393216	  60000	target/thumbv7em-none-eabi/release/nrf52840dk

=> text + data = 132792 = 0x206B8

@ia0
Copy link
Member

ia0 commented Mar 5, 2020

Sounds good to me to use 192K. Also note that we don't use 0x30000-0x40000. So we could also push that limit if needed.

@gendx
Copy link
Collaborator Author

gendx commented Mar 5, 2020

Sounds good to me to use 192K. Also note that we don't use 0x30000-0x40000. So we could also push that limit if needed.

That's good to know. Although I don't expect reaching the next limit soon. We could also tune the board configuration to remove all the drivers that we don't use, which would definitely free up some space but also be one more patch to maintain.

@gendx
Copy link
Collaborator Author

gendx commented Mar 5, 2020

I also tested the app on the nRF52840-DK. Works.

$ ./deploy.py os --board nrf52840_dk
$ ./deploy.py app --opensk
$ ./deploy.py app --panic-console --debug --opensk

.github/workflows/boards_build.yml Outdated Show resolved Hide resolved
.github/workflows/boards_build.yml Outdated Show resolved Hide resolved
.github/workflows/boards_build.yml Show resolved Hide resolved
@gendx
Copy link
Collaborator Author

gendx commented Mar 5, 2020

Removed the path rules in the new workflow, to keep this pull-request focused on removing the patch.

I don't think the build_boards is slow enough to justify specialized path rules, but if so we can do that in a separate pull-request.

bors bot added a commit to tock/tock that referenced this pull request Mar 5, 2020
1664: Generalize chip_layout.ld to Nordic boards and increase ROM size on nrf52840. r=ppannuto a=gendx

### Pull Request Overview

This pull request increases the `rom` size on nrf52840-based boards to fill up the space available before apps. This makes it consistent with the nRF52-DK (which has more `rom` available despite fewer flash space overall).

Files are also re-organized so that nRF52840-DK and nRF52840-Dongle share the same chip layout file.

Note that the previous limit of 128KB was just exceeded in google/OpenSK#67. There are more drivers in that case (persistent storage and USB), but it's simpler to keep the limit higher in Tock than to have to maintain a separate patch for OpenSK.


### Testing Strategy

This pull request was tested by:
- compiling all boards with `make ci-travis`,
- flashing an nRF52840-DK with OpenSK.


### TODO or Help Wanted

N/A


### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make formatall`.

Co-authored-by: Guillaume Endignoux <guillaumee@google.com>
@gendx gendx merged commit 96b1a68 into google:master Mar 6, 2020
This was referenced Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debug output sometimes freezes the OpenSK application
3 participants