ISR definitions create symbol conflict

[ezahavi]

I lately run into an issue when in the need to declare my own PCI ISR (of unused pins) that caused compilation (on UNO) to abort due to duplicate symbol of the specific _vector.

I have seen several issues that describe such conflicts (multiple definitions of _vector) caused by declarations of the same ISRs within multiple sources. E.g.:
arduino/ArduinoCore-avr#419,
arduino/ArduinoCore-avr#378
https://forum.arduino.cc/t/disable-hwserial-library-at-compile-time/164625
https://forum.arduino.cc/t/error-multiple-definition-of-__vector_13/579477

I have seen that some of the solutions were using __attribute(weak) to enable overriding of the ISR by non weak declarations.
However, I think this method is "weak" by itself. Since we need to support user code override of ISRs we will need to make all AVR definition of the ISRs weak. And then rely on linking order?

I claim that a good solution should not do that. Instead we should treat ISR declarations as interface/API of any module that declares them. Since only the pre-processor can avoid code segments, and in the restriction of not changing current default behavior, we will need to provide optional "define" statements to disable ISR code from being included.

For example from SoftwareSerial.c Instead of:

#if defined(PCINT0_vect)
ISR(PCINT0_vect) {   SoftwareSerial::handle_interrupt(); }
#endif

Use:

#if defined(PCINT0_vect)
#ifndef (SWSERIAL_NO_PCINT0)
ISR(PCINT0_vect)
{
  SoftwareSerial::handle_interrupt();
}
#endif
#endif

Some further complication may arise from SoftwareSerial using the same ISR for all by using ISR_ALIASOF.
If that is an important optimization, it is possible to use more complex logic to decide if to use the alias or it is the first ISR defined.

[matthijskooijman]

The current approach, which works reasonably well at least for AVR, is to write code in such a way that ISRs are only actually defined when they are (potentially) used. For example, Serial ISRs will only be defined if you reference the corresponding SerialX object from your code, and PCINT ISRs are only defined when you include SoftwareSerial in your sketch.

However, this method does have its limits, i.e. IIRC SoftwareSerial defines all PCINT ISRs, since it cannot know automatically at compiletime which pins you might be using (which might sometimes be fixed at compiletime but hard to detect within C/C++, and sometimes it might even be only known at runtime). So some improvement in this area would be nice, which would indeed mean that a sketch should explicitly state to a library which ISRs should (not) be defined (or ideally, just which pins it may or may not use, but it is probably challenging to reliably translate those pin numbers to ISRs at compiletime/in the preprocessor).

One way to specify this to libraries would indeed by to use preprocessor defines as you suggest. However, there is currently no good way for a sketch to pass preprocessor defines to libraries (or anywhere else), so generally adding such preprocessor options is not done much (though it is done in some cases, e.g. for serial buffer sizes IIRC). Some care must be taken to keep a good overview of such defines, since they do become part of the public API for a core or library.

Ideally, I would like to see that cores and libraries can define public options that the sketch can set, and where the library can map these options to preprocessor macros (or maybe other compiler flags), so the defined options become the public API instead of the preprocessor macros themselves, but that's a bit of a different discussion....

In any case, since your issue seems to be specifically about the AVR SoftwareSerial library, I think it should be moved to https://github.com/arduino/ArduinoCore-avr. Or did you intend this as a more generic discussion?

[ezahavi]

Thanks for your reply. I work around my current issue by commenting the ISRs I do not use in SoftwareSerial.cc, so this is not urgent for me. I understand your comment regarding the inclusion of the code in SoftwareSerial.cc making it hard to change it from the sketch. My inclination would then be to move the declarations into pragma(once) section in the SoftwareSerial.h. It would make them compile as part of the sketch and thus would be affected by any defines preceding the include statement. I agree with your ideal target, but I think there is no way to control ISR registration at runtime. If there was, then we could have provided a better API. Maybe we could provide an indirection struct (similar to virtual table) based generic ISRs that a user would be able to update the function pointers at runtime. But that is somewhat a performance waste. I would stick to documenting at the beginning of each module.h the optional defines that prevent ISR declaration. If there is support for my proposal I will prepare a pull request (proposal is to move all ISRs into to a "once" section and add optionally defined SWSERIAL_NO_ISR1... defines)

