Megunolink / MLP

Arduino library for sending data to MegunoLink visualisers and useful components
http://www.megunolink.com
GNU Lesser General Public License v3.0
46 stars 26 forks source link

Fix compilation for ArduinoCore-mbed based cores #13

Closed maxgerhardt closed 1 year ago

maxgerhardt commented 1 year ago

Fix #12 by

  1. renaming ArduinoTimer class and files to MegunoArduinoTimer
  2. #include <Arduino.h> instead of #include "Print.h" in Formatting.h
PaulMartinsen commented 1 year ago

Thanks for spotting this problem and taking the time to create the PR. Much appreciated.

I'm happy with solution for second problem. But not keen on breaking backwards compatibility to get ArduinoTimer.cpp to compile when only mbed is affected. Do you know if #include <Arduino.h> is bringing the mbed implementation of ArduinoTimer into the global scope?

I'd consider these alternatives first, if you haven't already:

What do you think?

maxgerhardt commented 1 year ago

Could work. Would need to check it out and regression test compilation (best to do with CI :) )

PaulMartinsen commented 1 year ago

I've done some exploration in the develop branch. It looks like :: can disambiguate our ArduinoTimer in the global scope from the mbed version in the arduino namespace. Or at least it builds for me with a Pico Pi target.

I need to do a bit more testing though. It looks like this may be working because the arduino namespace remains attached to the mbed ArduinoTimer despite the using namespace arduino; in their Arduino.h file. I didn't realize that happened, and/or if it is relying on a particular version of C++. If it checks out, this could be a nice solution that maintains backwards compatibility.

maxgerhardt commented 1 year ago

I see CI has been implemented and fixes like https://github.com/Megunolink/MLP/commit/7027e802361c5a169ce3809d2b6bb8677264e84e on top of more were added to fix compilability -- thus closing.