search

TheThingsNetwork / arduino-device-lib

Arduino Library for TTN Devices
MIT License
206 stars 96 forks source link

Fix regression for Arduino SAMD boards #246

Closed AmedeeBulle closed 5 years ago

AmedeeBulle commented 5 years ago

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

This PR checks for both AVR and SAMD architecture.

CLAassistant commented 5 years ago

CLA assistant check
All committers have signed the CLA.

johanstokking commented 5 years ago

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
AmedeeBulle commented 5 years ago

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 commented 5 years ago

OK, thanks for the clarification.

johanstokking commented 5 years ago

Released in https://github.com/TheThingsNetwork/arduino-device-lib/releases/tag/v2.5.13