…

On Fri, Oct 1, 2021, 18:47 Matthijs Kooijman ***@***.***> wrote: The current approach, which works reasonably well at least for AVR, is to write code in such a way that ISRs are only actually defined when they are (potentially) used. For example, Serial ISRs will only be defined if you reference the corresponding SerialX object from your code, and PCINT ISRs are only defined when you include SoftwareSerial in your sketch. However, this method does have its limits, i.e. IIRC SoftwareSerial defines *all* PCINT ISRs, since it cannot know automatically at compiletime which pins you might be using (which might sometimes be fixed at compiletime but hard to detect within C/C++, and sometimes it might even be only known at runtime). So some improvement in this area would be nice, which would indeed mean that a sketch should explicitly state to a library which ISRs should (not) be defined (or ideally, just which pins it may or may not use, but it is probably challenging to reliably translate those pin numbers to ISRs at compiletime/in the preprocessor). One way to specify this to libraries would indeed by to use preprocessor defines as you suggest. However, there is currently no good way for a sketch to pass preprocessor defines to libraries (or anywhere else), so generally adding such preprocessor options is not done much (though it is done in some cases, e.g. for serial buffer sizes IIRC). Some care must be taken to keep a good overview of such defines, since they do become part of the public API for a core or library. Ideally, I would like to see that cores and libraries can define public options that the sketch can set, and where the library can map these options to preprocessor macros (or maybe other compiler flags), so the defined options become the public API instead of the preprocessor macros themselves, but that's a bit of a different discussion.... In any case, since your issue seems to be specifically about the AVR SoftwareSerial library, I think it should be moved to https://github.com/arduino/ArduinoCore-avr. Or did you intend this as a more generic discussion? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#11674 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2PG5BC3TFHIC7XNDI62L3UEXJZHANCNFSM5FEZT6KQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[matthijskooijman]

My inclination would then be to move the declarations into pragma(once)
section in the SoftwareSerial.h.
It would make them compile as part of the sketch and thus would be affected
by any defines preceding the include statement.

This could work, but would require a bit of a onorthodox approach: You would need to define (rather than declare) stuff (the ISRs) in the .h file. When compiling, the ISRs would then be defined in the compilatoin unit that includes the file. If multiple compilation units (.ino/.cpp files) include the header file, then the ISR would end up being defined multiple times and get a linker error. Since including SoftwareSerial.h must be includable from multiple places, you would need to move the ISRs to a different .h file, which must then be guaranteed (by the user) to be included just once, which breaks compatibility and is quite fragile, so I'm not sure if this would be an appropriate API for Arduino...

[ezahavi]

You have a very good point about why we can't just put the code in the header file as it may be included in multiple compiled objects. Problem is that all ISRs are defined in the library object so without passing defines, and forcing new compilation to the library, they will be declared as ISRs. Makes me think the one way to make this happen is to provide ways to pass defined symbols on the compile of the modules and track them in the build system. Very complicated. Another option is to take ALL ISR defines from the modules and require their definition in the sketch or a special module. But this cannot be done in backward compatibility mode. I ran out of options. I need to learn more about the Arduino application builtin build system.

…

On Sun, Oct 3, 2021 at 12:25 PM Matthijs Kooijman ***@***.***> wrote: My inclination would then be to move the declarations into pragma(once) section in the SoftwareSerial.h. It would make them compile as part of the sketch and thus would be affected by any defines preceding the include statement. This could work, but would require a bit of a onorthodox approach: You would need to *define* (rather than *declare*) stuff (the ISRs) in the .h file. When compiling, the ISRs would then be defined in the compilatoin unit that includes the file. If multiple compilation units (.ino/.cpp files) include the header file, then the ISR would end up being defined multiple times and get a linker error. Since including SoftwareSerial.h must be includable from multiple places, you would need to move the ISRs to a different .h file, which must then be guaranteed (by the user) to be included just once, which breaks compatibility and is quite fragile, so I'm not sure if this would be an appropriate API for Arduino... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#11674 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2PG5GASDAV3TIGGSRA4ITUFAOR3ANCNFSM5FEZT6KQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.