search

ARMmbed / ble-nrf51822

Nordic stack and drivers for the mbed BLE_API
Other
46 stars 51 forks source link

use of extraIncludes #60

Closed autopulated closed 8 years ago

autopulated commented 8 years ago

This module makes extensive use of extraIncludes in module.json (a facility used to add directories to the include path for users of this module). This is bad for two reasons:

  1. these directories contain header files with many names which may clash with any other modules that also rely on including headers without the full path
  2. it substantially increases the length of compilation command lines for any users of this module: there is a finite limit to how long these can be, and at some point could cause problems (especially on windows).

@LiyouZhou just reported an interesting example of the sort of hard-to-debug fault that 1. above causes: Compiling https://github.com/ARMmbed/ble-examples/tree/master/BLE_Thermometer would fail because #include "BLE.h" was failing – but only on Linux (causing the problem to go undetected by developers using windows/mac).

The #include "BLE.h" statement was in a header file provided by the ble module, where the intention was to include ble/BLE.h. In fact, on windows and mac, ble.h from this ble-nrf51822 module was being included, which has the full path of source/nordic-sdk/components/softdevice/s130/include/ble.h.

Since a previously included header had already included the correct ble/BLE.h header, this happened to be harmless, and compilation succeeded (but it's possible this could have caused other weird bugs), breaking only on Linux, where a case-sensitive file system meant ble.h was not picked up in place of BLE.h.

rgrover commented 8 years ago

yes, I see the problem with using extraIncludes; but these sources are derived mainly from the Nordic SDK, and if we are to remove extraIncludes, it will mean that we will need to change include paths in many SDK files. That will mean deviating from the standard Nordic SDK; and would be a maintenance headache.

External code will use extraIncludes as a quick way to port to mbed OS. It is unfortunate that there is a ble.h in ble-nrf51822.

rainierwolfcastle commented 8 years ago

ARM Internal Ref: IOTSFW-1172

pan- commented 8 years ago

It is fixed since this module does not embed the Nordic SDK anymore. I close this.