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 regression for Arduino SAMD boards #246

Merged
merged 1 commit into from
Nov 3, 2018

Conversation

AmedeeBulle
Copy link
Contributor

The fix for #243 introduces a regression for Arduino SAMD boards (MKR series).

This PR checks for both AVR and SAMD architecture.

@CLAassistant
Copy link

CLAassistant commented Oct 29, 2018

CLA assistant check
All committers have signed the CLA.

@johanstokking
Copy link
Member

Hmm, are you sure this include is available on SAMD? Is it even necessary to import? Which reference is broken?

In two other libraries I don't see avr/pgmspace.h imported for this platform;

  1. https://www.mysensors.org/apidocs/AES__config_8h_source.html
  2. https://github.com/adafruit/RTClib/blob/master/RTClib.cpp

@johanstokking johanstokking self-requested a review October 31, 2018 14:04
@johanstokking johanstokking self-assigned this Oct 31, 2018
@AmedeeBulle
Copy link
Contributor Author

It is definitely available for Arduino SAMD boards:

$ find . -name pgmspace.h
./arduino/hardware/samd/1.6.19/cores/arduino/avr/pgmspace.h

It is also needed as it provides stubs to basically ignore the AVR pgmspace macros as the SAMD does not have the memory limitations the AVR has.

Now, strictly speaking you don't need to include it at all, as it is already included in Arduino.h

$ grep  pgmspace arduino/hardware/samd/1.6.19/cores/arduino/Arduino.h
arduino/hardware/samd/1.6.19/cores/arduino/Arduino.h:#include "avr/pgmspace.h"

The fix for #243 (ESP32 support) definitely introduces a regression for SAMD boards, as version 2.5.11 of this library works fine.

This PR pragmatically reverts this change for SAMD, a better approach would have been to specifically check for ESP32 in #243 rather than having a wild card #else for non-AVR architectures, but I don't have an ESP32 environment to validate such solution so I went for a safe approach that won't break anything else.

@johanstokking johanstokking merged commit 70b3648 into TheThingsNetwork:master Nov 3, 2018
@johanstokking
Copy link
Member

OK, thanks for the clarification.

@johanstokking
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants