error: multiple definition of 'enum out::output_t'

[acu192]

This error happens if you do both #include <Comparator.h> and #include <Logic.h> in the same .cpp (or .ino) file.

Here is the error in more detail:

In file included from mytest.ino:2:0:
.../MegaCoreX/hardware/megaavr/1.0.10/libraries/Logic/src/Logic.h:163:8: error: multiple definition of 'enum out::output_t'
   enum output_t : uint8_t {
        ^~~~~~~~
In file included from mytest.ino:1:0:
.../MegaCoreX/hardware/megaavr/1.0.10/libraries/Comparator/src/Comparator.h:8:8: note: previous definition here
   enum output_t : uint8_t
        ^~~~~~~~

A workaround is to not include both header files in a single compilation unit. Rather, I now have one .cpp that includes Comparator.h and another .cpp that includes Logic.h. Not so bad... but maybe not something all users would know to do.

The solution (I think) is easy -- just rename one of the enums?

[SpenceKonde]

I've got tons of people complaining about trhis too and don';t know how to solve without breaking code.

Renaming an enum WILL BREAK EXISTING CODE

[SpenceKonde]

@MCUdude can you please lend a hand here?

[acu192]

Yes, it'll break code in certain situations, however the common-use-case is something like this:

Logic0.output = out::disable;

or

Comparator.output = out::enable;

Such (common) uses don't reference the name of the enum, so it can be changed transparently in these cases.

Yes, other cases (where the enum name is referenced) will break, however... to be fair... the name of the enum is undocumented, so such folks will have only seen it by looking at the source code. Such folks will be knowledgeable enough to fix their code if the name of the enum changes.

On the other hand, people running into the error above may not be knowledgeable enough to overcome it. So, in the utilitarian view, we should change the name of the enum to help the most people.

[MCUdude]

I haven't figured out a way to solve this without introducing breaking changes, sorry.

Yes, it'll break code in certain situations, however the common-use-case is something like this:
Logic0.output = out::disable;
or
Comparator.output = out::enable;
Such (common) uses don't reference the name of the enum, so it can be changed transparently in these cases.

@acu192 I'm not sure what I understand. The names of the enums aren't critical, as these aren't referenced directly, as you pointed out. However, I can't get it to work even if I rename the enums so that they have unique names.

Logic.h:

namespace out {
  enum output_logic_t : uint8_t {
    disable  = 0x00,
    enable   = 0x01,
  };
  enum pinswap_t : uint8_t {
    no_swap  = 0x00,
    pin_swap = 0x01,
  };
};

Comparator.h

namespace out
{
  enum output_comp_t : uint8_t
  {
    disable         = 0x00,
    disable_invert  = 0x80,
    enable          = 0x40,
    invert          = 0xC0,
    enable_invert   = 0xC0,
  };
};

The following code still doesn't compile:

#include <Logic.h>
#include <Comparator.h>

void setup()
{
  Logic0.output = out::enable;
  Comparator.output = out::enable;
}

void loop()
{
}

Error:

In file included from /var/folders/6l/ypg6qbw172v1s4vtt6g990tw0000gn/T/arduino_modified_sketch_497526/Five_input_NOR.ino:19:0:
/Users/hans/Documents/Arduino/hardware/MegaCoreX/megaavr/libraries/Comparator/src/Comparator.h:10:23: error: redeclaration of 'disable'
     disable         = 0x00,
                       ^~~~
In file included from /var/folders/6l/ypg6qbw172v1s4vtt6g990tw0000gn/T/arduino_modified_sketch_497526/Five_input_NOR.ino:18:0:
/Users/hans/Documents/Arduino/hardware/MegaCoreX/megaavr/libraries/Logic/src/Logic.h:164:5: note: previous declaration 'out::output_logic_t disable'
     disable  = 0x00,
     ^~~~~~~
In file included from /var/folders/6l/ypg6qbw172v1s4vtt6g990tw0000gn/T/arduino_modified_sketch_497526/Five_input_NOR.ino:19:0:
/Users/hans/Documents/Arduino/hardware/MegaCoreX/megaavr/libraries/Comparator/src/Comparator.h:12:23: error: redeclaration of 'enable'
     enable          = 0x40,
                       ^~~~
In file included from /var/folders/6l/ypg6qbw172v1s4vtt6g990tw0000gn/T/arduino_modified_sketch_497526/Five_input_NOR.ino:18:0:
/Users/hans/Documents/Arduino/hardware/MegaCoreX/megaavr/libraries/Logic/src/Logic.h:165:5: note: previous declaration 'out::output_logic_t enable'
     enable   = 0x01,
     ^~~~~~
In file included from /var/folders/6l/ypg6qbw172v1s4vtt6g990tw0000gn/T/arduino_modified_sketch_497526/Five_input_NOR.ino:19:0:
/Users/hans/Documents/Arduino/hardware/MegaCoreX/megaavr/libraries/Comparator/src/Comparator.h:76:38: error: cannot convert 'out::output_logic_t' to 'out::output_comp_t' in initialization
     out::output_comp_t output = out::disable;
                                      ^~~~~~~
/var/folders/6l/ypg6qbw172v1s4vtt6g990tw0000gn/T/arduino_modified_sketch_497526/Five_input_NOR.ino: In function 'void setup()':
Five_input_NOR:24:28: error: cannot convert 'out::output_logic_t' to 'out::output_comp_t' in assignment
   Comparator.output = out::enable;
                            ^~~~~~
Using library Logic at version 1.1.1 in folder: /Users/hans/Documents/Arduino/hardware/MegaCoreX/megaavr/libraries/Logic 
Using library Comparator at version 1.0.0 in folder: /Users/hans/Documents/Arduino/hardware/MegaCoreX/megaavr/libraries/Comparator 
exit status 1
cannot convert 'out::output_logic_t' to 'out::output_comp_t' in assignment

[acu192]

@MCUdude Oh gosh. I did not expect changing the enum name would cause that error 😔. Yeah, the way C/C++ does enums is very weird...

So, this will be much harder to solve...

I suppose the only way to solve this will be to create an entirely new interface for either Logic or Comparator, and to leave the existing one as-is so we don't break anyone's existing code. The implementation of that entirely new interface can call into the old one (so there is no duplicated code). Documentation can change to describe the entirely new interface (while the old interface will continue working, it will become undocumented). Kinda a lot of work for solving this... Is it worth it? Thoughts?

PS: Thank you for putting together this library. Using it has saved me a lot of time, and I'm very thankful.

[MCUdude]

I don't think there's a way around this without breaking changes.
However, we can try to make the changes as small as possible.
@SpenceKonde how about renaming the Comparator out namespace to output instead? The code will look like this:

namespace output
{
  enum output_t : uint8_t
  {
    disable         = 0x00,
    disable_invert  = 0x80,
    enable          = 0x40,
    invert          = 0xC0,
    enable_invert   = 0xC0,
  };
};

The following code compiles:

#include <Logic.h>
#include <Comparator.h>

void setup()
{
  Logic0.output = out::enable;
  Comparator.output = output::enable;
}

void loop()
{
}

[MCUdude]

On the other side, now all these libraries have all these constants, it might be difficult to distinguish them from each other.
Another alternative is to wrap all library constants in their own namespaces.

logic for the logic library
comparator for the comparator
opamp for the opamp library
zcd for the Zerocross library
event for the event library (?)

More verbose, breaking changes for all libraries, but very straightforward and easy to understand. There's very little changes of future naming conflicts too:

  Logic0.output = logic::out::enable;
  Comparator.output = comparator::out::enable;

Tempting...

[MCUdude]

@SpenceKonde how about renaming the Comparator out namespace to output instead?

This will work for MegaCoreX because there are only two conflicting libraries, Logic, and Comparator. DxCore on the other hand has multiple libraries with naming conflicts (sorry...), so adding another namespace (logic::out::enable) is a better solution IMO.

[SpenceKonde]

What is the syntax for wrapping namespaces in a second namespace like you you describe in the past two comments? remembver, I know C, I do not know C++!

[MCUdude]

What is the syntax for wrapping namespaces in a second namespace like you describe in the past two comments?

Nested namespaces. Just put all the existing namespaces inside another one, as shown below. But what are your thoughts on adding another namespace, and the "new" syntax: comparator::in_p::in0?

// Comparator.h, MegaCoreX
namespace comparator
{
  namespace out
  {
    enum output_t : uint8_t
    {
      disable         = 0x00,
      disable_invert  = 0x80,
      enable          = 0x40,
      invert          = 0xC0,
      enable_invert   = 0xC0,
    };
  };

  namespace hyst
  {
    enum hysteresis_t : uint8_t
    {
      disable = 0x00, // No hysteresis
      small   = 0x02, // 10 mV
      medium  = 0x04, // 25 mV
      large   = 0x06, // 50 mV
    };
  };

  namespace in_p
  {
    enum inputP_t : uint8_t
    {
      in0    = 0x00,
      in1    = 0x01,
      in2    = 0x02,
      in3    = 0x03,
    };
  };

  namespace in_n
  {
    enum inputN_t : uint8_t
    {
      in0    = 0x00,
      in1    = 0x01,
      in2    = 0x02,
      dacref = 0x03,
    };
  };

  namespace ref
  {
    enum reference_t : uint8_t
    {
      vref_0v55 = 0x00, // 0.55V
      vref_1v1  = 0x01, // 1.1V
      vref_1v5  = 0x04, // 1.5V
      vref_2v5  = 0x02, // 2.5V
      vref_4v3  = 0x03, // 4.3V
      vref_avcc = 0x07, // Vcc
      disable   = 0x08,
    };
  };
};

I know C, I do not know C++!

I don't "know" C++ either. Since I'm only using it on microcontrollers, I'm really only using "C with classes". This means no c++ standard library (std), no dynamic memory allocation, etc. Even though my codes and libraries are like 90% C code, it's nice to be able to them in a C++ flavored package. C++ offers some convenient "library features" like function overloading, templates and namespaces.

[SpenceKonde]

Hmmmmm

Hey - I just got a brilliant idea.... say we have a second header that contains the types that have competition. in a manner that covers all parts?

Then each library.h could include this header to bring in types like out, pinswap, but they'd have an includeguard and never fight within a single compilation unit? And this would require no changes to user code!!

[MCUdude]

Well, one thing is inside the libraries. That's the easy part. But how about the user program where both libraries are present simultaneously?

The compiler just doesn't know what to do when out::enablehas two different separate declarations.

#include <Logic.h>
#include <Comparator.h>

void setup()
{
  Logic0.output = out::enable;
  Comparator.output = out::enable;
}

void loop()
{
}

Feel free to give it a try, but I doubt it will work in a .ino file. I'd love to be proven wrong though.

[MCUdude]

I outlined above a way to fix this without breaking existing code, but no interest was taken in that approach. It involves maintaining a set of "old" and "new" header files, and changing the documentation to only talk about the "new" ones.

The problem with this approach is that as long as the old names are present and defined in two separate files, there is no way to resolve the issue without adding another namespace or using enum classes I think. One of the conflicting enums has to be renamed/removed in either the Logic or Comparator library.

My time is limited at the moment, so I don't have much time for experimenting with alternative solutions at the moment.

AFAIK, the most common way of dealing with naming conflicts in libraries in C++ is to include the library inside the namespace, and it. However, this isn't a "real" solution in this case:

#include <Logic.h>
namspace comparator
{
  #include <Comparator.h>
};

void setup()
{
  Logic0.output = out::enable;
  comparator::Comparator.output = comparator::out::enable;
}

void loop()
{
}

[MCUdude]

OK, I think I found a solution that introduces a new namespace (logic or comparator), but the old names can still be used, with the naming conflicts we know from today. Here's the library code from Comparator.h

This means that comparator::out::enable is the same thing as out::enable.

I suggest we update our documentation to refect these new names (out::enable -> logic::out::enable or comparator::out::enable), and don't document the "legacy" names to prevent new users from using them.
Are you OK with this fix @SpenceKonde?

namespace comparator
{
  namespace out
  {
    enum output_t : uint8_t
    {
      disable         = 0x00,
      disable_invert  = 0x80,
      enable          = 0x40,
      invert          = 0xC0,
      enable_invert   = 0xC0,
    };
  };

  namespace hyst
  {
    enum hysteresis_t : uint8_t
    {
      disable = 0x00, // No hysteresis
      small   = 0x02, // 10 mV
      medium  = 0x04, // 25 mV
      large   = 0x06, // 50 mV
    };
  };

  namespace in_p
  {
    enum inputP_t : uint8_t
    {
      in0    = 0x00,
      in1    = 0x01,
      in2    = 0x02,
      in3    = 0x03,
    };
  };

  namespace in_n
  {
    enum inputN_t : uint8_t
    {
      in0    = 0x00,
      in1    = 0x01,
      in2    = 0x02,
      dacref = 0x03,
    };
  };

  namespace ref
  {
    enum reference_t : uint8_t
    {
      vref_0v55 = 0x00, // 0.55V
      vref_1v1  = 0x01, // 1.1V
      vref_1v5  = 0x04, // 1.5V
      vref_2v5  = 0x02, // 2.5V
      vref_4v3  = 0x03, // 4.3V
      vref_avcc = 0x07, // Vcc
      disable   = 0x08,
    };
  };
};

// Legacy definitions
namespace out  { using namespace comparator::out;  };
namespace hyst { using namespace comparator::hyst; };
namespace in_p { using namespace comparator::in_p; };
namespace in_n { using namespace comparator::in_n; };
namespace ref  { using namespace comparator::ref;  };

Here's a few examples:

#include <Comparator.h>

void setup()
{
  // Works
  Comparator.output = out::enable;
  // Works too
  Comparator.output = comparator::out::enable;
}

Does not work due to out:: naming conflict:

#include <Comparator.h>
#include <Logic.h>
void setup()
{
  // Does not work
  Comparator.output = out::enable;
  // Works
  Comparator.output = comparator::out::enable;
}

Works

#include <Logic.h>
#include <Comparator.h>

void setup()
{
  Logic0.output = logic::out::enable;
  Comparator.output = comparator::out::enable;
}

[acu192]

@MCUdude YES YES YES! That is perfect.

[MCUdude]

@SpenceKonde any thoughts?

[MCUdude]

Well, I think that the solution I provided with "legacy definitions" is the best one yet. Update documentation and examples to include the extra namespace, but keep the old ones for compatibility.

As soon as @SpenceKonde confirms that he will follow the same approach for his libraries (Comparator, Event, Logic, Opamp and ZCD), I'll push a fix for this core.

[MCUdude]

I've now pushed a fix that affects Comparator, Event, and Logic. Legacy names are still supported, but the new ones are recommended for future use as these don't conflict with each other.

[SpenceKonde]

Wow, I'm amazed that you can have duplicate namespaces aslong as you don't use them

[SpenceKonde]

I've checke d in changes to Comparator, Event, and Logic, (note Event also contains critcial fixes for tinyAVR parts) Bumped version to 1.3.0 on all libraries, (1.2.1 event was just the critical fixes for tinyAVR) to megaTinyCore.

These will be synced to DxCore and similar changes applied to Opamp and ZCD there.