Skip to content

Conversation

Enoch247
Copy link
Contributor

@Enoch247 Enoch247 commented Apr 1, 2025

Contribution description

This PR originally started out as a fix to isrpipe's init of it's mutex, but it grew into adding unit tests and improving its doc as well. I also improved some doc for tsrb. I can break that into a separate PR if you desire.

Testing procedure

make -C tests/unittests/ tests-isrpipe test

Issues/PRs references

None known

@Enoch247 Enoch247 requested a review from miri64 as a code owner April 1, 2025 21:02
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Apr 1, 2025
@Enoch247 Enoch247 changed the title DRAFT: Fix isrpipe DRAFT: Fix init of isrpipe's mutex Apr 1, 2025
@benpicco benpicco requested a review from kaspar030 April 2, 2025 09:45
@crasbe crasbe added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 2, 2025
@crasbe
Copy link
Contributor

crasbe commented Apr 2, 2025

Now the test works successfully on my nrf52dk, let's see what CI thinks 👀

@riot-ci
Copy link

riot-ci commented Apr 2, 2025

Murdock results

✔️ PASSED

5b7c5b2 sys/tsrb: emphasize buffer size requirement

Success Failures Total Runtime
10295 0 10295 10m:39s

Artifacts

@github-actions github-actions bot added the Area: sys Area: System label Apr 2, 2025
@Enoch247 Enoch247 changed the title DRAFT: Fix init of isrpipe's mutex sys/isrpipe: unit tests, doc, and fix init Apr 2, 2025
@crasbe
Copy link
Contributor

crasbe commented Apr 4, 2025

You can squash the fixup and resolve the conversations. Also you can remove the WIP label, as it doesn't seem to be WIP anymore?

@Enoch247 Enoch247 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Apr 6, 2025
@Enoch247
Copy link
Contributor Author

Enoch247 commented Apr 6, 2025

I believe this ready to merge, pending an approval.

Copy link
Contributor

@crasbe crasbe left a comment

Choose a reason for hiding this comment

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

Tested and working on native and a nucleo-g071rb I have laying around. Please address the comment above before merging.

@Enoch247
Copy link
Contributor Author

Enoch247 commented Apr 7, 2025

I rebased to take advantage of #21367.

Enoch247 added 2 commits April 8, 2025 20:17
This patch adds unit tests for the isrpipe module.
The mutex used to sync the reader and writer of the pipe is initialized
as unlocked. This results in a bit of wasted CPU cycles the first time a
read blocks. This patch inits the mutex in a locked state so that the
first blocking read blocks immediately.
Enoch247 added 3 commits April 8, 2025 20:55
This patch makes the requirement on buffer size more prominent.
Additionally, it adds the missing argument to the doxygen block of the
static initializer. Finally, it chanes the argument name passed to the
static intializer to decouble the API from the implmentation details.
This patch documents what happens when a timeout occurs when
isrpipe_read_all_timeout() has been called.
This patch makes the requirement on buffer size more prominent.
Additionally, it adds the missing argument to the doxygen block of the
static initializer.
@Enoch247 Enoch247 requested a review from benpicco April 9, 2025 01:02
@Enoch247 Enoch247 added this pull request to the merge queue Apr 9, 2025
Merged via the queue into RIOT-OS:master with commit dcf71b5 Apr 10, 2025
27 checks passed
@mguetschow mguetschow added this to the Release 2025.04 milestone Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System 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
Development

Successfully merging this pull request may close these issues.

6 participants