Make SERCOM(0,1)_Handler optional

[maxlem]

Hello,

In file variant.cpp, two handlers are defined

void SERCOM0_Handler(){  Serial1.IrqHandler();}
void SERCOM5_Handler(){  Serial.IrqHandler();}

I understand it simplify the life of many, e.g. but in my case, I use SERCOM5 as an SPI device and I have to comment out that ISR definition.

What I propose is to make this optional by adding define condition

#if !defined(_VARIANT_ARDUINO_ZERO_DISABLE_SERCOM_HANDLERS_)
void SERCOM0_Handler(){  Serial1.IrqHandler();}
void SERCOM5_Handler(){  Serial.IrqHandler();}
#endif

if it make sense, I will send a PR

[matthijskooijman]

Doing this with defines would work when controlling this from the variant, but ideally you would be able to control this from the sketch (i.e. have the handler defined when you use Serial and undefined when you do not). This already works for AVR, see https://github.com/arduino/ArduinoCore-samd/issues/489 for some discussion on achieving the same for SAMD.

I'm not entirely sure if this really solves your usecase, if the variant needs to force that e.g. SERCOM5 is used for SPI rather than serial, then maybe some define-based solution makes sense, though I would think something more fine grained that can disable individual handlers (and serial objects, if only the handler is disabled but not the object, then you'd end up with a non-functioning serial object), or even peform arbitrary mappings between serial objects and SERCOM objects (but I'm not too familiar with the samd code, so I'm not sure if this would be at all feasible).

[maxlem]

if only the handler is disabled but not the object, then you'd end up with a non-functioning serial object

Yeah that's true, I though so after my comment

So here's another take

#if !defined(_VARIANT_ARDUINO_ZERO_DISABLE_SERCOM_HANDLERS_)
Uart Serial1( &sercom0, PIN_SERIAL1_RX, PIN_SERIAL1_TX, PAD_SERIAL1_RX, PAD_SERIAL1_TX ) ;

Uart Serial( &sercom0, PIN_SERIAL1_RX, PIN_SERIAL1_TX, PAD_SERIAL1_RX, PAD_SERIAL1_TX ) ;

void SERCOM0_Handler()
{
  Serial1.IrqHandler();
}

void SERCOM5_Handler()
{
  Serial.IrqHandler();
}

#endif

with that version, if you try to use Serial you're greeted with

main.cpp:(.text.setup+0x28): undefined reference to `Serial'

but if you don't, it builds and links just fine (no warnings)

I went to read the other feature request, which I'm all in favor, but it doesn't fix the fact that a SERCOM#_Hanlder() has been defined.

I think both patches are complementary.

Also, can you already send me a CLA, I don't want to wait 16 months!

[matthijskooijman]

I went to read the other feature request, which I'm all in favor, but it doesn't fix the fact that a SERCOM#_Hanlder() has been defined.

It should, the idea is that if you don't use a particular Serial object, than its storage and its interrupt handler is not included in the link, so can be defined elsewhere.

[maxlem]

oh, I must have missed that part, so the other idea would imply that the instances and definitions are retired from variant.cpp. Then that fixes my issue

[matthijskooijman]

Yeah, the would be moved to separate files per instance.

Though I now see that indeed that these handlers are currently defined in variant.cpp, not in the core itself, so that might need some more significant changes than with the AVR core, since maybe this then needs for a different way for the variant to specify which serial handlers it needs to use (or maybe the same trick linking the serial object to the ISR definition could also be applied inside the variant itself).

[maxlem]

Well, I'm all in support of the idea, but this is way out my time budget for now, so I went on and created this PR