Allow serial buffers of different sizes

[jerabaul29]

Allow serial buffers of different sizes

[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).

[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).

[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?).

[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 :) ).