bxparks / AceButton

An adjustable, compact, event-driven button library for Arduino that debounces and dispatches events to a user-defined event handler.
MIT License
385 stars 37 forks source link

use of __FlashStringHelper is ambiguous #120

Closed geoffreyheller closed 1 year ago

geoffreyheller commented 1 year ago

Hello,

Love your AceButton library!

One minor issue with the 1.10.0 release: In AceButton.h you define class __FlashStringHelper in the global namespace, but in AceButton.cpp you define it in the ace_button namespace.

Depending on how restrictive your compiler is, this may result in a compiler error. In my case I'm seeing this: .pio/libdeps/debug/AceButton/src/ace_button/AceButton.h:127:12: error: reference to '__FlashStringHelper' is ambiguous

The issue can be fixed by moving __FlashStringHelper into the ace_button namespace in AceButton.h

bxparks commented 1 year ago

Can you try removing line#26

class __FlashStringHelper;

in AceButton.h? I think the problem is that your board is using the new ArduinoCore-API, which breaks backwards compatibility in many ways, one of them is moving __FlashStringHelper into the arduino:: namespace.

I don't use PlatformIO so cannot verify.

geoffreyheller commented 1 year ago

I can confirm I'm using the ArduinoCore-API. I'm doing rp2040 development on PlatformIO so I'm necessarily using newer libraries.

I am able to comment out the forward declare for __FlashStringHelper in AceButton.h and get a successful build, so that's a solution for now.

Not sure if there's a great general solution for this issue.

It might be necessary to do an #ifdef check for ArduinoCore-API in AceButton.h and set up the forward declare namespace accordingly.

bxparks commented 1 year ago

For some reason, GitHub sent an email with your previous reply, but there is no record of it here:

That's a possibility. I can comment out line 26 in Acebutton.h and I can build ok class FlashStringHelper; It is possible this means FlashStringHelper is defined elsewhere.

That said, it is ambiguous to be referencing __FlashStringHelper on lines 79 and 83 in AceButton.cpp while in the ace_button namespace while defining the __FlashStringHelper in AceButton.h in the global namespace.

If that inconsistency is intentional then perhaps add a comment explaining why it is important.

I think you are confused about the code in AceButton.cpp. All of that code is within the ace_button:: namespace, so there is no additional redeclaration of __FlashStringHelper.

The problem is that ArduinoCore-API redefines __FlashStringHelper (and all other previous Arduino classes) into the new arduino:: namespace, then adds a using arduino::__FlashStringHelper statement to pollute the global namespace anyway, in an attempt to provides backwards compatibility, which then causes conflicts with any other previous application-level declarations which used the previously valid declaration of __FlashStringHelper in the global namespace.

Removing the statement in AceButton.h is the actual solution here, because I forgot that I include #include <Arduino.h> at the top of that file, so I don't need my own declaration of __FlashStringHelper. I will make that change shortly.

geoffreyheller commented 1 year ago

Sorry about that. I realized I misunderstood your earlier comment and edited my comment. Sorry if I caused some confusion there.

Thanks for responding. I think your proposed solution sounds good.

bxparks commented 1 year ago

With regards to your suggestion:

It might be necessary to do an #ifdef check for ArduinoCore-API in AceButton.h and set up the forward declare namespace accordingly.

I've made a conscious decision to not support the ArduinoCore-API. It's not worth my time to track down all the undocumented backwards compatible breaking changes in that they made. AceButton accidentally works with the ArduinoCore-API because it is so simple. Well.. I guess even this library requires additional time and effort on my part to keep it compatible, something I promised myself I wouldn't waste my time doing...

bxparks commented 1 year ago

Released v1.10.1 with this fix. Enjoy!

geoffreyheller commented 1 year ago

Thank you for taking care of this so quickly!