-
Notifications
You must be signed in to change notification settings - Fork 2.1k
sys/tsrb: add tsrb_clear #17337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sys/tsrb: add tsrb_clear #17337
Conversation
sys/include/tsrb.h
Outdated
/** | ||
* @brief Reset a tsrb. | ||
* @param[out] rb Ringbuffer to operate on | ||
*/ | ||
static inline void tsrb_reset(tsrb_t *rb) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just nitpicking the name, maybe tsrb_clear
would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought about it too and personally I would expect that clear
just clears bytes and reset
also resets the read and write pointer, which is the case here 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But no strong preference on my side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for tsrb_clear()
. The read and write pointers are implementation details that should be never accessed from users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then lets go with clear
, you can squash that right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to. I don't insist on the inline-suggestion, as I see arguments for both versions.
sys/include/tsrb.h
Outdated
static inline void tsrb_reset(tsrb_t *rb) | ||
{ | ||
unsigned irq_state = irq_disable(); | ||
tsrb_init(rb, rb->buf, rb->size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tsrb_init(rb, rb->buf, rb->size); | |
rb->reads = rb->writes; |
would also work. It might be beneficial to keep rb->reads
and rb->writes
monotonically growing (except for overflows) for debugging (e.g. to check the total number of bytes written to the tsrb). But I agree that it is more obvious that a call to tsrb_init()
will clear the buffer when reading the code.
I think that a modern compiler will emit equally efficient code for both versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to address or not @HendrikVE squash and trigger ci in either case!
9d18d1c
to
de4b32e
Compare
Note to self: use this in #17265 instead of zeroing out the read/write members. |
The CI is happy with the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, thanks @HendrikVE!
Contribution description
Add a function to reset the buffer to its initial state.
Testing procedure
A test case was added. Run
make tests-tsrb term