Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix forward declaration of HardwareSerial for megaAVR architecture #2006

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

brusherru
Copy link
Contributor

There is no issue.
The problem was reported on the forum: https://forum.xod.io/t/nano-every-upload-issue/4198

Copy link
Contributor

@evgenykochetkov evgenykochetkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixtures are not updated, but otherwise LGTM

@brusherru brusherru force-pushed the fix-megaavr-uart branch 2 times, most recently from ec12240 to 11754af Compare July 9, 2020 08:15
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why workspace/big-patch/__fixtures__/arduino.cpp has changed the node order (is it flanky?), but what’s about the PR itself:

LGTM

Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, no, there’s a better way. Wait for a minute, I’ll explain. For now, I’m vetoing the merge

@nkrkv
Copy link
Member

nkrkv commented Jul 10, 2020

Take a look at the core used by Nano Every (https://github.com/arduino/ArduinoCore-megaavr). Namely, to the file https://github.com/arduino/ArduinoCore-megaavr/blob/master/cores/arduino/UART.h. See:

#include "api/HardwareSerial.h"

using namespace arduino;

What the api/. See README:

Copy or symlink the api folder from the ArduinoCore-API (https://github.com/arduino/ArduinoCore-API) repo.

Go there. Read:

This repository hosts the hardware independent layer of Arduino core.

All Arduino official cores are being ported to the new structure so they take advantage of this single repo.

Including this repo in your existing Arduino core will allow the language to grow and include new features. For backwards compatibility, every revision of this repo will increase ARDUINO_API_VERSION define.

Some cores have been ported to the new structure, for example: megaAVR, nRF52-mbedos, classic AVR, SAMD

Whoa! Some classic cores already use the new API wich has arduino namespace. So, merging this PR as is only fixes a little portion of the problem. It will return again and again as users update their packages.

So the proper way of fixing the issue would be using namespace arduino; at the global level. However, I afraid it’s too wide and can cause problems which are not apparent immediately. For now, a safer workaround would be looking at ARDUINO_API_VERSION macro definition, not ARDUINO_ARCH_MEGAAVR.

@brusherru brusherru requested a review from nkrkv July 10, 2020 12:43
@brusherru
Copy link
Contributor Author

@nkrkv done!

@nkrkv
Copy link
Member

nkrkv commented Jul 10, 2020

The fixtures do not match uart.h

@brusherru brusherru force-pushed the fix-megaavr-uart branch 2 times, most recently from a94ccce to b3f2ec0 Compare July 16, 2020 12:05
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brusherru brusherru merged commit 54b2c07 into master Aug 13, 2020
@brusherru brusherru deleted the fix-megaavr-uart branch August 13, 2020 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants