Hi Paul, I do like the idea of using a common ring buffer class, like the current SAM/SAMD implementations do. And it would be great to go through and do it.
I also like the idea of being able to control the size of the RX/TX buffers. I am a little on the fence if I like the addStorage way of doing it. I personally prefer keeping it sort of simple as have methods like setRXSize(), setTXSize() or maybe members on begin for size and have a system function allocate the buffers at the first time begin is called...
I like this approach as you don't waste memory for devices you don't use, and I personally don't like the idea of simple user screw ups killing things...
However I know I am maybe in the minority as I know there are those who want a statically defined system where there are no malloc or new or... And the user could screw up and says set my rxBuffer to 32K when I only have 8K free... Or more subtle where they allocate enough where their stack later collides with the heap...
But I can also easily imagine user screw ups using the add storage method as well. Users could do things like:
Code:
void setup () {
char * extra_buffer[128];
Serial1.begin(...,RXBUFFER(extra_buffer));
The buffer is on the stack, and...
or
Code:
char * extra_buffer[128];
void setup () {
Serial1.begin(...,RXBUFFER(extra_buffer), TXBUFFER(extra_buffer));
Use same buffer for TX and RX... Likewise, I can easily imagine something like:
Code:
char * serial1_buffer[128];
char * serial2_buffer[128];
void setup () {
Serial1.begin(...,RXBUFFER(serial1_buffer));
Serial2.begin(...,RXBUFFER(serial1_buffer));
They did copy paste for Serial1 to Serial2, but did not update which buffer and so both use the same extra...
As you mentioned adding the extra buffer adds a little complexity to the code. Like storing a character:
Code:
void RingBuffer::store_char( uint8_t c )
{
int i = nextIndex(_iHead);
// if we should be storing the received character into the location
// just before the tail (meaning that the head would advance to the
// current location of the tail), we're about to overflow the buffer
// and so we don't write the character or advance the head.
if ( i != _iTail )
{
if (_iHead < size) {
_aucBuffer[_iHead] = c ;
} else {
additionalBuffer[_iHead - size] = c;
}
_iHead = i ;
}
}
Again probably not a big deal, it simply has to check if an index is in the fixed part or the additional part...
I see that his implementation has variables like _iHead as variable type int, could imagine it could be something like uint16_t, but that would restrict the max size.
So I like the idea of the flexibility going either way... For those who wish to not waste memory on devices they don't use, then maybe they could setup to have all of the devices allocate a very small buffer like 8 bytes... And then for the ones they use they allocate the extra..
Edit: As there is already a Pull request into main Arduino probably should just do it!
Let me know if you want to do it, and if you want any help
