Fix incorrect #ifdefs on DEVICE_FOO macros

[deece]

Description

The DEVICE_FOO macros are predefined to be 0 or 1. There are a number of instances which incorrectly check if they are defined, rather than checking the value.

This PR addresses most (if not all) of these incorrect checks.

Fixes #8913

Testing on master is blocked by #8912. A partial test on mbed-os-5.9.7 with Nucleo-F030R8 was successful.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

[kjbracey]

Are you sure those are ever defined as 0? See comment on issue.

I agree that the majority of the code uses #if, suggesting there's at least an attempt to support them being defined as 0. But I suspect that's historical rather than something that still happens with the mbed cli JSON config stuff.

I would have thought the "approved" way to do what you're attempting these days would be to make a custom target derived from your base target, and put a device_has_remove in its JSON. (And I'm guessing that would undefine the macro?) Can you try that?

We certainly should be consistent on the tests in the code. I just wonder if the consistency change should be to #ifdef, as that's certainly what most of the other FEATURE_, TOOLCHAIN_ etc things governed by has and remove in the JSON do. (Some MBED_CONF_ do #if, because they can take true or false JSON settings - undefined would be null).

[cmonr]

@deece Testing should now be unblocked since #8932 came in.

[deece]

Thanks, testing currently blocked by #8983

[cmonr]

@deece Is this still blocked by #8983?

[deece]

No, I've tested this successfully, thanks for the reminder.

[cmonr]

Out of curiosity, how did you manage to test this?
I'm wondering if there are test(s) that should check for this kind of thing in the future.

[deece]

I'm using the -D option to build.py to (re)define the DEVICE_foo macros to 0.

I'm no longer doing this btw, but instead have created custom platforms that never define them in the first place.

[deepikabhavnani]

All these config options are not macros, and not used as ‘-D’. Instead they are added to json files in device_has section, so if device does not have analog_in respective define will not be present, it won’t be zero. With ‘if (Device_foo)’ build will error if ‘foo’ is not present in specific target.

[deece]

@deepikabhavnani: The json results in these macros passed in via CFLAGS. Undefined macros evaluate to 0, and most checks in the code are #ifs, this PR just makes this usage consistent.

[deepikabhavnani]

@kegilbert - Please review

[kegilbert]

Thinking on this a bit, I don't have any major objections at the moment. Pinging @c1728p9 for a follow up as this touches a lot of target level configuration options. This was something we sort of wanted to move towards in general for the OS as it encourages using the config system rather than defining a macro or using CFLAGS which we currently have a bit of a split on.

That being said the targets work in a very related but subtlety different manner so would like to hear from HAL.

Side note, ran this briefly by @theotherjimmy and @bridadan and they didn't have a strong preference it seemed like.

[0xc0170]

@ARMmbed/mbed-os-tools Can you review? There's similar issue reported #9133. Can we clean this up and have it also documented ?

There are some good points in this discussion worth documenting if not yet there

[bridadan]

This doesn't really affect the tools. This seems more like an Mbed OS convention/choice. But I would say definitely be consistent!

I'll just clarify what has already been mentioned. The behavior of device_has is documented here. If an api is present in the device_has config list, a macro with the name DEVICE_<API> will be generated and set to 1. If a certain api is not present in the list, it will not be included as a macro.

My 2 cents: regardless of how the tools produce/don't produce macros, seems like the source code should be able to switch its behavior on the value of the macro, not just its presence.

[0xc0170]

@kjbracey-arm @bulislaw Please review

[kjbracey]

Approve the concept.

This also tells us no toolchain is set to warn on #if FOO if FOO is undefined, and I'm okay with that. And that means that #if defined(FOO) && FOO is an antipattern if in the main mbed code base, but I do see that a lot - presumably to try to avoid such a warning.

As this has been open a while, I suggest rebasing, and making sure to re-run the search-and-replace to check for any new ones arrived during discussion.

[0xc0170]

That is correct to avoid a warning from Wundef in gcc case, havent seen it in other toolchains but might be there as well. I also preferred to use that to avoid a warning.

This also tells us no toolchain is set to warn on #if FOO if FOO is undefined

@deece or @ARMmbed/mbed-os-core can we verify this is the case?

[bulislaw]

Makes sense, if it doesn't spam us with warning on any of supported toolchains.

[cmonr]

@deece It sounds like everyone has warmed up to the idea.

As this has been open a while, I suggest rebasing, and making sure to re-run the search-and-replace to check for any new ones arrived during discussion.

Would you mind doing this before we start CI?

[deece]

@cmonr Done, picked up a few more things (maybe I missed them initially as I was manually reviewing).

git grep '#if.*def.*DEVICE'

(note: there are many false positives with this grep)

It's worth getting more eyes on targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F439xI/TARGET_MODULE_UBLOX_ODIN_W2/sdk/ublox-odin-w2-drivers/OdinWiFiInterface.*, I cleaned up some bits that looked sloppy, it probably warrants reviewing the whole module in detail.

[cmonr]

@deece As a word of caution, any changes to targets need to be OK'd by partners.

We can proceeed in two ways:

1. Keep the PR asis, requiring reviews by six-ish different partners.
2. Partially revert the last commit, removing the modifications to partner code, and we can open an issue for partners to follow up with when they have time.

Option 1 would hold up this PR until an OK was reached.
Option 2 would allow this PR to progress as soon as the commit was corrected.

Let us know what you'd like to do. I personally prefer Option 2.

[deece]

@cmonr I can split the partner-specific changes into separate PRs, just let me know which files should be split out (presumably the targets/*)?

[cmonr]

Yup, the targets/* portion. It can even be in the same PR. We just need to get the OK from them, and I imagine you want to use this PR sooner rather than later :)

[deece]

Ok, I've split the partner code into #9163

[cmonr]

CI started

[mbed-ci]

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts