feature_ble: rename error_t to nrf_error_t

[andrewleech]

Description

I just updated to mbed-os-5.7.3 and could no longer build for NRF51 (windows, offline, gcc 6 2017-q1-update).

./mbed-os/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF5/source/common/ble_error.h:143:2: error: conflicting declaration 'typedef enum error_t error_t'
 }error_t;
  ^~~~~~~
In file included from f:\program files (x86)\gnu tools arm embedded\6 2017-q1-update\arm-none-eabi\include\c++\6.3.1\cerrno:42:0,
    ...
                 from f:\program files (x86)\gnu tools arm embedded\6 2017-q1-update\arm-none-eabi\include\c++\6.3.1\memory:79,
                 from .\mbed-os\features\FEATURE_BLE\targets\TARGET_NORDIC\TARGET_NRF5\source\nRF5XPalGattClient.cpp:17:
f:\program files (x86)\gnu tools arm embedded\6 2017-q1-update\arm-none-eabi\include\errno.h:5:13: note: previous declaration as 'typedef int error_t'
 typedef int error_t;

The definition of the new error_t type in TARGET_NRF5/source/common/ble_error.h states that the enum has been mapped from nrf_error.h (softdevice code) so in this PR I've simply renamed error_t to nrf_error_t to avoid conflicting with the standard library type

Status

READY

Migrations

There should be no changes to end user code.

NO

Related PRs

None

Deploy notes

Notes regarding the deployment of this PR. These should note any required changes in the build environment, tools, compilers and so on.

Steps to test or reproduce

Any code using the unified nordic target and ble should trigger this issue when compiled with gcc.
I've only tested with arm's distribution of gcc 6 2017-q1-update on windows 10 x64.

[mbed-ci]

User not whitelisted, CI not run.

[0xc0170]

@andrewleech Can you please sign https://os.mbed.com/contributor_agreement/ ?

[pan-]

@andrewleech Which NRF51 board did you use ?

[andrewleech]

Hi @0xc0170, all signed (https://os.mbed.com/users/corona/)

[andrewleech]

@pan- I'm actually using a custom board currently but it's based around a raytac mdbt40 module (nrf51 256K / 16K, no 32k crystal)

[pan-]

Looking more closely at the error; it is caused by the inclusion of newlib's errno.h which alias error_t with an int. This is a libc implementation issue; error_t is not a type that should be exported by errno.h.

However with the same setup (win 10 x64; mbed os 5.7.3 and gcc 6 2017-q1-update) I cannot reproduce the issue :(

[pan-]

@0xc0170 The nanostack may encounter the same kind of failure as it defines its own error_t

[0xc0170]

@0xc0170 The nanostack may encounter the same kind of failure as it defines its own error_t

Thanks @pan- !

@kjbracey-arm Have you experienced this. Looking at the codebase, there are 3 functions error_t socket_xxxx in the core that uses it. Might be as that type is expected to be integer type?

This PR looks good to me (the changes are internal changes ).

[kjbracey]

No, not seen this problem. The error_t in Nanostack is in an internal header, and no internal files include <errno.h>, and we generally don't include any platform files from most C files, so we should hopefully continue to miss it.

[andrewleech]

@pan- Sounds like compiler settings might have something to do with it.... in my project I'm using -std=gnu++14 perhaps this is unearthing the issue.
I currently can't compile without this flag so makes it hard to test (without starting a new project, don't have time right this minute to do so)

[andrewleech]

Hi, is there anything particularly stopping this being merged?

Do you need to see a rock solid reproduction, or is it ok as is?

[andrewleech]

Ok, I can confirm that the cxx standard change unearths this issue.

On a minimal example, it all compiles fine with -std=gnu++98
Same code fails as above with -std=gnu++11

[0xc0170]

The changes look fine (no braking changes in the user facing API, only internal API changed) and works with the current toolchain profiles we support (c+11 is not there yet)

Just one minor comment below, to the one change that might not be related.

One question, is errno.h included in nrf files, and if yes, why it is pulling that header file (what is needed) ?

[0xc0170]

how is this change relevant to error_t ? separate commit or PR?

[andrewleech]

That was not supposed to be changed, thanks for catching that.
My fault for working on multiple issues at once and trying to split them up in git.

[cmonr]

@andrewleech, starting test whilst waiting for a reply to @0xc0170's comment.
/morph build

[andrewleech]

With regard to errno.h inclusion, I snipped the traceback in the compiler error in the initial post... however it's the inclusion of <memory> in nRF5XPalGattClient.cpp that chains through to errno.h.

So no direct inclusion, but via other standard libe.

[0xc0170]

It's not more clear. I can not find why memory header file there is needed nRF5XPalGattClient.cpp (interesting that in this code file there is ble_error_t, they might have noticed this error previously ?
)
Memory header file for pre C+1x defines only very little and could not locate any of the functionality used in that file. I wonder why also system header file is included on the top of the file (would be better to be moved down). If it is not needed, then it can be removed.
Removing memory header file shall fix the problem (if really not needed) you are seeing but might uncover it later if ble_error.h gets included somewhere else.

[andrewleech]

@0xc0170 yes you're right, removing the #import <memory> does also fix the issue.
Everything still compiles fine for me, no missing or unknown functions.

I don't have access to the other compilers though to check one of them doesn't need the import, but I can't see anything in the file that should need it either.

Regardless of how safe this PR should be, there's no denying simply removing one import is a much smaller change.
I'd be happy to abandon this pr in favor of just that change if preferred?

[0xc0170]

Regardless of how safe this PR should be, there's no denying simply removing one import is a much smaller change.
I'd be happy to abandon this pr in favor of just that change if preferred?

Yes please, lets remove that header if not used.

@ARMmbed/team-nordic please review

[pan-]

@0xc0170 yes you're right, removing the #import does also fix the issue.

@andrewleech That's a mistake, #include <memory> is not needed however #include <new> is required.

[andrewleech]

@0xc0170, @pan- I've created a new PR with just the #include change, it appears to work well.
#6115

This PR can probably be closed in preference for that one.