sys/tsrb: bug fix for tsrb_get_one()

[korotkoves]

Contribution description

Fixes bug for getting 0xFF byte from ring buffer.
When we want to get such byte as char (signed type), it returns -1 (means buffer is empty).

Testing procedure

Put 0xFF byte to buffer and try to get it with tsrb_get_one()

[smlng]

To me the problem lies in mixing error return codes with values. A better solution would be to change the signature of the function into: int tsrb_get_one(tsrb_t *rb, char *dst). Also the same problem is otherwise with tsrb_get() below.

[miri64]

I disagree with changing the signature of the API-facing tsrb_get_one() / tsrb_get(). Rather, _pop()'s return value should just be cast to unsigned char. Since sizeof(unsigned char) < sizeof(int) on all of our platforms, this is acceptable and does not mix error return codes with values.

[smlng]

this still looks wrong to me bc it uses char not unsigned char when adding stuff also the buffer is char. It just does not look really (type) safe to me - I would use uint8_t to make it explicit.

[kaspar030]

IMO casting to unsigned char is the best option here. It is what getchar is doing, so can be called ideomatic C.
Also we might get away with calling this a bugfix, as the documentation clearly states that it returns the byte as >= 0. Now that I think of it, the cast must have been my initial intention anyways.

[miri64]

this still looks wrong to me bc it uses char not unsigned char when adding stuff also the buffer is char. It just does not look really (type) safe to me - I would use uint8_t to make it explicit.

In my opinion, we should focus on fixing the bug first, than we can discuss API changes for type safeness. The bug is fixable without an API change, so we should do that first, then do an API change if necessary. This not only makes the merge of this bug fix faster, but also allows for a better understanding of the thought process of the API change when looking at the code's history.

[kaspar030]

@smlng are you fine with this fix for now, and we revisit the API later?

[smlng]

yeah, but it should not be that much later.

---

Another idea for a fix:

Documentation states, that this assumes a single producer and consumer scenario, right? So actually there is no need to return -1 on error but rather change the function to char tsrb_get_one() and the consumer MUST call tsrb_empty before access on its own otherwise the consumer might get an old/invalid value. This works because there is only one consumer hence there can be no other consumer manipulating the reads.

Still it might be sensible to change every char to unsigned char anyway.

[miri64]

(reminder to self: there should be a unittest for tsrb)

[miri64]

Did it in #11632.

[kaspar030]

This problem also popped up in #9869.

won't block bugfix

[miri64]

The problem now is, that it might fix for \xff, but now the result is not comparable to what you put into the ring buffer. If you e.g. put '\xdb' in the buffer, tsrb_get_one() will get you an integer 219 (0x000000db), which will yield false when you compare '\xdb' == 219 without any cast.

[miri64]

I guess long-term we need to do an API change, but I'd vote rather for going from char to uint8_t.

[miri64]

ACK for this change however, though the values are not comparable directly now, it fixes that a valid output (\xff) is interpreted as an error.