Skip to content

sys/test_utils/interactive_sync: AVR puts to pgmspace#12941

Merged
aabadie merged 1 commit intoRIOT-OS:masterfrom
fjmolinas:pr_interactive_sync_puts_pgmspace
Dec 13, 2019
Merged

sys/test_utils/interactive_sync: AVR puts to pgmspace#12941
aabadie merged 1 commit intoRIOT-OS:masterfrom
fjmolinas:pr_interactive_sync_puts_pgmspace

Conversation

@fjmolinas
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas commented Dec 13, 2019

Contribution description

When testing #12862 I realized some tests where not compiling on arduino-uno, in the PR as well as in master. @kaspar030 helped me to figure out this was because of the application being so tite that
the lenght of RIOT_VERSION caused the section to overflow.

The reason behind the overflow comes from the addition of tests_interactive_sync_utils which adds some prints and constant data that goes into .bss. To reduce the strain imposed by tests_interactive_sync_utils we use puts_p in test_utils/interactive_sync when compiling for __AVR__ to store those prints in pgmspace

Testing procedure

  • Reproduce failure on master

    • SUCCEEDSRIOT_VERSION="short" BOARD=arduino-uno make -C tests/l2util
    • FAILS RIOT_VERSION="verylongversiontocauseoverflow" BOARD=arduino-uno make -C tests/l2util/

  • This PR saves about ~60 bytes in RAM but increases the total FW size ~100bytes.

PR

RIOT_VERSION=short BOARD=arduino-uno make -C tests/l2util/
   text    data     bss     dec     hex filename
  14612     842     975   16429    402d /home/francisco/workspace/RIOT-test/tests/l2util/bin/arduino-uno/tests_l2util.elf

master

RIOT_VERSION=short BOARD=arduino-uno make -C tests/l2util/
   text    data     bss     dec     hex filename
  14450     906     975   16331    3fcb /home/francisco/workspace/RIOT-test/tests/l2util/bin/arduino-uno/tests_l2util.elf

@fjmolinas fjmolinas added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tests Area: tests and testing framework labels Dec 13, 2019
@fjmolinas fjmolinas requested a review from aabadie December 13, 2019 10:06
kaspar030
kaspar030 approved these changes Dec 13, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

Poor little AVR's.

@kaspar030 kaspar030 added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 13, 2019
@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Dec 13, 2019

Unrelated test fail, will re-trigger without running tests.

---
--- run_test job results (1 failed, 8 passed, 9 total):
    failed:
    tests/shell/esp32-wroom-32:gnu

    passed:
    tests/pkg_qdsa/samr21-xpro:gnu
    tests/pkg_relic/esp32-wroom-32:gnu
    tests/pkg_relic/nrf52dk:gnu
    tests/pkg_relic/samr21-xpro:gnu
    examples/suit_update/nrf52dk:gnu
    examples/suit_update/samr21-xpro:llvm
    examples/suit_update/samr21-xpro:gnu
    examples/suit_update/nrf52dk:llvm

@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 13, 2019
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Dec 13, 2019

All green now, let's go!

@aabadie aabadie merged commit 7dbf5c6 into RIOT-OS:master Dec 13, 2019
This was referenced Dec 15, 2019
Merged
Merged
@fjmolinas fjmolinas deleted the pr_interactive_sync_puts_pgmspace branch January 6, 2020 12:45
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Jan 15, 2020
fjmolinas added a commit to fjmolinas/RIOT that referenced this pull request Mar 24, 2020
@fjmolinas
tests: remove uneeded blacklist
ec33b3e 
With RIOT-OS#12941 and RIOT-OS#13613 some of the blacklisting introduced in RIOT-OS#12461
are no longer needed, since `test_interactive_test_util` is lighter
or adds no extra code.
@fjmolinas fjmolinas mentioned this pull request Mar 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaspar030 kaspar030 kaspar030 approved these changes

@aabadie aabadie Awaiting requested review from aabadie

Assignees

No one assigned

Labels

Area: tests Area: tests and testing framework 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

Milestone

Release 2020.01

Development

Successfully merging this pull request may close these issues.

3 participants

@fjmolinas @aabadie @kaspar030