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

Feature/arduino compatibility regression #1283

Merged

Conversation

@m-mcgowan
Copy link
Contributor

commented Mar 27, 2017

Attempts to restore 0.6.0-style arduino compatibility as outlined by issue #1278

In a nutshell:

  • the default is 0.6.0 Arduino compatibility - let's call this partial compatibility.
  • applications and libraries include "Arduino.h" to define ARDUINO and bring in many more additional defines to complete the Arduino API. We'll call this full compatibility.

Mixing libraries that require partial and full Arduino compatibility

Libraries that require full Arduino compatibility may include Arduino.h in their library header file, while libraries that depend on partial compatibility (because they themselves define missing symbols) don't include Arduino.h. If a library requiring partial compatibility is included after one requiring full compatibility then the partial compatibility library may break, since Arduino symbols that it's not expecting will be defined.

The workaround for this is to include all libraries requiring partial compatibility before libraries requiring full compatibility in the application file.


Doneness:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • API tests compiled
  • Run unit/integration/application tests on device
  • Add documentation
  • Add to CHANGELOG.md after merging (add links to docs and issues)

ENHANCEMENT / BUG FIX

@m-mcgowan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

Internal Design

  • The partial Arduino API as it was in 0.6.0 is defined in spark_wiring_arduino.h. This file has been restored to it's former 0.6.0 state without the new Arduino APIs. This file is always included with Particle.h/application.h.

  • The full Arduino API is declared in "Arduino.h". By keeping it separate we make a clear distinction between which symbols are included in the partial API and the full API.

  • There is some overlap of the partial API and the full API - constrain and range are examples. In the partial API, these are declared as templates. In the full API, they are macros. Hence, the usual trick of using #ifndef constrain won't work in the full API since it's defined as a template and not a macro. The workaround is to use PARTICLE_WIRING_ARDUINO_TEMPLATE as a define when these few common symbols have already been defined in the partial API, so they are not re-defined in the full API. If we are splitting hairs, this may mean that some arduino libraries fail to compile in cases where the template does not behave the same as the macro (or that they are relying upon double-evaluation side-effects of the macro!!). In this rare (hopefully non-existent) case, it is possible to get the arduino version of these defines, by disabling the preprocessor, and including Arduino.h at the top of the sketch.

@technobly
Copy link
Member

left a comment

should this be #if PARTICLE_WIRING_ARDUINO_COMPATIBILITY == 1 just in case PARTICLE_WIRING_ARDUINO_COMPATIBILITY is defined as 0 by the user for some reason? Not even sure if that's a valid use case, but from Arduino.h it seems like it's supported.

@monkbroc

This comment has been minimized.

Copy link
Member

commented Mar 29, 2017

Did you check what happens when multiple include files are included? Are the Arduino symbols defined "as expected"?

For example:

#include "some_library.h"
#include "Arduino.h"

and some_library.h includes Particle.h.

Or

#include "library1.h" // includes "Particle.h"
#include "library2.h" // includes "Arduino.h"

It can get hairy with symbol redefinitions in libraries so it would be fine if some cases don't work, but it would be nice if the easy case (both Arduino.h and Particle.h included, libraries don't redefine Arduino symbols) works.

@@ -137,7 +137,7 @@ class SPISettings : public Printable {
}

#ifdef PARTICLE_WIRING_ARDUINO_COMPATIBILITY
typedef particle::SPISettings SPISettings;
typedef particle::__SPISettings __SPISettings;

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 3, 2017

Member

Should this be typedef particle::__SPISettings SPISettings?

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Apr 3, 2017

Author Contributor

yes...good catch. I had the IDE do the renaming...

@m-mcgowan m-mcgowan force-pushed the feature/arduino_compatibility_regression branch from 780000e to a4229e1 Apr 3, 2017

m-mcgowan added 13 commits Mar 27, 2017
adds demarkation for functions already defined by particle as templat…
…es so they are not redefined as macros later. If we are splitting hairs, this could be a potential compatibility issue when porting Arduino libraries, in cases where the macro behaves differently from the template, since our version is included by the particle preprocessor for .ino files. The workaround is to disable the preprocessor and then include “Arduino.h” to get the arduino macros rather than the particle templates. I mention this only as a possibility and hope it is seldom needed in practice.
ensures that SPISettings (a new Arduino API) is only available when A…
…rduino.h is included to prevent clashes with libraries that define this.
remove “active_object.h” from application headers since this stops th…
…e C functions `isnan`, `isinf` from being available.
makes the arduino compatibility symbols always defined. Adds a test f…
…or SPISettings API and arduino dependency.

@technobly technobly force-pushed the feature/arduino_compatibility_regression branch from a4229e1 to 1292314 Apr 3, 2017

@technobly

This comment has been minimized.

Copy link
Member

commented Apr 3, 2017

Github can't tell if the changes I requested were implemented... but I think they were?

force push kind of ruined the review linking -_-

@technobly technobly added this to the 0.6.2 milestone Apr 4, 2017

@m-mcgowan m-mcgowan changed the base branch from develop to release/v0.6.2-rc.1 Apr 5, 2017

@m-mcgowan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2017

Once the arduino header is included, the symbols remain visible until the end of the compilation unit (and for all subsequent headers.)

For example, in this case

#include "libraryExpecting062ArduinoSymbols.h"  // library 1
#include "libraryExpecting060Symbols.h"  // library 2

The second library is not expecting the arduino symbols to be defined (e.g. it might declare it's own copy of SPISettings) but these symbols are defined if library 1 includes Arduino.h in its header. To work around this, includes should be done in libraries an applications in a way thats all the headers not expecting arduino support first, and then all arduino-dependent headers afterwards.

In the example above, reversing the order of the headers would fix the problem.

@m-mcgowan m-mcgowan merged commit 724c359 into release/v0.6.2-rc.1 Apr 5, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@technobly
Copy link
Member

left a comment

Looks good!

@m-mcgowan m-mcgowan deleted the feature/arduino_compatibility_regression branch Apr 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.