Skip to content

tests/ps_schedstatistics: fix test on AVR + improve Python test script#12832

Merged
aabadie merged 2 commits intoRIOT-OS:masterfrom
aabadie:pr/tests/ps_schedstatistics_enh
Nov 28, 2019
Merged

tests/ps_schedstatistics: fix test on AVR + improve Python test script#12832
aabadie merged 2 commits intoRIOT-OS:masterfrom
aabadie:pr/tests/ps_schedstatistics_enh

Conversation

@aabadie
Copy link
Copy Markdown
Contributor

@aabadie aabadie commented Nov 28, 2019

Contribution description

This PR is an attempt to fix the tests/ps_schedstatistics on AVR by simply increasing the XTIMER_BACKOFF value.

The PR also contains small cleanup in the Python test script:

  • It ensures all lines are displayed before exiting the script
  • It escapes the parenthesis that are not use for group regexp

Testing procedure

The following command should succeed, also on AVR (where it fails on master):

make BOARD=<board of your choice> -C tests/ps_schedstatistics flash test

Issues/PRs references

Tick one item in #12651

@aabadie
tests/ps_schedstatistics: use lower load in background threads
c9fccbc 
On slow platforms, such as AVR, the main thread could never run and the shell would remain stuck.
@aabadie
tests/ps_schedstatistics: improve automatic test script
f885b90 
- Ensure the whole ps output is displayed in the terminal before exiting the script
- Escape parenthesis in line regexp
@aabadie aabadie added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) 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 CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Nov 28, 2019
@aabadie aabadie requested a review from maribu November 28, 2019 07:34
@aabadie aabadie mentioned this pull request Nov 28, 2019
Closed
13 tasks
@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 28, 2019
@maribu
Copy link
Copy Markdown
Member

maribu commented Nov 28, 2019

After thinking about it again, I think that @fjmolinas is right that it is better to should increase the value of XTIMER_BACKOFF on AVR. Ideally, depending on the CPU frequency. (This is currently still not running on 8 MHz AVRs.)

@maribu
Copy link
Copy Markdown
Member

maribu commented Nov 28, 2019

But on the other hand I see no reason to not just also decrease the load a bit to make the test more robust in case of small values of XTIMER_BACKOFF. (The value of XTIMER_BACKOFF for ATmegas can still be tweaked definitely.)

maribu
maribu approved these changes Nov 28, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Works fine on Nucleo-F767ZI. The improvement of the test script makes sense and being more conservative about the load a board can handle is always a good idea.

Ran successfully on an ATmega1284P clocked at 8MHz when reducing the load a bit more. I'd say the remaining load reduction should be done by tweaking XTIMER_BACKOFF on AVR; ideally to increase with decreasing core clock.

@maribu
Copy link
Copy Markdown
Member

maribu commented Nov 28, 2019

@fjmolinas: Could you double-check my reasoning above? To me, the value in xtimer_usleep() is pretty arbitrary anyway, so increasing it a bit (in addition to fixing XTIMER_BACKOFF later on) to make the test more robust makes sense.

@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Nov 28, 2019

@fjmolinas: Could you double-check my reasoning above? To me, the value in xtimer_usleep() is pretty arbitrary anyway, so increasing it a bit (in addition to fixing XTIMER_BACKOFF later on) to make the test more robust makes sense.

I have no issue with increasing the sleep time, it will have an impact on how the load is shared among threads, e.g. for native runtime for the threads goes down from ~1.4% to ~0.3%. But this is not in itself wrong, so +1.

After thinking about it again, I think that @fjmolinas is right that it is better to should increase the value of XTIMER_BACKOFF on AVR. Ideally, depending on the CPU frequency. (This is currently still not running on 8 MHz AVRs.)

Here is where is get tricky, I'm not sure how CPU frequency and TIMER frequency interact here, and since there are other xtimer issues. I would need to take a closer look at xtimer again as well as all the resent PR's trying to fix xtimer issues, so I agree with not dealing with this here.

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Nov 28, 2019

The issue with Murdock is unrelated, I'm retriggering a simple build.

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

fjmolinas commented Nov 28, 2019

The issue with Murdock is unrelated, I'm retriggering a simple build.

Next time please post the build result in the PR, specially if triggering a simple build.

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Nov 28, 2019

Only examples/suit_update test was failing on samr21-xpro.

@aabadie aabadie merged commit f5252bf into RIOT-OS:master Nov 28, 2019
@aabadie aabadie deleted the pr/tests/ps_schedstatistics_enh branch November 28, 2019 12:58
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
@maribu maribu mentioned this pull request Jul 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maribu maribu maribu approved these changes

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: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2020.01

Development

Successfully merging this pull request may close these issues.

3 participants

@aabadie @maribu @fjmolinas