search

ARMmbed / ble-nrf51822

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

Renamed BLE type defines to avoid clash with BLE_API enums. #17

Closed jcady closed 9 years ago

jcady commented 9 years ago

This fixes these issues:

mbedmicro/nRF51822#16 mbedmicro/BLE_API#46

rgrover commented 9 years ago

Hi,

Thanks for making this effort. I see that you've renamed ble.h to nrf_ble.h, and you've renamed symbols using an NRF infix.

I have some reservations against this change. We don't want to deviate too far from the Nordic sources; particularly the Nordic nRF SDK which is inherited nearly verbatim from Nordic. If we start deviating from the state of files in the Nordic SDK, updating to newer releases of the SDK would become painful. Had Nordic been supporting this repository directly, it would not have mattered; but they aren't. I've made an alternate commit to the develop branch; please take a look. https://github.com/mbedmicro/nRF51822/commit/3d87035e266c827ed082c417dd549b143c0b70ec

BLE_API's new BLE.h include didn't conflict for me with ble.h from Nordic SDK. I develop on a filesystem which is case sensitive. But I can see that this could be a problem for other filesystems. Our intention is to prefix all BLE_API includes with ble/. So we should be including ble/BLE.h.

Do you agree with this?

jcady commented 9 years ago

Looks good to me, you certainly know this code better than I do. I tested the develop branch and it compiles without those issues.

I found one more issue, see here: mbedmicro/nRF51822#16