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

Add exporter for Analog Devices' CrossCore Embedded Studio #5666

Merged
merged 3 commits into from Feb 14, 2018

Conversation

Projects
None yet
7 participants
@mharringADI
Contributor

mharringADI commented Dec 6, 2017

Notes:

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

@0xc0170 0xc0170 requested a review from theotherjimmy Dec 7, 2017

@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.

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Dec 7, 2017

Contributor

Could this class be a named tuple?

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Dec 7, 2017

Contributor

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.

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Dec 7, 2017

Contributor

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)
"-Werror": "arm.base.compiler.option.warningaserror",
"-Wconversion": "arm.base.compiler.option.conversionwarning"}
for flag in booleans:

This comment has been minimized.

@theotherjimmy

theotherjimmy Dec 7, 2017

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Dec 7, 2017

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Dec 7, 2017

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Dec 7, 2017

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Dec 7, 2017

Contributor

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.

This comment has been minimized.

@mharringADI

mharringADI Dec 7, 2017

Contributor

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?

This comment has been minimized.

@theotherjimmy

theotherjimmy Dec 7, 2017

Contributor

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?

This comment has been minimized.

@mharringADI

mharringADI Dec 7, 2017

Contributor

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

@mharringADI

This comment has been minimized.

Contributor

mharringADI commented Dec 18, 2017

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

This comment has been minimized.

andrew-mclachlan commented Jan 4, 2018

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

Not sure but maybe @theotherjimmy would know.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 4, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 11, 2018

@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

This comment has been minimized.

Contributor

mharringADI commented Jan 11, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 5, 2018

@mharringADI, any updates?

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 6, 2018

@theotherjimmy Mind taking another look?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 6, 2018

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

@mharringADI mharringADI force-pushed the mharringADI:feature-cces-exporter branch from c3b92a8 to 1f3a587 Feb 6, 2018

@mharringADI

This comment has been minimized.

Contributor

mharringADI commented Feb 6, 2018

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

@andrew-mclachlan

This comment has been minimized.

andrew-mclachlan commented Feb 6, 2018

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!

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

@andrew-mclachlan

This comment has been minimized.

andrew-mclachlan commented Feb 9, 2018

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

@mharringADI mharringADI force-pushed the mharringADI:feature-cces-exporter branch from 1f3a587 to d93bdaf Feb 9, 2018

@mharringADI

This comment has been minimized.

Contributor

mharringADI commented Feb 9, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 9, 2018

Ping @theotherjimmy

@theotherjimmy

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Feb 13, 2018

Contributor

These 3 lines should be a call to clean_flags

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Feb 13, 2018

Contributor

These 3 lines should be a call to clean_flags

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Feb 13, 2018

Contributor

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?

This comment has been minimized.

@mharringADI

mharringADI Feb 13, 2018

Contributor

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.

This comment has been minimized.

@theotherjimmy

theotherjimmy Feb 13, 2018

Contributor

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)

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

@theotherjimmy

Looks good. Thanks.

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Feb 13, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 13, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 13, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 13, 2018

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

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

@mharringADI

This comment has been minimized.

Contributor

mharringADI commented Feb 14, 2018

@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

This comment has been minimized.

Contributor

theotherjimmy commented Feb 14, 2018

@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

This comment has been minimized.

Contributor

mharringADI commented Feb 14, 2018

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

This comment has been minimized.

Contributor

theotherjimmy commented Feb 14, 2018

@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

This comment has been minimized.

Contributor

mharringADI commented Feb 14, 2018

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!

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

17 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mbed-ci-generic Build finished.
Details
travis-ci/docs/ Local docs testing has passed
Details
travis-ci/events/ Local events testing has passed
Details
travis-ci/littlefs/ Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL/ Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM/ Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC/ Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON/ Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP/ Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-SILICON_LABS/ Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM/ Local mbed2-STM testing has passed
Details
@adbridge

This comment has been minimized.

Contributor

adbridge commented Feb 23, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment