NeoTrellis callback registration does not compile

[ukmaker]

NeoTrellis.h defines three methods for registering a callback with NeoTrellis and MultiTrellis. The method signatures are such that this does not compile for me using PlatformIO.

A signature such as void registerCallback(uint8_t key, TrellisCallback (*cb)(keyEvent)); Should be rewritten to void registerCallback(uint8_t key, TrellisCallback cb); And the _callbacks[] array adjusted to suit.

I wonder how this can compile at all?

[ladyada]

hmm does it work with arduino ide? we dont use platformio

[ukmaker]

It does work with Arduino IDE, but I need a debugger :-( To me, it just looks like that's not the way the method declaration should be - TrellisCallback is already defined as a function pointer:typedef void (*TrellisCallback)(keyEvent evt);

[TriviumGames]

Like a year ago something changed in the esp32 toolchain where if a function declared a non-void return type, but didn't return anything, returning would crash (with illegal instruction), even if nobody used the result. There's some debate as to whether this is a compiler bug here. But even if it is a compiler bug it's a fairly longstanding one that affects this library in a way that is pretty hard to troubleshoot. The examples don't work, code that used to work with a pre-2.0 esp32 toolchain no longer works, and when it fails you just get a callstack just shows the crash as being in the callback when it returns, even if the callback doesn't do anything bad at all.

This is pretty unfriendly behavior, and I don't really see how someone without a very strong C++ background would be able to troubleshoot it. (The workaround, btw, is to just return 0; at the end of your callback.)

I absolutely agree with ukmaker that the Neotrellis library uses function pointer typedefs incorrectly. It demands we pass in a function that returns a TrellisCallback, not a function that is a TrellisCallback. And then the examples define those, but never actually get around to returning that.

That said, I don't see any practical way to fix the API that doesn't break all existing client code. My request/recommendation would be to just add the "return 0;" to all the examples. I'm mixed on whether it would be helpful or just confusing to document why this is there, since it's basically totally nuts and the ESP toolchain folks position of "this is standards compliant so I don't care if it breaks real wold code" is clearly wrong, but at least it gives people a path to success - the examples work, and at least someone can line-by-line compare them to their own code to find out what causes the crash.