Add exporter for Analog Devices' CrossCore Embedded Studio #5666

mharringADI · 2017-12-06T14:55:11Z

Notes:

Pull requests will not be accepted until the submitter has agreed to the contributer agreement.

Description

This pull request adds an exporter for Analog Devices' CrossCore Embedded Studio (CCES) Eclipse-based IDE (http://www.analog.com/en/design-center/processors-and-dsp/evaluation-and-development-software/adswt-cces.html#dsp-overview).

Currently only supports Analog Devices (ADI) targets:

EVAL_ADICUP3029
EV_COG_AD3029LZ
EV_COG_AD4050LZ

Status

READY

Migrations

NO

Related PRs

None.

Todos

Sign CLA

Deploy notes

Requires CrossCore Embedded Studio 2.8.0 or newer to be installed (set to release in March 2018). 30-day evaluation licenses are available. Once installed, set CCES_HOME environment variable to point to the top level of the installation (i.e. "C:\Analog Devices\CrossCore Embedded Studio 2.8.0")

Steps to test or reproduce

Export a project to CCES:
mbed export -i cces -m EV_COG_AD4050LZ

theotherjimmy

Thanks for contributing a new exporter. I'm especially excited to see that it has a build method.

It looks like much of this exporter was copied then modified from the GNU ARM Eclipse exporter. Please inherit from the GNU ARM Eclipse exporter instead. Further code comments below.

theotherjimmy · 2017-12-07T20:52:58Z

tools/export/cces/__init__.py

+from tools.targets import TARGET_MAP
+from tools.export.exporters import Exporter
+
+class Option:


Could this class be a named tuple?

theotherjimmy · 2017-12-07T20:54:58Z

tools/export/cces/__init__.py

