Skip to content

Conversation

HendrikVE
Copy link
Contributor

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

@HendrikVE HendrikVE requested a review from miri64 as a code owner December 3, 2021 15:58
@github-actions github-actions bot added Area: sys Area: System Area: tests Area: tests and testing framework labels Dec 3, 2021
Comment on lines 70 to 74
/**
* @brief Reset a tsrb.
* @param[out] rb Ringbuffer to operate on
*/
static inline void tsrb_reset(tsrb_t *rb)
Copy link
Contributor

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?

Copy link
Contributor Author

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 🤷

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor

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.

@benpicco benpicco requested review from kaspar030 and maribu December 3, 2021 20:31
Copy link
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.

Looks good to. I don't insist on the inline-suggestion, as I see arguments for both versions.

static inline void tsrb_reset(tsrb_t *rb)
{
unsigned irq_state = irq_disable();
tsrb_init(rb, rb->buf, rb->size);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor

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!

@HendrikVE HendrikVE added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 6, 2021
@miri64
Copy link
Member

miri64 commented Dec 6, 2021

Note to self: use this in #17265 instead of zeroing out the read/write members.

@HendrikVE HendrikVE changed the title sys/tsrb: add tsrb_reset sys/tsrb: add tsrb_clear Dec 6, 2021
@HendrikVE
Copy link
Contributor Author

The CI is happy with the changes.

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK, thanks @HendrikVE!

@fjmolinas fjmolinas enabled auto-merge December 7, 2021 08:20
@fjmolinas fjmolinas merged commit 6749b71 into RIOT-OS:master Dec 7, 2021
@HendrikVE HendrikVE deleted the pr/tsrb_init branch December 28, 2021 22:33
@fjmolinas fjmolinas added this to the Release 2022.01 milestone Jan 21, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants