mcci-catena / arduino-lmic

LoraWAN-MAC-in-C library, adapted to run under the Arduino environment
https://forum.mcci.io/c/device-software/arduino-lmic/
MIT License
636 stars 207 forks source link

Relative library includes #349

Open jpmeijers opened 5 years ago

jpmeijers commented 5 years ago

In hal/getpinmap_catena4420.cpp I see an include at the top: #include <arduino_lmic_hal_boards.h> Shouldn't this read as #include "../arduino_lmic_hal_boards.h"

If one wants to use this library as a local include, not a global include from the Arduino sketches directory, this #include gives an error during compile.

terrillmoore commented 5 years ago

Thanks for your comment. At present, the design principle is: if you want to use this library as a local include, you must add -I or other c compiler magic. Policy is not to use relative paths unless absolutely necessary. The header file is part of the API, and so <> notation is supposed to be used.

jpmeijers commented 5 years ago

you must add -I or other c compiler magic

That's not something one can easily do using the Arduino IDE. Seeing as this is an Arduino library I believe the library should work in all normal ways in which Arduino projects are laid out. Local includes via the src directory is one of them.

Another comment is that the core LMIC works fine. It's just the "new" hal headers that are causing compile time errors.

jpmeijers commented 5 years ago

Maybe an example of how I'm including LMIC might clear things up. Perhaps I'm doing that incorrectly.

I have an Arduino project with the following directory structure:

Project/
|_ Project.ino
|_ src/
  |_ radio.h
  |_ radio.cpp
  |_ arduino-lmic-2.3.2/
    |_ src/
    |_ ...

In Project.ino I include radio.h.

In radio.cpp I include LMIC and some other libraries as follows:

#include "radio.h"
#include "arduino-lmic-2.3.2/src/lmic.h"
#include "arduino-lmic-2.3.2/src/hal/hal.h"
#include <SPI.h>
#include <avr/eeprom.h>

Is there perhaps a better or more standard way of doing this? Where "this" is keeping all my dependencies within one folder and one source control with "snapshots" of the libraries and the versions I depend on. Using a src directory inside the project directory is a standard Arduino method and Arduino will recursively include everything inside the src directory at compile time.

terrillmoore commented 5 years ago

@jpmeijers Oh, I see. I don't do it this way -- I just have the git libraries live in my arduino/libraries folder and I tag them for releases; or I build in a VM. But I can see why you would. If we're going to support this workflow, we're going to need a simple test case for CI.

However, I don't think it's a great idea to include src/lmic.h and hal/hal.h directly; clients should only use the header files in src. There should be enough header files there to allow everything else to be found, and I'm pretty sure those use relative paths. (If not, we should fix them when we address this bug as well.)

terrillmoore commented 5 years ago

@jpmeijers, Thanks again, I will take a look at this in the next release cycle. I think that's a reasonable design flow, and it seems like an innocuous change. The only problem is that it might break certain "side-by-side" build scenarios -- strange build systems that require everything to be flattened. I've had to deal with them. In that case, if there are absolute paths in header files, things break. However, since our primary target is Arduino IDE, I think it's reasonable to put in the paths, perhaps with a #define to further obscure things -- #include ARDUINO_LMIC_HAL_BOARDS_H so it can be overridden.