Skip to content

tests/thread_flood: account for all threads already in use#14181

Merged
aabadie merged 1 commit intoRIOT-OS:masterfrom
keestux:fix-test-thread-flood
Jun 8, 2020
Merged

tests/thread_flood: account for all threads already in use#14181
aabadie merged 1 commit intoRIOT-OS:masterfrom
keestux:fix-test-thread-flood

Conversation

@keestux
Copy link
Contributor

@keestux keestux commented May 30, 2020

Before there were only two: "main" and "idle". But now there can also be
a thread for "usbus". The code will now first count how many there are in
use when the tests starts. It subtracts that number from MAXTHREADS.

This resolves issue #14180 (fixes #14180)

@aabadie aabadie added 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) labels May 31, 2020
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.

Tested on arduino-mkr1000, I confirm the test is failing on master and fixed with this PR. Thanks!
It also works the same as on master on boards without stdio over USB (tested on stm32f429i-disc1).

I have one question regarding the use of volatile, I don't think it's required here. See below. Apart from that, I think this is good.

@benpicco benpicco 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 Jun 1, 2020
@keestux
Copy link
Contributor Author

keestux commented Jun 7, 2020

There are more tests suffering from this, e.g. tests/cond_order. Perhaps we can collect corrections for all of them

@keestux
Copy link
Contributor Author

keestux commented Jun 7, 2020

And tests/cpp11_thread

/* http://en.cppreference.com/w/cpp/thread/thread */
int main() {
  puts("\n************ C++ thread test ***********");

  expect(sched_num_threads == 2); // main + idle

@keestux keestux force-pushed the fix-test-thread-flood branch from a774a73 to 9a72ee7 Compare June 7, 2020 18:31
Before there were only two: "main" and "idle". But now there can also be
a thread for "usbus". The code will now use sched_num_threads and it
subtracts that number from MAXTHREADS.

This resolves issue RIOT-OS#14180
@keestux keestux force-pushed the fix-test-thread-flood branch from 9a72ee7 to bbd7c43 Compare June 8, 2020 09:23
@keestux
Copy link
Contributor Author

keestux commented Jun 8, 2020

Can I add more fixes to this PR to solve similar problems? I have fixes for tests/cond_order, tests/mutex_order and tests/cpp11_thread.

@kaspar030
Copy link
Contributor

Can I add more fixes to this PR to solve similar problems? I have fixes for tests/cond_order, tests/mutex_order and tests/cpp11_thread.

rather open individual PRs, faster to test.

BTW, #14224 also needs this!

@kaspar030
Copy link
Contributor

tests/rmutex is also affected.

@kaspar030
Copy link
Contributor

I used this to test: #14227

Works with dummy thread and this PR, fails with dummy thread on master.

ACK.

Copy link
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.

@aabadie, last look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

Test thread_flood is failing since we have a new thread "usbus"

5 participants