Fixed sw4stm32 exporter #4779

JarnoEtt · 2017-07-19T09:38:21Z

Notes:
Fixes issues with sw4stm32 exporter

Description

Added mbed_config.h as -included, fixes #4084.

Added exclude directory generation, fixes #4091 (see theotherjimmy post).

Added -O2 to GCC compiler because e.g. mbed-os/rtos/rtx5/TARGET_CORTEX_M/rtx_delay.c needs it.

Exporter generated broken project file if there was quotes in toolchain symbols, fixed that also.

Status

READY

Todos

Tests
Documentation

Deploy notes

Steps to test or reproduce

0xc0170 · 2017-07-19T10:01:29Z

Can you please sign https://developer.mbed.org/contributor_agreement/ ?

cc @bcostm @adustm @LMESTM @jeromecoutant

JarnoEtt · 2017-07-19T10:02:37Z

Signed already, is there somekind of a problem?

0xc0170 · 2017-07-19T10:09:32Z

I could not locate JarnoEtt on mbed developer page, whats your nick name?

JarnoEtt · 2017-07-19T10:11:29Z

Oh, my mbed developer nickname is Jarno.

Nodraak · 2017-07-19T12:10:05Z

tools/export/sw4stm32/__init__.py

-from os.path import splitext, basename, join
+import os
+
+from os.path import splitext, basename, join, dirname


Is there a specific reason to import os; from os.path import XX instead of doing directly from os.path import XX as it was before ?

Nodraak · 2017-07-19T12:16:30Z

tools/export/sw4stm32/__init__.py

+        This function removes ./ from str.
+        str must be converted with win_to_unix() before using this function.
+        """
+	if str == None:


There seems to be a tabulation here. Python conventions (PEP8) prefer 4 spaces for indentation, and dislike mixed tabs and spaces. Moreover, str is a builtin class and should not be used as a variable name: prefer a name like path which is also more descriptive of what the variable holds. Also the prefered method to test for None is if path is None because None is a singleton (also PEP8).

Nodraak · 2017-07-19T12:17:08Z

tools/export/sw4stm32/__init__.py

+            src) for src in self.resources.c_sources + self.resources.cpp_sources + self.resources.s_sources)]
+        if '.' in self.source_folders:
+            self.source_folders.remove('.')
+        #print source_folders


Debug comment should be removed. (Not the only occurence of this, but I will report only this one)

Nodraak · 2017-07-19T12:22:08Z

tools/export/sw4stm32/__init__.py

        libraries = []
        for lib in self.resources.libraries:
            l, _ = splitext(basename(lib))
            libraries.append(l[3:])

+        self.include_path = [self.filter_dot(s) for s in self.resources.inc_dirs]
+        print 'Include folders: {0}'.format(len(self.include_path))


Python 3's print is not a keyword anymore and require parenthesis.
Mbed uses Python 2, but if some day Python 3 is supported (and it should be asap), best is to ease this work as much as possible.

JarnoEtt · 2017-07-19T12:31:44Z

@Nodraak changed.

Nodraak · 2017-07-19T12:31:45Z

As a Python lover, I made a few comments about the Python code. I think mbed's Python code could be greatly enhanced and therefore I think that each PR should try to improve the quality and not introduce new code style issues.

@JarnoEtt, first of all, thank you for your contribution. I don't know if your are an experienced Python dev, but if you don't already, I suggest you use tools such as Pylint (others exists, such as Flake8 or others but I like Pylint), in order to check the quality of your code. This will improve the readability of your code and even sometimes highlight potential issues that could lead to bugs.

JarnoEtt · 2017-07-19T12:34:41Z

Thx for your feedback, going to check those.

theotherjimmy · 2017-07-19T16:07:28Z

As a Python lover, I made a few comments about the Python code. I think mbed's Python code could be greatly enhanced and therefore I think that each PR should try to improve the quality and not introduce new code style issues.

My hero.

theotherjimmy

This is a great start. I'm going to remove the deprecation notice for SW4STM32. But, This looks like it does nothing with the passed in compiler flags. It could be supported better if it did.

theotherjimmy · 2017-07-19T16:08:54Z

tools/export/sw4stm32/__init__.py

+        This function removes ./ from str.
+        str must be converted with win_to_unix() before using this function.
+        """
+        if path is None:


