Allow serial buffers of different sizes

[jerabaul29]

It looks like for now all serial buffers have the same size. This is a bit annoying, as if one may expect a lot of input on a single RX channel for example, one needs to use a lot of RAM increasing the size of all serial buffers instead of just the one needed.

Do you think it would be possible to:

• add a constexpr size_t buffer_size argument in the RingBuffer constructor, suppose that would be here:

ArduinoCore-sam/cores/arduino/RingBuffer.h

Line 38 in 790ff2c

RingBuffer( void ) ;

ArduinoCore-sam/cores/arduino/RingBuffer.cpp

Lines 22 to 27 in 790ff2c

	RingBuffer::RingBuffer( void )
	{
	memset( (void *)_aucBuffer, 0, SERIAL_BUFFER_SIZE ) ;
	_iHead=0 ;
	_iTail=0 ;
	}

• use this constexpr size as the size of the internal array, and store it in the class so that it remains aware of its size, ie adding 1 internal size_t field in the header declaration, storing it in the constructor, and replacing in the implementation calls to the macro size by call to this field:

ArduinoCore-sam/cores/arduino/RingBuffer.cpp

Line 31 in 790ff2c

int i = (uint32_t)(_iHead + 1) % SERIAL_BUFFER_SIZE ;

• add to the UART and USART the same constexpr size argument, so that we can propagate it from the UART / USART creation to the buffer

• specify independently the size of each RX and TX buffer. The values could be given through macros, so that this is easily overwritten by a compiler flag. I think that would take place here, right?

ArduinoCore-sam/variants/arduino_due_x/variant.cpp

Lines 301 to 336 in 790ff2c

	/*
	* UART objects
	*/
	RingBuffer rx_buffer1;
	RingBuffer tx_buffer1;
	UARTClass Serial(UART, UART_IRQn, ID_UART, &rx_buffer1, &tx_buffer1);
	void serialEvent() __attribute__((weak));
	void serialEvent() { }
	// IT handlers
	void UART_Handler(void)
	{
	Serial.IrqHandler();
	}
	// ----------------------------------------------------------------------------
	/*
	* USART objects
	*/
	RingBuffer rx_buffer2;
	RingBuffer rx_buffer3;
	RingBuffer rx_buffer4;
	RingBuffer tx_buffer2;
	RingBuffer tx_buffer3;
	RingBuffer tx_buffer4;
	USARTClass Serial1(USART0, USART0_IRQn, ID_USART0, &rx_buffer2, &tx_buffer2);
	void serialEvent1() __attribute__((weak));
	void serialEvent1() { }
	USARTClass Serial2(USART1, USART1_IRQn, ID_USART1, &rx_buffer3, &tx_buffer3);
	void serialEvent2() __attribute__((weak));
	void serialEvent2() { }
	USARTClass Serial3(USART3, USART3_IRQn, ID_USART3, &rx_buffer4, &tx_buffer4);
	void serialEvent3() __attribute__((weak));
	void serialEvent3() { }

Any problem you would see with such an approach? Any difficulty I I forgotten / anything wrong with this idea?

I suppose this would be applicable in general to all Arduino cores, right?

[jerabaul29]

(note: I suppose we may need to add a bit of templating on top of that, and / or use templating instead, so that we can have different sizes of arrays in different classes at the same time?).

[matthijskooijman]

One problem with arguments to the constructor is that they cannot influence the static size of the class, so the only way to implement that is by using malloc for the buffers (which could be acceptable, given it's allocated once at startup and never freed, so little chance of fragmentation, but it would also make it harder to show memory used by the sketch).

This can indeed be fixed using template arguments, but that makes the interface more complicated (though if this is not needed in normal use, that might be acceptable).

One alternative is to allocate the actual storage external to the RingBuffer class and pass a pointer and size to it, which would remove the need for template arguments and ensures that more code can be shared between RingBuffer instances with different sizes.

If I understand your idea correctly, you're suggesting that the actual user-facing API would always be the macros? So the template/constructor arguments are only needed to propagate these macros from the UART code to the RingBuffer code (i.e. so the RingBuffer code does not need to know about these UART macros)?

One complication is that Arduino does not currently have any proper way to configure compiler flags and/or defines for a sketch, which means that using macros to configure the core is not ideal (but there aren't really any working alternatives, so it's probably good to implement this anyway and then maybe fix the compiler flags thing separately). Other cores already implement such macros as well (i.e. the AVR core allows setting HardwareSerial buffer sizes using macros, IIRC).

[jerabaul29]

Ok, yes I suppose that having either some templating, or external static storage where we transmit the size, may be a good idea. I suppose templating may be the safest / easiest, and it is anyways very little code in the ring buffer implementation, so I suppose this would not come at a too large flash cost. What is your opinion? Would you support a templating approach?

Yes, my initial idea was to use macros as the user interface. In a sense, this is already what is in place - just that there is a single macro entry here for now:

ArduinoCore-sam/cores/arduino/RingBuffer.h

Line 28 in 790ff2c

#define SERIAL_BUFFER_SIZE 128

I suppose the advantage of using macros as an interface is that from the point of view of the user, "nothing changes", and we keep the same system for defining these sorts of constants.

Regarding how to use it from the Arduino IDE, I suppose that "power users" will be able to edit their cores, though I agree that is not very convenient.

I also wonder what happens if the user #undef and then #define anew some buffer size setup macro at the beginning of their sketch. Would this well over-write the default setting, or not? I am not well known enough in the preprocessor to be sure. If it is enough to undef and re-define at the start of the sketch, that would be great.

But to be honest, I do not use the arduino IDE, but VSC+platformio. In this case, it is very easy to set a compiler flag that over-rides the macro, hence my initial desire to keep the macro :) See:

