Skip to content

Rename mbed_conf.h to mbed_config.h #1968

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 21, 2016
Merged

Rename mbed_conf.h to mbed_config.h #1968

merged 5 commits into from
Jun 21, 2016

Conversation

screamerbg
Copy link
Contributor

@screamerbg screamerbg commented Jun 17, 2016

Following the pattern of "device_has" to "DEVICE_", "features" to "FEATURE_", the mbed config system should map to mbed_config.h

@screamerbg
Copy link
Contributor Author

@mbed-bot: TEST

HOST_OSES=windows
BUILD_TOOLCHAINS=GCC_ARM,ARM,IAR
TARGETS=K64F,KL25Z

@mbed-bot
Copy link

Merged build finished. No test results found.
[Build 506]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

@mbed-bot
Copy link

[Build 506]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

@screamerbg
Copy link
Contributor Author

@mbed-bot: TEST

HOST_OSES=windows
BUILD_TOOLCHAINS=GCC_ARM,ARM,IAR
TARGETS=K64F,KL25Z

@mbed-bot
Copy link

Merged build finished. No test results found.
[Build 507]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

@mbed-bot
Copy link

[Build 507]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 17, 2016

Please read #1957 (comment), where it was proposed. I would go with the full name as here.

cc @bogdanm

@bogdanm
Copy link
Contributor

bogdanm commented Jun 17, 2016

Unfortunately we have used mbed_config.h in a different context in the past, see here:

https://github.com/mbedmicro/mbed/blob/master/tools/toolchains/__init__.py#L501

I don't think this previous mechanism was ever really adopted by our users, but keeping it in place is the best option for backward compatibility, I think.

@sg-
Copy link
Contributor

sg- commented Jun 17, 2016

1a1dafe

Not sure this was ever documented as a feature, at least I cant find anything other than the commit message (and google confirms). I think this would be a safe change to revert given we now have a more complete and documented config system unless you know specifically of user projects using this mechanism.

@bogdanm
Copy link
Contributor

bogdanm commented Jun 17, 2016

I don't know of any users of this, no. It was designed to be mbed's "equivalent" to autoconf's config.h and HAS_CONFIG_H, but we never really documented it, and if there are some users of this, they aren't many. I don't have any strong objections against removing it. Also, we might want to keep the idea of defining MBED_HAS_CONFIG_H automatically if a mbed_config,h was generated by the config system.

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 17, 2016

I don't know of any users of this, no.

Hidden feature :-) Let's use mbed_config.h.

Also, we might want to keep the idea of defining MBED_HAS_CONFIG_H automatically if a mbed_config,h was generated by the config system.

Does Config define a macro like MBED_CONF if it's active and there's at least one config value within the app? That would be sufficient? It does not matter if the config comes via -D or preinclude? I would imagine having MBED_HAS_CONFIG if it would be a regular header so users or us have to include it in the app somewhere:

#ifdef MBED_HAS_CONFIG
#include "mbed_config.h"
#endif

Any other use cases?

@bogdanm
Copy link
Contributor

bogdanm commented Jun 17, 2016

Hidden feature :-) Let's use mbed_config.h.

Sure, but then let's also remove the previous logic related to mbed_config.h

Any other use cases?

Not sure. Let's leave it out for now and add it later if needed.

@@ -344,7 +344,7 @@ def get_symbols(self):

# Config support
if self.has_config:
self.symbols.append('HAVE_MBED_CONFIG_H')
self.symbols.append('HAVE_MBED_CONFIG')
Copy link
Contributor

@bogdanm bogdanm Jun 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will work, but looks confusing. You're still adding the HAVE_MBED_CONFIG symbol,, even though the line that sets self.has_config to True was removed (below), so this code will never be executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was left so the config system can raise the flag. The user app can use the macro to "know" that configuration is in place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But currently it doesn't, so I guess this is meant to be implemented later. In which case I'd leave a comment explaining this in the source, because otherwise we're likely to forget this later.

@screamerbg
Copy link
Contributor Author

Now has_config is completely removed

@screamerbg
Copy link
Contributor Author

@mbed-bot: TEST

HOST_OSES=windows
BUILD_TOOLCHAINS=GCC_ARM,ARM,IAR
TARGETS=K64F,KL25Z

@mbed-bot
Copy link

[Build 517]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

Following the pattern device_has to DEVICE_, features to FEATURE_,
the mbed config system should map to mbed_config.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants