Name collision with MegaCoreX core

[JChristensen]

Jack, this thread seems vaguely similar to my problem - didn't want to open a new Issue, not sure of github etiquette ...
Using the Megacorex arduino core, i get this:
`
Users/lindsay/Documents/Arduino/libraries/DS3232RTC-master/src/DS3232RTC.cpp:70:0: warning: "RTC_STATUS" redefined
#define RTC_STATUS 0x0F

In file included from /Users/lindsay/Library/Arduino15/packages/arduino/tools/avr-gcc/7.3.0-atmel3.6.1-arduino7/avr/include/avr/io.h:691:0,
from /Users/lindsay/Library/Arduino15/packages/arduino/tools/avr-gcc/7.3.0-atmel3.6.1-arduino7/avr/include/avr/pgmspace.h:90,
from /Users/lindsay/Library/Arduino15/packages/MegaCoreX/hardware/megaavr/1.0.9/cores/coreX-corefiles/api/String.h:30,
from /Users/lindsay/Library/Arduino15/packages/MegaCoreX/hardware/megaavr/1.0.9/cores/coreX-corefiles/api/IPAddress.h:25,
from /Users/lindsay/Library/Arduino15/packages/MegaCoreX/hardware/megaavr/1.0.9/cores/coreX-corefiles/api/Client.h:22,
from /Users/lindsay/Library/Arduino15/packages/MegaCoreX/hardware/megaavr/1.0.9/cores/coreX-corefiles/api/ArduinoAPI.h:29,
from /Users/lindsay/Library/Arduino15/packages/MegaCoreX/hardware/megaavr/1.0.9/cores/coreX-corefiles/Arduino.h:23,
from /Users/lindsay/Library/Arduino15/packages/MegaCoreX/hardware/megaavr/1.0.9/libraries/Wire/src/Wire.h:25,
from /Users/lindsay/Documents/Arduino/libraries/DS3232RTC-master/src/DS3232RTC.cpp:31:
/Users/lindsay/Library/Arduino15/packages/arduino/tools/avr-gcc/7.3.0-atmel3.6.1-arduino7/avr/include/avr/iom4808.h:1979:0: note: this is the location of the previous definition
#define RTC_STATUS _SFR_MEM8(0x0141`

Do you have any wisdom for me? I appreciate your help.

Originally posted by @lmorris99 in #50 (comment)

[bperrybap]

If you move the defines into the class, they will no longer collide with defines from other headers/libraries.
however, if a header from somewhere else that has a colliding symbol define name is included first, there will still be an issue.

It kind of looks like in this case the iom4808.h may be getting included first.
If so, this will be an issue since that define will interfere with the definition of symbol with the same name in the DS3232RTC library even when the symbol is moved to be inside the class.

i.e. cpp substitutes the define/macro string on the name of the symbol inside the class to muck up the symbol definition.

[JChristensen]

Hi @bperrybap,
Actually I was thinking of converting the #defines to const or constexpr within the class.

[bperrybap]

Right, that is what I meant. Move/change the #defines to static const int declarations & definitions inside the class.
similar to the current errCode
They can be public or private as needed.
(I assume most can be private)
When in the class they will never conflict with any other symbol or define outside the library.
If public they can be accessed by a sketch using a fully qualified name such as:
DS3232RTC::SRAM_START_ADDR

However, and this is the big one.
If there is a symbol name collision with a #define name in some other header file that gets included prior to the symbol declaration inside the class definition, it will still cause a problem.

And it looks like there will be an issue for RTC_STATUS since it looks like iom4808.h is included prior to DS3232RTC.h
If that is the case, then moving the defines into the class as const values will not help since cpp will still substitute the macro definition on the variable names that collide with #define symbol names even if in the class.

In other words, you can prevent this code from causing name collision problems in other code but you can't modify this code to prevent other code from causing name collision problems for this code.

i.e. if iom4808 is included before DS3232RTC.h
cpp will replace a line like this inside the class definition:
static const int RTC_STATUS = 0x0F;
with
static const int _SFR_MEM8(0x0141) = 0x0F;
which turns into
static const int (0x0141) = 0xF;
which will not compile.

[JChristensen]

Ah! I hadn't thought it through that far but I see what you mean. That could be a sticky issue in certain situations. Is this one of them though? Is iom4808.h likely to be directly included in many sketches? Am I thinking about that correctly?

Having said that, I only just now installed the MegaCoreX and compiled several of the example sketches from the library without issue. So I don't know what's up with the OP's sketch. I haven't heard back from him; when I do I'll ask to see his sketch.

In fact, if I do try #include <iom4808.h> I get:

rtcTimeTemp:11:10: fatal error: iom4808.h: No such file or directory

I suppose I could use the full path to the file if I really wanted to force the issue.

[JChristensen]

@lmorris99 would you post your sketch please.

[JChristensen]

@lmorris99 also, would you please try compiling the rtcTimeTemp example sketch and advise the results?

[bperrybap]

A sketch is unlikely to include the offending header file.
But it could be included by some other code, like an Arduino core header, or library.
In his trace it looks like DS3232RTC.cpp included <Wire.h> which caused a chain of other headers to be included that eventually caused iom4808.h to be included.

The way to test it is to build the DS3232RTC library with the specific board type that the OP used.

Complicating things is that the IDE will now detect a "newer" version of the AVR gcc tools that may exist in the "packages" area that has been installed using the board manager.
If so, the IDE will use this version of the gcc tools rather than the toolset that came bundled with the IDE.
Who wouldn't always want the newer/latest stuff, right?