+        target_name - the name of the target.
+        """
+        target = TARGET_MAP[target_name]
+        return cls.TOOLCHAIN in target.supported_toolchains


From your description:

Currently only supports Analog Devices (ADI) targets:

EVAL_ADICUP3029

EV_COG_AD3029LZ

EV_COG_AD4050LZ

Could you make the code here check for that? It currently would try to export for K64F, for example.

theotherjimmy · 2017-12-07T21:02:39Z

tools/export/cces/__init__.py

+        booleans = {"-v": "arm.assembler.option.verbose",
+                    "-g": "arm.assembler.option.debuginfo"}
+
+        for flag in booleans:


Since you use both the key and the value from the booleans variable, you can iterate through the key, value pairs instead of doing double lookups?

for flag, setting in booleans.items(): value = "false" if flag in flags: value = "true" flags.remove(flag) options[setting] = Option("baseId", value)

theotherjimmy · 2017-12-07T21:04:19Z

tools/export/cces/__init__.py

+                    "-Werror": "arm.base.compiler.option.warningaserror",
+                    "-Wconversion": "arm.base.compiler.option.conversionwarning"}
+
+        for flag in booleans:


See the comment about booleans above. Given that this loop appeared twice in this code, could you extract it into it's own function?

theotherjimmy · 2017-12-07T21:04:49Z

tools/export/cces/__init__.py

+                    "-s": "arm.linker.option.strip",
+                    "-Wl,--gc-sections": "arm.linker.option.elimination"}
+
+        for flag in booleans:


Here's the 3rd invocation of the same thing. Please extract this snippet into it's own function.

theotherjimmy · 2017-12-07T21:05:43Z

tools/export/cces/__init__.py

+
+        self.resources.win_to_unix()
+
+        t = TARGET_MAP[self.target]


You should not create your own target object. Please use toolchain.target instead.

theotherjimmy · 2017-12-07T21:10:02Z

tools/export/cces/__init__.py

+            lib, ext = os.path.splitext(os.path.basename(libpath))
+            libs.append(lib[3:]) # skip 'lib' prefix
+
+        system_libs = [


This variable is part of the toolchain object. Please use toolchain.sys_libs.

theotherjimmy · 2017-12-07T21:12:33Z

tools/export/cces/__init__.py

+        self.gen_file('cces/cces.json.tmpl', jinja_ctx,
+                      json, trim_blocks=True, lstrip_blocks=True)
+
+        # import .json file using CCES headless builder


You cannot run IDE commands in an exporter. Please remove the following section of code. It looks like this was intended for the build method, and eventually made its way there.

Thanks for the review. I'm not sure I understand this comment but perhaps some clarification on our end will help. Here we generate the cces.json file as an input to our headlesstools application to generate the project files ('-command projectcreate'). There isn't any building done at this time - only project creation. The user can then import the project into the IDE for building / debugging, or use the build method (note the differences in command line arguments). Does this help clarify things, or am I misunderstanding your comment?

Does this help clarify things, or am I misunderstanding your comment?

Yes. This is still not allowed in exporters.

The online IDE does not have your IDE's executable installed. How can it be expected to handle this part of the export process?

Understood - I hadn't considered the online IDE. Thanks for the clarification.

mharringADI · 2017-12-18T20:03:56Z

Is there a way to invoke the exporter build command from the mbed command line? Wondering about the user experience after running the exporter but before importing into the IDE.

andrew-mclachlan · 2018-01-04T14:14:03Z

Is there a way to invoke the exporter build command from the mbed command line?

Not sure but maybe @theotherjimmy would know.

0xc0170 · 2018-01-04T14:18:25Z

Our exporters provide static method build we use for testing but it's not exposed via mbed cli.

cmonr · 2018-01-11T17:05:21Z

@mharringADI Has any progress been made on this PR?

If you have a PR in mbed-cli in progress, feel free to link it here for reference.

mharringADI · 2018-01-11T17:24:59Z

@cmonr We are making some changes internally and will post them here once reviewed. Apologies for the delays.

cmonr · 2018-02-05T16:42:20Z

@mharringADI, any updates?

cmonr · 2018-02-06T02:10:38Z

@theotherjimmy Mind taking another look?

0xc0170 · 2018-02-06T09:08:16Z

@mharringADI Thanks for the update. Could you please rebase instead of @mharringADI Merge with master. ? This is unnecessary commit

mharringADI · 2018-02-06T15:52:31Z

Thanks @0xc0170, I've rebased and pushed the changes.

andrew-mclachlan · 2018-02-06T20:05:33Z

Hi @theotherjimmy - could you let us know what the final change request is so that we can address it? It's not terribly clear to us. Thanks for your support with this update! Super appreciated!

andrew-mclachlan · 2018-02-09T19:34:43Z

Looks like we just need to resolve the conflict. Hopefully we're good after that.

…ME describing how to use headless tools after exporting.

mharringADI · 2018-02-09T19:55:28Z

All set - I've resolved the conflict. It doesn't look like the new changes have been reviewed yet.

cmonr · 2018-02-09T20:08:00Z

Ping @theotherjimmy

theotherjimmy

I suggested a few simplifications, and a method for better online compiler support.

theotherjimmy · 2018-02-13T15:43:40Z

tools/export/cces/__init__.py

+        remove = ["-c"]
+        for flag in remove:
+            if flag in flags:
+                flags.remove(flag)


These 3 lines should be a call to clean_flags

theotherjimmy · 2018-02-13T15:44:16Z

tools/export/cces/__init__.py

+        remove = ["-x", "assembler-with-cpp"]
+        for flag in remove:
+            if flag in flags:
+                flags.remove(flag)


These 3 lines should be a call to clean_flags

theotherjimmy · 2018-02-13T15:48:24Z

tools/export/cces/__init__.py

+                                            "WORKSPACE", project)
+        }
+
+        self.gen_file('cces/README.md.tmpl', jinja_ctx, "README.md")


As it stands, This file will be generated incorrectly in the online export system. You depend on the exporting machine's OS, which will always be Linux in the online compiler, irrespective the OS of the requesting computer. Could you change these files to contain instructions for all 3 OSs?

Thanks @theotherjimmy. The resulting README.md should be the same for all OSs - note that the full OS path isn't used for the jinja context since we don't know the client's OS. That said, having instructions for each OS seems reasonable.

My mistake. It looks like get_cces_path is only called from build (which will be called from the same machine invoking the build; no tricks there)

…tems. Reduced duplicate code.

theotherjimmy

Looks good. Thanks.

theotherjimmy · 2018-02-13T20:06:52Z

@mharringADI How can we add this exporter to CI?
@studavekar Another one for CI!

cmonr · 2018-02-13T20:10:23Z

/morph build

mbed-ci · 2018-02-13T20:42:27Z

Build : SUCCESS

Build number : 1129
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5666/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

mbed-ci · 2018-02-13T22:03:57Z

Test : SUCCESS

Build number : 937
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5666/937

mbed-ci · 2018-02-13T22:25:48Z

Exporter Build : SUCCESS

Build number : 806
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5666/

cmonr · 2018-02-13T23:39:20Z

Waiting on feedback from @mharringADI @studavekar on how to add to CI.

mharringADI · 2018-02-14T15:52:59Z

@theotherjimmy @cmonr I'm not familiar with your CI process, do you have any documentation available? In terms of testing the exporter with CCES, we are creating automated tests on our end that:

Install CCES
Import the mbed-os-example-blinky
Run the exporter (mbed export -i cces -m EV_COG_AD4050LZ)
Create the project / build it via the command line (see generated README)
Import the project into CCES IDE, build and test basic debug functions with hardware

theotherjimmy · 2018-02-14T15:58:22Z

@mharringADI We use a python script to automate steps 2-4. We use the build method for step 4. You could use the tools/test/examples/examples.py script to help automate that process.

mharringADI · 2018-02-14T17:07:09Z

Thanks @theotherjimmy. Looking at tools/test/examples/example_lib.py, it looks like your export_repos test should cover steps 2-4 above. The export should pass with these changes, but the build will fail unless you have CrossCore Embedded Studio 2.8.0 which is set to release in a few weeks.

theotherjimmy · 2018-02-14T17:08:26Z

@mharringADI Should we put this on hold? or would you rather merge now, and submit another PR to turn on CI for it when it's released?

mharringADI · 2018-02-14T19:55:45Z

I think it makes sense to merge now and I'll submit another PR for the CI once it's released. Thanks for all your efforts on this - much appreciated!

adbridge · 2018-02-23T16:45:42Z

This is in turn dependent on #5848 now targeted at 5.8

0xc0170 requested a review from theotherjimmy December 7, 2017 08:14

0xc0170 added the needs: review label Dec 7, 2017

theotherjimmy suggested changes Dec 7, 2017

View reviewed changes

theotherjimmy added needs: work and removed needs: review labels Dec 7, 2017

cmonr added needs: review and removed needs: work labels Feb 6, 2018

mharringADI force-pushed the feature-cces-exporter branch from c3b92a8 to 1f3a587 Compare February 6, 2018 15:51

0xc0170 changed the title ~~Added exporter for Analog Devices' CrossCore Embedded Studio~~ Add exporter for Analog Devices' CrossCore Embedded Studio Feb 8, 2018

mharringADI added 2 commits February 9, 2018 14:48

Added exporter for Analog Devices' CrossCore Embedded Studio

60b22db

Resolved pylint errors and made changes based on feedback. Added READ…

d93bdaf

…ME describing how to use headless tools after exporting.

mharringADI force-pushed the feature-cces-exporter branch from 1f3a587 to d93bdaf Compare February 9, 2018 19:50

theotherjimmy suggested changes Feb 13, 2018

View reviewed changes

cmonr added needs: work and removed needs: review labels Feb 13, 2018

Updated README.md to include instructions for different operating sys…

8c87c26

…tems. Reduced duplicate code.

theotherjimmy approved these changes Feb 13, 2018

View reviewed changes

theotherjimmy added needs: CI and removed needs: work labels Feb 13, 2018

cmonr added needs: review and removed needs: CI labels Feb 13, 2018

cmonr merged commit b48df25 into ARMmbed:master Feb 14, 2018

cmonr added needs: review release-version: 5.7.6 and removed needs: review labels Feb 14, 2018

adbridge added release-version: 5.8.0-rc1 and removed release-version: 5.7.6 labels Feb 23, 2018

Add exporter for Analog Devices' CrossCore Embedded Studio #5666

Add exporter for Analog Devices' CrossCore Embedded Studio #5666

Uh oh!

Conversation

mharringADI commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Migrations

Related PRs

Todos

Deploy notes

Steps to test or reproduce

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 Dec 7, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mharringADI commented Dec 18, 2017

Uh oh!

andrew-mclachlan commented Jan 4, 2018

Uh oh!

0xc0170 commented Jan 4, 2018

Uh oh!

cmonr commented Jan 11, 2018

Uh oh!

mharringADI commented Jan 11, 2018

Uh oh!

cmonr commented Feb 5, 2018

Uh oh!

cmonr commented Feb 6, 2018

Uh oh!

0xc0170 commented Feb 6, 2018

Uh oh!

mharringADI commented Feb 6, 2018

Uh oh!

andrew-mclachlan commented Feb 6, 2018

Uh oh!

andrew-mclachlan commented Feb 9, 2018

Uh oh!

mharringADI commented Feb 9, 2018

Uh oh!

cmonr commented Feb 9, 2018

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!

mharringADI commented Dec 6, 2017 •

edited

Loading

theotherjimmy Dec 7, 2017 •

edited

Loading

theotherjimmy commented Feb 14, 2018 •

edited

Loading

mharringADI commented Feb 14, 2018 •

edited

Loading