Skip to content

sys/tsrb: Make thread-safe#15218

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
maribu:tsrb-fix
Oct 20, 2020
Merged

sys/tsrb: Make thread-safe#15218
miri64 merged 1 commit intoRIOT-OS:masterfrom
maribu:tsrb-fix

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Oct 13, 2020

Contribution description

Drop the requirement of having only one writer and one reader, as the name of the ring-buffer does not indicate any limitation on the thread-safety. The two-threads-one-buffer kind of ring buffer can be reintroduced with a different name.

Testing procedure

On e.g. STM32 or SAM0 boards, have two threads continuously produce stdio output. The thread with higher priority should sleep for random intervals.

Expected output:

  • The higher priority threads interrupts the lower priority thread and stdio output gets mixed up. But no characters are lost or printed more than once

Actual output:

  • Occasionally, chars get lost or printed more than once

Issues/PRs references

Fixes #9882

Split out of #15103

Drop the requirement of having only one writer and one reader, as the name of
the ring-buffer does not indicate any limitation on the thread-safety. The
two-threads-one-buffer kind of ring buffer can be reintroduced with a different
name.
@maribu maribu requested a review from kaspar030 as a code owner October 13, 2020 08:38
@miri64 miri64 added Area: sys Area: System Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 13, 2020
@miri64 miri64 added this to the Release 2020.10 milestone Oct 13, 2020
@miri64 miri64 requested a review from bergzand October 13, 2020 09:31
@benpicco benpicco added 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 and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 14, 2020
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Makes sense, especially when you add the old version under a different name for the one reader one writer case.

The 'thread safe' ring buffer actually not being thread safe at all has thrown me off already too.

@miri64 miri64 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 Oct 20, 2020
@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 20, 2020

pkg_libhydrogen test fail is unrelated (see #15066)

@miri64 miri64 merged commit 9e4c508 into RIOT-OS:master Oct 20, 2020
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 20, 2020

Thanks :-)

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 20, 2020

Backport provided in #15254

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

Labels

Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch 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.

sys/tsrb is not thread safe

3 participants