[Exporters] Update exporters to include and use mbed_conf.h (Was #1964) #1975

theotherjimmy · 2016-06-20T15:00:52Z

Affected exporters:

gcc_arm
uVision4
IAR
emblocks
atmelstudio
codered
simplicityv3

Summary of discussion:

@0xc0170:
We should wait until the name of mbed_conf.h is set in stone.

@bogdanm:
We should introduce a configuration variable for all exporters to indicate if they export the mbed_conf.h. This will help eliminate duplication.

bogdanm · 2016-06-20T15:18:36Z

We should introduce a configuration variable for all exporters to indicate if they export the mbed_conf.h.

That's more an attribute of the exporter itself than a configuration variable. If it supports mbed_conf.h or whatever its name is going to end up being, that's the mechanism it should use; if not (because it was not upgraded it or simply because it can't support prefix headers for whatever reason), it should still use the old method of defining macros.

theotherjimmy · 2016-06-20T15:20:32Z

Thanks for the clarification @bogdanm. That was what I was trying to say, but used the wrong word (configuration varible) as opposed to exporter attribute.

theotherjimmy · 2016-06-20T19:35:43Z

@bogdanm, does the 7th commit (Create and enable...) resolve the switching between -D style and mbed_confi.h (or whatever we decide to call it) style that you suggested?

theotherjimmy · 2016-06-20T19:40:47Z

To answer my own question, No the mechanism does not work yet.

theotherjimmy · 2016-06-20T22:19:33Z

Well, actually it might. I don't have any content in my mbed_conf.h files, and therefore cannot test it.

bogdanm · 2016-06-21T10:20:31Z

tools/export/exporters.py

@@ -47,6 +48,9 @@ def flags(self):
    def progen_flags(self):
        if not hasattr(self, "_progen_flag_cache") :
            self._progen_flag_cache = dict([(key + "_flags", value) for key,value in self.flags.iteritems()])
+            if self.config_header:
+                self._progen_flag_cache['c_flags'] += self.toolchain.get_config_option(self.config_header)
+                self._progen_flag_cache['cxx_flags'] += self.toolchain.get_config_option(self.config_header)


Just realized that you might need to add this to ASM files as well, since some toolchains (like GCC) can preprocess the ASM file before invoking the assembler, and we use this feature in a few parts of the system. Not sure though if this is something that we want to enable for all toolchains or only for a few of them.

bogdanm · 2016-06-21T10:21:05Z

The mechanism looks like it'll probably work, but I didn't test it locally yet.

theotherjimmy · 2016-06-21T18:08:18Z

Let me know when you do.

bogdanm · 2016-06-23T13:13:09Z

Let me know when you do.

Doing my best, but there seem to be some unrelated issues with the exporters right now. We don't really have a test suite for exporters, so all I can do is try a few manually and see if they work.

Also, #1968 happened meanwhile, so the name of the configuration file is now mbed_config.h. Which means that this shouold be declared somewhere (maybe in mbedToolchain) rather than hard-coded in more than one place.

theotherjimmy · 2016-06-23T17:42:09Z

Rebased. @bogdanm is the move to one location commit what you want?

screamerbg · 2016-06-24T01:17:11Z

tools/export/gccarm.py

@@ -121,6 +121,8 @@ class GccArm(Exporter):

    DOT_IN_RELATIVE_PATH = True

+    MBED_CONF_ACTIVE = True


@theotherjimmy Can you rename this to HAS_MBED_CONFIG please?

Is this flag to signal if mbed config is active (=present) or if there is a configuration somewhere (has config)? I would say the former, same as PROGEN_ACTIVE flag. @theotherjimmy what do you think?

This should really be MBED_CONF_HEADER_SUPPORTED. If present, this field says "this exporter can use the header files generated by the configuration system". If not present, the exporter can still work with the configuration system, but using the old method (direct generation of macros for the configuration system).

Fixing now.

screamerbg · 2016-06-24T01:19:19Z

LGTM
+1

screamerbg · 2016-06-24T01:21:09Z

Minor comment. Noticed that MBED_CONF_ACTIVE is always true. Isn't it fair/safe to assume that the config is always there (even if empty)?

bogdanm · 2016-06-24T09:27:43Z

Noticed that MBED_CONF_ACTIVE is always true.

The point of MBED_CONF_ACTIVE is to differentiate between exporters that support configuration using the new mechanism (with automatically included configuration header) and exporters that support only the old mechanism (adding the config macros to the exporter's macro list directly). MBED_CONF_ACTIVE (which should probably be callsed MBED_CONFIG_ACTIVE now) is only True for those exporters that support the new mechanism. In time, as more exporters are updated, the intention is to get rid of MBED_CONF_ACITVE completely.

bogdanm · 2016-06-24T11:43:59Z

@theotherjimmy, did you test this PR with uvision? It doesn't seem to work for me. A .uvproj file is created and I can see this line in it:

<MiscControls> --restrict --preinclude --multibyte_chars --apcs=interwork -c -O3 -Otime --brief_diagnostics --cpu=Cortex-M4.fp -D__ASSERT_MSG --no_depend_system_headers mbed_config.h --md --split_sections --no_rtti --gnu</MiscControls>

As you can see, mbed_config.h appears there without --preinclude (this is the only place in the .uvproj file where mbed_config.h appears). Consequently, when compiling, I get this error: Error: C4065E: type of input file 'mbed_config.h' unknown.

Tested with uVision 5.20, project-generator 0.9.4.

theotherjimmy · 2016-06-24T17:32:04Z

@bogdanm this looks better I think:
<MiscControls> --restrict --multibyte_chars --apcs=interwork -c -O3 --cpu=Cortex-M3 -Otime --brief_diagnostics -D__ASSERT_MSG --no_depend_system_headers --preinclude=mbed_config.h --md --split_sections --no_rtti --gnu</MiscControls>

bogdanm · 2016-06-27T09:53:05Z

@theotherjimmy, that looks better indeed. Now I'm getting a different set of errors:

"no source": Error:  #5: cannot open source input file "mbed-os\core\hal\common\AnalogIn.cpp": No such file or directory
"no source": Error:  #5: cannot open source input file "mbed-os\core\hal\common\assert.c": No such file or directory
"no source": Error:  #5: cannot open source input file "main.cpp": No such file or directory

and so on. It looks like that paths are messed up. I doubt this is because of this PR, but in any case, it means that I can't really test it. Cc @0xc0170.

0xc0170 · 2016-06-27T13:21:48Z

I am looking at it, seems to me that master is broken, thus is not fault of this PR. The paths are wrong. I should have a fix for it today.

bogdanm · 2016-06-27T14:19:07Z

With #2026 in place, this works with uVision on blinky. IAR still fails, but for a different reason (duplicate object names). So I'd say that after we merge #2026 we're good to merge this.
Take also a look at #2020 which fixes a issue with --preinclude and IAR (I added a comment about that this to this PR a while ago).

bogdanm · 2016-06-27T15:55:41Z

tools/export/exporters.py

-        self.config_macros = config.get_config_data_macros()
-
+
+        if hasattr(self, "MBED_CONFIG_HEADER_SUPPORTED") and self.MBED_CONF_ACTIVE :


and self.MBED_CONFIG_HEADER_SUPPORTED. Please do not issue/modify a PR without testing its functionality at least once locally.

theotherjimmy · 2016-06-27T18:56:12Z

thanks @screamerbg!

theotherjimmy · 2016-06-27T18:57:18Z

@bogdanm, @0xc0170, @screamerbg is this ready to merge?

theotherjimmy · 2016-06-27T18:58:40Z

Just a moment, the changes in this branch is out of date. Rebasing.

bogdanm · 2016-06-27T19:01:01Z

I suspect we'll have some problems related to assembler flags, but let's merge it like this for now and see how it goes. However, it looks like it needs to be rebased.

theotherjimmy · 2016-06-27T19:01:21Z

Yeah, I'm working on it right now.

theotherjimmy · 2016-06-27T19:02:52Z

Don't merge just yet, I need to verify that IAR asm gets the right flags.

get_conifg_option -> get_config_option

The mbed_conf_active feature disables configuration macros on the command line and replaces them with a preinclude header file

relpaces the --preinclude mbed_config.h with --preinclude=mbed_config.h

…rt/$file -e "s/MBED_CONF_ACTIVE/MBED_CONFIG_HEADER_SUPPORTED/"; done

theotherjimmy · 2016-06-27T19:17:08Z

Looks fine. Side note: IAR IDE works on my Linux machine!

bogdanm · 2016-06-27T19:33:58Z

tools/toolchains/iar.py

    def get_compile_options(self, defines, includes, for_asm=False):
-        opts = ['-D%s' % d for d in defines] + ['-f', self.get_inc_file(includes)]
+        opts = ['-f', self.get_inc_file(includes)]


defines isn't used anymore ?

Uh, whoops. I did not intend to delete that. OTOH, they still seem to propagate to the exporters. I suspect this would break the direct build on IAR, but not an exported project.

bogdanm · 2016-06-27T19:59:18Z

tools/toolchains/iar.py

    def get_compile_options(self, defines, includes, for_asm=False):
        opts = ['-D%s' % d for d in defines] + ['-f', self.get_inc_file(includes)]
+        config_header = self.get_config_header()


Duplicated (see below in the else: block). Not a real problem though.

bogdanm · 2016-06-27T19:59:39Z

+1, ready to merge.

This commit address the issue presented in #2073. Previously, mbed_config.h was automatically included by the toolchain. With this change, all the sources that need "mbed_config.h" need to include it manually. However, "mbed.h" now includes "mbed_config.h", so configuration data is automatically available to sources that include "mbed.h". With this change, even if there's no configuration data in the project being built, an empty "mbed_config.h" is generated by the build system. This change looks big, but most of it is an almost complete reversal of #1975. This is likely a breaking change, and needs to be properly communicated.

theotherjimmy mentioned this pull request Jun 20, 2016

Update exporters to include and use mbed_conf.h #1964

Closed

theotherjimmy force-pushed the export-mbed-conf branch 2 times, most recently from 6ac1703 to b202db5 Compare June 20, 2016 19:33

theotherjimmy force-pushed the export-mbed-conf branch from b202db5 to c2ed550 Compare June 20, 2016 19:37

0xc0170 added the needs: review label Jun 21, 2016

bogdanm reviewed Jun 21, 2016
View reviewed changes

theotherjimmy force-pushed the export-mbed-conf branch from c2ed550 to aed51dc Compare June 23, 2016 17:41

screamerbg reviewed Jun 24, 2016
View reviewed changes

bogdanm added needs: work and removed needs: review labels Jun 24, 2016

bogdanm mentioned this pull request Jun 27, 2016

Fixed config-related options for the IAR assembler #2020

Merged

theotherjimmy force-pushed the export-mbed-conf branch from 7b7c9ee to daf6c67 Compare June 27, 2016 15:43

bogdanm reviewed Jun 27, 2016
View reviewed changes

screamerbg changed the title ~~Update exporters to include and use mbed_conf.h (Was #1964)~~ [Exporters] Update exporters to include and use mbed_conf.h (Was #1964) Jun 27, 2016

Update exporters to include the generated mbed_conf.h

4f29483

theotherjimmy force-pushed the export-mbed-conf branch from 4995e70 to ad0e429 Compare June 27, 2016 19:02

theotherjimmy force-pushed the export-mbed-conf branch from ad0e429 to 7190244 Compare June 27, 2016 19:05

theotherjimmy and others added 9 commits June 27, 2016 14:06

Export command line for including mbed_conf.h from toolchains

5532fb8

Update progen-style flags with the toolchains include mbed_conf.h

d9b4109

Fixed gcc.py typo

879ae8c

get_conifg_option -> get_config_option

Fix typo after rebase

a1b73c9

Change uVisoun[45] to get preincludes

c06dad5

Create and enable exporter flag mbed_conf_active for exporters

3a273f7

The mbed_conf_active feature disables configuration macros on the command line and replaces them with a preinclude header file

Move name of config file to one location

f92d3ec

Switch to = style for preincludes

4746beb

relpaces the --preinclude mbed_config.h with --preinclude=mbed_config.h

for file in ls tofor file in ls tools/export`; do sed -i tools/expo…

7b83c30

…rt/$file -e "s/MBED_CONF_ACTIVE/MBED_CONFIG_HEADER_SUPPORTED/"; done

theotherjimmy force-pushed the export-mbed-conf branch from 7190244 to 7b83c30 Compare June 27, 2016 19:06

bogdanm reviewed Jun 27, 2016
View reviewed changes

Undelete defines from IAR

8e11fa2

bogdanm reviewed Jun 27, 2016
View reviewed changes

0xc0170 removed the needs: work label Jun 28, 2016

bogdanm merged commit af71d87 into ARMmbed:master Jun 28, 2016

bogdanm deleted the export-mbed-conf branch June 28, 2016 08:15

bogdanm mentioned this pull request Jul 13, 2016

Change how the config system is used #2162

Closed

		@@ -121,6 +121,8 @@ class GccArm(Exporter):

		DOT_IN_RELATIVE_PATH = True

		MBED_CONF_ACTIVE = True

		self.config_macros = config.get_config_data_macros()


		if hasattr(self, "MBED_CONFIG_HEADER_SUPPORTED") and self.MBED_CONF_ACTIVE :

[Exporters] Update exporters to include and use mbed_conf.h (Was #1964) #1975

[Exporters] Update exporters to include and use mbed_conf.h (Was #1964) #1975

Uh oh!

Conversation

theotherjimmy commented Jun 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bogdanm commented Jun 20, 2016

Uh oh!

theotherjimmy commented Jun 20, 2016

Uh oh!

theotherjimmy commented Jun 20, 2016

Uh oh!

theotherjimmy commented Jun 20, 2016

Uh oh!

theotherjimmy commented Jun 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdanm commented Jun 21, 2016

Uh oh!

theotherjimmy commented Jun 21, 2016

Uh oh!

bogdanm commented Jun 23, 2016

Uh oh!

theotherjimmy commented Jun 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

screamerbg commented Jun 24, 2016

Uh oh!

screamerbg commented Jun 24, 2016

Uh oh!

bogdanm commented Jun 24, 2016

Uh oh!

bogdanm commented Jun 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Jun 24, 2016

Uh oh!

bogdanm commented Jun 27, 2016

Uh oh!

0xc0170 commented Jun 27, 2016

Uh oh!

bogdanm commented Jun 27, 2016

Uh oh!

bogdanm Jun 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Jun 27, 2016

Uh oh!

theotherjimmy commented Jun 27, 2016

Uh oh!

theotherjimmy commented Jun 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bogdanm commented Jun 27, 2016

Uh oh!

theotherjimmy commented Jun 27, 2016

Uh oh!

theotherjimmy commented Jun 27, 2016

Uh oh!

theotherjimmy commented Jun 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdanm commented Jun 27, 2016

Uh oh!

Uh oh!

theotherjimmy commented Jun 20, 2016 •

edited

Loading

bogdanm commented Jun 24, 2016 •

edited

Loading

bogdanm Jun 27, 2016 •

edited

Loading

theotherjimmy commented Jun 27, 2016 •

edited

Loading