https://community.platformio.org/t/how-to-increase-the-arduino-serial-buffer-size/122/2

So I would like to be able to just write my project platformio.ini file as something like:

[env:due]
platform = atmelsam
board = due
framework = arduino
build_flags = -D SERIAL_RX1_BUFFER_SIZE=1024

Would be really nice. I suppose to make sure this works well we would just need to protect a bit this part of the code:

ArduinoCore-sam/cores/arduino/RingBuffer.h

Line 28 in 790ff2c

#define SERIAL_BUFFER_SIZE 128

Into something like:

#if !defined(SERIAL_RX1_BUFFER_SIZE)
  #define SERIAL_RX1_BUFFER_SIZE 128
#endif
[... etc for all RX and TX buffer sizes]

[jerabaul29]

(note though that I am not sure of where these macros should be defined; in order to make the code easy to read / adapt for other boards - as I think this modification could be good for the Arduino Mega too for example), I suppose these macros definition may be moved to something that is more tightly coupled to the board - either variants.h, of something like this maybe?

[jerabaul29]

(note: once we agree on the design of this I will be happy to work on a pull request, but I would like to get some help on the general design first and some debates / suggestions first :) ).

[matthijskooijman]

and it is anyways very little code in the ring buffer implementation, so I suppose this would not come at a too large flash cost.

Yeah, so that's probably fine, most of the operations should be inlined anyway, I think. It even seems the ringbuffer is currently actually too small now, it only has a store_char, so I suppose that reading from the buffer is done by poking in the buffer directly, which isn't so nice.

It actually seems that the ArduinoCore-API repo already has the templated RingBuffer approach: https://github.com/arduino/ArduinoCore-API/blob/master/api/RingBuffer.h

Not sure when the SAMD core will be converted to use that, but maybe importing that part already could make sense (certainly better than writing something different from scratch).

Yes, my initial idea was to use macros as the user interface. In a sense, this is already what is in place - just that there is a single macro entry here for now:

Right, though there is no #ifndef guard around that, so if you would override this value from the commandline, you'd get a "macro redefined" warning or so, I think (and I'm not sure if it would use the old or new value). But the basic thing is there, yeah.

I also wonder what happens if the user #undef and then #define anew some buffer size setup macro at the beginning of their sketch. Would this well over-write the default setting, or not? I am not well known enough in the preprocessor to be sure. If it is enough to undef and re-define at the start of the sketch, that would be great.

That won't work: Those defines will only be active when compiling the sketch, when compiling e.g. RingBuffer.cpp or UARTClass.cpp, those macros will not be seen, resulting in disagreement about the buffer size between different parts of the code. With some special care you could maybe make this work (by somehow ensuring that the buffer is actually allocated in the .ino compilation unit, but I think this will be fragile).

Would be really nice. I suppose to make sure this works well we would just need to protect a bit this part of the code:
...
Into something like:

Yup, that would make sense to me.

I actually don't think this should be in RingBuffer.h, but in HardwareSerial.h or something like that, but the -API repo does not currently agree with that.

(note though that I am not sure of where these macros should be defined; in order to make the code easy to read / adapt for other boards - as I think this modification could be good for the Arduino Mega too for example), I suppose these macros definition may be moved to something that is more tightly coupled to the board - either variants.h, of something like this maybe?

I think a default in the serial header would make sense, but with #ifndef around it so it can be overriden from the commandline or variant.h would make sense (does mean that the serial header needs to include the variant).