Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork140
Remove forceful "using namespace arduino" from headers#144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bxparks left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hi, This is great news. The explicitusing statements seem to provide backwards compatibility with existing forward declarations. As noted above, I had to add theusing arduino::String statement to make the fix more complete.
I validated your 2 PRs,
by switching my git repos to your branches (so basically, I get a samd 1.8.11, plus your changes). I then compiled and uploaded about 70 of my unit tests, across most of my libraries, to an actual MKR1000 that I recently purchased. I had a few hiccups, but eventually got everything working.
There were 3 classes of problems:
Stringforward declarations, fixed by above.- More stringent compiler errors and warnings from the SAMD21 compiler. (Every compiler seems to complain about slightly different things...)
- The breaking change to
::digitalWrite(uint8_t, uint8_t)which is now::digitalWrite(uint8_t, PinStatus)in the new API.- I have to use an explicitly scoped
::digitalWrite()because I have an overloadeddigitalWrite()in that class. But::digitalWrite()does not match yourarduino::digitalWrite()) overload. So I had to insert a handful of#if defined(ARDUINO_API_VERSION)statements.
- I have to use an explicitly scoped
With regards to the 2 PR on my libraries:
I will check in a version that removes that#error for the SAMD21 core. But I want to retain the blacklist for megaAVR platform until I personally verify thatarduino/ArduinoCore-megaavr#62 is fixed on actual hardware. I currently don't have any megaAVR boards, but it's on my shopping list.
I have a question onbxparks/AceTime#57 that you sent: You replaced the#include <Arduino.h> with an#include <arvr/pgmspace.h>. Unfortunately, that breaks the ESP32 platform, where it needs to be#include <pgmspace.h>. I can add some conditional code onARDUINO_API_VERSION in there, or just keep using#include <Arduino.h> in that.cpp file.
I will update this comment when I have submitted the fixes on my end that adds support for arduino:samd >= 1.8.10.
Uh oh!
There was an error while loading.Please reload this page.
bxparks commentedApr 2, 2021
I have prepared a series of PRs, which replace the PRs sent to me by@facchinm:
These are designed to reenable arduino:samd when a new version (1.8.12?) is released. The version number of the arduino:samd Core doesn't matter. Instead, I assume that the I will wait until the new arduino:samd core is pushed out. Then I will re-validate my libraries, and if everything looks right, I will submit my PRs listed above, and everything should be stable again. We can undo the version-pinning of my libraries (arduino/ArduinoCore-samd#607) in the arduino:samd CI workflow if desired. |
First step tofixarduino/ArduinoCore-samd#606