This should just be:

if not path: return path

That way you avoid empty string issues too.

theotherjimmy · 2017-07-19T16:09:53Z

tools/export/sw4stm32/__init__.py

+        """
+        if path is None:
+            return None
+        if path[:2] == './':


This check should be:

if path.startswith('./'):

That's more in line with what you're trying to check.

theotherjimmy · 2017-07-19T16:11:45Z

tools/export/sw4stm32/__init__.py

+        if not found:
+            for directory in self.exclude_dirs:
+               # Do not exclude subfolders from excluded folder
+                if path.find(directory+'/') != -1:


You probably don't care where the substring is, so:

if (directory + '/') in path:

Alternatively,

if join(directory, '') in path:

theotherjimmy · 2017-07-19T16:13:21Z

tools/export/sw4stm32/__init__.py

+        found = False
+        for used in self.include_path:
+            if path == used:
+                found = True


This whole for loop is the same as:

found = path in self.include_path

theotherjimmy · 2017-07-19T16:15:55Z

tools/export/sw4stm32/__init__.py

+
+        ld_script = self.filter_dot(self.resources.linker_script)
+
+        # self.lib_dirs = [self.filter_dot(s) for s in self.resources.lib_dirs]


Could you remove commented out code?

Changes made. Can't promise anything about build method or other things, there was pretty tight timeline for this and sw4stm32 exporter was more broken and bare boned than we thought.

theotherjimmy · 2017-07-19T16:18:58Z

@JarnoEtt Would it be possible to write a build method for this class? If you do, it will be added to CI and will significantly more stable.

theotherjimmy

Python bits look great! Still needs to pass compiler flags into the template though.

theotherjimmy · 2017-07-26T19:57:28Z

@JarnoEtt You mentioned that your timeline for this fix was short. Would you be able to support SW4STM32 after that time? How long do you have to fix this? I might be able to help out with the fix, if your need it in really soon.

theotherjimmy · 2017-08-07T15:13:31Z

@JarnoEtt Any news?

@bcostm @adustm @LMESTM @jeromecoutant Could you review? Would you like to help make this exporter better?

JarnoEtt · 2017-08-07T15:28:40Z

Oh, sorry for the silence. I have been out of office/out of reach for a while. @ajaakko-arm and @Veli-PekkaPorrassalmi took this task and going to continue with flags + linker script problem #4269. They are also informed about headless build method request. I added them to forked repository, is there anything else to do so they can do changes to this case?

theotherjimmy · 2017-08-16T13:52:15Z

@ajaakko-arm It's great to see progress. Let me re-review and give you the checklist of things I would like to see before merge and how well this PR is doing.

theotherjimmy

We're getting there. I'm really excited to see that the SW4STM32 exporter is being maintained, and that it heeds the scan resources rules.

I have left a few suggestions that may help reduce code duplication among the exporters. Please either follow them, or explain why you must duplicate the code.

Further, for the best user experience, , an exporter:

Takes input from the resource scan.
Uses the flags in the build profiles. (I'm pretty sure that the template just ignores them)
Has a single template file for each file type they produce. For example, an eclipse CDT project would have one template for .project files and one for .cproject files.
Does not call Mbed CLI. It is possible to export from the website, which will not include Mbed CLI in the resulting zip.

theotherjimmy · 2017-08-16T13:57:11Z

tools/export/sw4stm32/__init__.py

+                # Paths returned by os.walk() must be split with os.dep
+                # to accomodate Windows weirdness.
+                parts = root.split(sep)
+                self.remove_unused('/'.join(parts))


FYI, You should be able to avoid walking here. self.resources has an ignored_dirs member on export that should be very useful to you. Moreover, you could just inherit from the GnuArmEclipse class and get most of this for free.

When I looked at the code first time, my thought was, that there should be some common or base package every exporter (or more likely every exporter for IDE's based on Eclipse) could use. There were so much same or similar functionality.

To inherit GnuArmEclipse is of course another option, and maybe simpler. I could give it a try.

FYI.
Flags were next on my todo-list.

Awesome! I wanted to record what should happen before we get this in. Thanks for taking this over!

theotherjimmy · 2017-08-16T13:58:17Z

tools/export/sw4stm32/__init__.py

+        print ('\nCreate a System Workbench for STM32 managed project')
+        print ('Project name: {0}'.format(self.project_name))
+        print ('Target:       {0}'.format(self.toolchain.target.name))
+        print ('Toolchain:    {0}'.format(self.TOOLCHAIN) + '\n')


I appreciate the attention to making this printing python 3 compatible, but I have to ask: Why do you need to tell the user exactly what they specified? They really should know all of this stuff.

To be honest, I borrowed this from gnuarmeclipse exporter. But I personally like the idea, that program is printing something, so that I know what's going on.

Better option would be to print some deeper information when exporter is called with verbose flag on. Like path to linker script, flags etc. But i suppose there's no way to know if 'mbed export' was called with -v flag?

But i suppose there's no way to know if 'mbed export' was called with -v flag?

That's correct.

Alright then. I will switch to using logging or something like it for the exporters soon. I'll change your prints to logs at a good level then. now you can leave it in.

noutram-old-uopaccount · 2017-08-17T16:41:33Z

Can I ask:

"Added -O2 to GCC compiler because e.g. mbed-os/rtos/rtx5/TARGET_CORTEX_M/rtx_delay.c needs it"

May I ask why this is the case? So if someone changes the optimisation settings, it breaks?

theotherjimmy · 2017-08-17T17:06:40Z

@noutram You might want to ask that at https://github.com/ARM-software/CMSIS_5 as that file comes from https://github.com/ARM-software/CMSIS_5/blob/develop/CMSIS/RTOS2/RTX/Source/rtx_delay.c

…ader() instead of hardcoding it to project file.

Added makefile.target, and rule for linker script pre-compilation.

SW4STM32 linker script name now contains target name. Print formatting is using new Python style.

Sw4STM32 is using methods from GNUARMEclipse class to handle flags. Flags are also passed to cproject file.

Removed unused methods and methods with dublicate functionality. Removed unused imports. Generating list of defines for assembler.

Fixed invalid linker script path in project file. Using 'defines' instead of 'symbols' in compiler options.

Quotation marks in defines had to be replaced with html codes.

Sometimes Sw4STM32 exporter receives wrong linker script file. Because build directories cannot be excluded from scanned resources, ld files are removed.

Quoting linker preprocessor command, if path contains parentheses. Using relative path to shorten list of included directories. Using Eclipse variables to get path to preprocessor.

Directories Debug and Release must not be excluded. Otherwise, Eclipse is not able to find binaries, which are stored to build directories.

Add double quotes to linker command unconditionally.

Some of the optimization and warning flags, which were supported by the gnuarmeclipse exporter, were missing from the sw4stm32.

ajaakkoh · 2017-09-13T13:02:01Z

Did rebase from https://github.com/ARMmbed/mbed-os master to test NUCLEO_F767ZI.

bcostm · 2017-09-15T07:54:56Z

I have rechecked the exporter on NUCLEO_F767ZI and it is OK with me.

theotherjimmy · 2017-09-15T14:21:41Z

Thanks @bcostm for testing!

@tommikas It looks like something is going wrong with the Oulu CI, but no C files were harmed in the making of this pull request. Further, this only touches an untested (in CI) exporter. Could you take a look?

tommikas · 2017-09-18T06:29:28Z

@theotherjimmy There were some IAR build machines that were configured slightly wrong which caused this to fail. All green now.

theotherjimmy · 2017-09-21T14:08:46Z

/morph export-build

mbed-bot · 2017-09-21T22:43:51Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph export-build

Output

mbed Build Number: 149

All exports and builds passed!

noutram-old-uopaccount · 2017-10-05T06:40:45Z

This is good news! I will test today.

0xc0170 added the needs: CLA label Jul 19, 2017

0xc0170 added the needs: review label Jul 19, 2017

0xc0170 removed the needs: CLA label Jul 19, 2017

Nodraak reviewed Jul 19, 2017

View reviewed changes

LMESTM mentioned this pull request Jul 19, 2017

Nucleo_blink_led fails to compile on SW4STM32 #4269

Closed

theotherjimmy suggested changes Jul 19, 2017

View reviewed changes

theotherjimmy mentioned this pull request Jul 19, 2017

Deprecate unsupported exporters #4742

Merged

theotherjimmy suggested changes Jul 21, 2017

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jul 24, 2017

theotherjimmy mentioned this pull request Jul 26, 2017

Export for sw4stm32 - project does not build ARMmbed/mbed-cli#525

Closed

theotherjimmy suggested changes Aug 16, 2017

View reviewed changes

JarnoEtt and others added 14 commits September 13, 2017 12:05

Review changes + including config header from toolchain.get_config_he…

fbc4a89

…ader() instead of hardcoding it to project file.

export: Fix sw4stm32 linker script problem

b2876c4

Added makefile.target, and rule for linker script pre-compilation.

export: Change linker script name

dbf20fc

SW4STM32 linker script name now contains target name. Print formatting is using new Python style.

export: Sw4STM32 inherits GNUARMEclipse

83127db

Sw4STM32 is using methods from GNUARMEclipse class to handle flags. Flags are also passed to cproject file.

export: Remove dublicate methods in Sw4STM32

d8d2b40

Removed unused methods and methods with dublicate functionality. Removed unused imports. Generating list of defines for assembler.

export: Invalid linker script path in Sw4STM32

e0cdda6

Fixed invalid linker script path in project file. Using 'defines' instead of 'symbols' in compiler options.

export: Fix after testing Sw4STM32

98e578f

Quotation marks in defines had to be replaced with html codes.

export: Remove preprocessed ld files after build

e1d163b

Sometimes Sw4STM32 exporter receives wrong linker script file. Because build directories cannot be excluded from scanned resources, ld files are removed.

export: Fix Windows problems with Sw4STM32

9d386b2

Quoting linker preprocessor command, if path contains parentheses. Using relative path to shorten list of included directories. Using Eclipse variables to get path to preprocessor.

export: Add debug configuration to Sw4STM32

8472764

export: Remove build directories from excluded paths

b94ad5a

Directories Debug and Release must not be excluded. Otherwise, Eclipse is not able to find binaries, which are stored to build directories.

export: Fix invalid id's in Sw4STM32 project files

5b0fe22

export: Corrections to Sw4STM32 after review

c13b80c

Add double quotes to linker command unconditionally.

export: Add missing flags to Sw4STM32

6b6187a

Some of the optimization and warning flags, which were supported by the gnuarmeclipse exporter, were missing from the sw4stm32.

ajaakkoh force-pushed the sw4stm_exporter_fix branch from f3cfda5 to 6b6187a Compare September 13, 2017 11:59

theotherjimmy added needs: CI and removed needs: work labels Sep 14, 2017

0xc0170 added ready for merge release-version: 5.6.1 and removed needs: CI labels Sep 22, 2017

0xc0170 merged commit c6f655c into ARMmbed:master Sep 22, 2017

0xc0170 removed the ready for merge label Sep 22, 2017

bcostm mentioned this pull request Dec 1, 2017

error: 4//tmp/tmpFumSHy/mbed_config.h: No such file or directory in import for SW4STM32 #5619

Closed


		ld_script = self.filter_dot(self.resources.linker_script)

		# self.lib_dirs = [self.filter_dot(s) for s in self.resources.lib_dirs]

Fixed sw4stm32 exporter #4779

Fixed sw4stm32 exporter #4779

Conversation

JarnoEtt commented Jul 19, 2017

Description

Status

Todos

Deploy notes

Steps to test or reproduce

Uh oh!

0xc0170 commented Jul 19, 2017

Uh oh!

JarnoEtt commented Jul 19, 2017

Uh oh!

0xc0170 commented Jul 19, 2017

Uh oh!

JarnoEtt commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nodraak Jul 19, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

JarnoEtt commented Jul 19, 2017

Uh oh!

Nodraak commented Jul 19, 2017

Uh oh!

JarnoEtt commented Jul 19, 2017

Uh oh!

theotherjimmy commented Jul 19, 2017

Uh oh!

theotherjimmy left a comment

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!

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!

theotherjimmy commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Aug 7, 2017

Uh oh!

JarnoEtt commented Aug 7, 2017

Uh oh!

theotherjimmy commented Aug 16, 2017

Uh oh!

theotherjimmy left a comment • 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!

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!

Nodraak Jul 19, 2017 •

edited

Loading

theotherjimmy commented Jul 19, 2017 •

edited

Loading

theotherjimmy commented Jul 26, 2017 •

edited

Loading

theotherjimmy left a comment •

edited

Loading