IMO, this is a VERY dumb thing to do and can cause all kinds of strange breakages, particularly when multiple versions of the IDE are installed. For example in my case I have 20+ versions of the IDE installed for testing purposes. If a core installs a new version of gcc tools, it can break things in other platforms/cores and in older IDE versions.
The IDE in some cases will also use the core library code from the "newer" platform rather than the bundled code.
I have also seen cases where this breaks things even everything is using the most up to date IDE, and platforms.
The biggest offender of this is the megaAVR platform from Arduino.cc
Even for the simple user with a single IDE installed,
it has broken their standard AVR package that comes bundled with the IDE more than once and they have had to do emergency board package updates to fix it.
There are also some core API changes that can break other 3rd party libraries as well including in other platforms since the newer core can override the bundled core of a compatible h/w.

I have tried to convince them more than once to abandon this capability but they refuse as they see it as a useful feature.
IMO, it is unacceptable that installing a board package can alter the tools used for other boards (not part of that package) that came with their own bundled software and are completely independent of that added new board package.
I.e. why should adding board package Y be able to affect and potentially break the tools and builds for boards in package X?

This makes complete testing quite complicated nearly impossible since now you have test against not only a specific version of the IDE and board manager package for the selected board type, but also take into consider all the other board packages for compatible h/w architectures that may have bee installed since they may include their own tool set that might newer.

i.e. if testing for AVR UNO you have to fully verify that there isn't an issue when other board packages are installed since those other board packages (even if not needed) can alter what s/w gets used when building for the UNO.
And it can be version specific of those other board packages.
So it becomes an impossible testing matrix to fully test that there isn't an issue with all the combinations of what could be installed.

And history has shown that even Arduino.cc can't even keep things straight. As they put out releases of the megaAVR package that has broken 100% of the ability to build code for the standard boards like UNO and Mega boards.

[JChristensen]

The way to test it is to build the DS3232RTC library with the specific board type that the OP used.

I think I did that, and could not recreate the problem. Taking a cue from the iom4808.h file, I did make the assumption that the OP was compiling for ATmega4808; I will confirm if I hear back from him.

At this point, unless we hear back from the OP and I can understand his issue better and hopefully reproduce it, I'll probably assume user error and close this issue. Although I have started cleaning up the #defines in the library, so I may as well follow through with that, it's well worthwhile.

I agree that the ecosystem has become quite complex and with that comes risks. The proliferation of new boards and cores, while on the one hand is nice to see, brings a lot of baggage especially where different architectures are concerned. Most of the time I'm amazed that it works as well as it does. I gave up some time ago trying to keep up with it all.

[JChristensen]

Nothing heard from OP, closing issue.

[bperrybap]

I get the same warnings when building for boards in both the MegaCoreX and the megaAvr core.
i.e.I get the warnings the OP mentioned when building the DS3232RTC library TimeRTC example
I have all compiler warnings enabled in the IDE to ensure no warnings are hidden by the IDE.
Then build for either
board "ATmega4808" in the MegaCoreX core
board "Arduino Nano Every" in the megaAVR core

oddly enough it is just a warning and the code compiles and links. (Didn't try it as don't have the hardware)
I'm used to macro re-definitions creating a compile error vs just a warning.

I wish that the Arduino IDE would ship using -Werror by default

[JChristensen]

I get the same warnings when building for boards in both the MegaCoreX and the megaAvr core.

Hmm, now I can recreate the issue as well. Not sure what happened earlier.
I'm continuing to work on the library.

[JChristensen]

however, if a header from somewhere else that has a colliding symbol define name is included first, there will still be an issue.

That was spot on. The only fix here appears to be renaming RTC_STATUS. That doesn't feel like a very satisfactory solution; more like whack-a-mole. Maybe something like DS32_STATUS. I suppose all the names should be consistent too.

[bperrybap]

You could just undef it before your define for a quick fix. Then work on figuring out a better solution.

One unfortunate issue is that if you move the symbols to const int inside the class, a name collision with a previous #define will create an error vs just a warning.

[JChristensen]

I now have

        static constexpr uint8_t
            DS32_ADDR        {0x68},     // I2C device address
            DS32_SECONDS     {0x00},     // register addresses
            DS32_MINUTES     {0x01},
            DS32_HOURS       {0x02},
            etc etc etc

and I get a good compile for the ATmega4808. I didn't prefix the enums; I figure they are used more, so that would create more backwards compatibility issues. I think I'll just go with this and see what happens.

I'll push my development branch to GitHub once I'm done testing and updating the documentation, in case you'd like to have a look. I appreciate all your input. I knew #define was evil but this is a good example of how bad it really is. The iom4808.h file has over 2000 #defines. What could possibly go wrong!

[bperrybap]

The issue for low level AVR headers is that they are used in C modules which limits some of their alternatives.
They should have used more unique names.

One thing to keep in mind is that constexpr is a C++ 11 thing which I think equates to gcc 4.8.1
While the standard is10 years old, I'm not sure if all the 3rd party cores are using a gcc version that recent.
I did check all the platforms I have installed (15 total). All had 4.8.1 or newer gcc tools.
This included all the arduino.cc platform/cores, as well as these platforms:
chipkit, STM32, DxCore, DigiStump, Microsoft, Intel, esp32, esp8266, rp2040, stm32duino (uses arduino.cc arm tools from the due package)

There might be some old 3rd party less maintained cores like for some of the tiny AVRs that may use an older version of gcc.