Skip to content
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 support for debug and program launch configurations #9342

Merged
merged 8 commits into from Feb 1, 2019

Conversation

Projects
None yet
7 participants
@Cypress-OpenOCD
Copy link
Contributor

Cypress-OpenOCD commented Jan 10, 2019

Description

As of now the only one launch configuration is generated for the Eclipse which cannot cover all use cases. This PR is intended to extend support for different launch configurations (program, debug and erase) for the "cdt" exporter.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@theotherjimmy

@cmonr cmonr requested a review from ARMmbed/mbed-os-tools Jan 10, 2019

@cmonr

cmonr approved these changes Jan 10, 2019

Copy link
Contributor

cmonr left a comment

LGTM

@cmonr cmonr requested a review from ARMmbed/mbed-os-tools Jan 10, 2019

@cmonr cmonr added the needs: review label Jan 10, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Jan 10, 2019

LGTM, but afaik, we're not testing this specific exporter in CI.

@Cypress-OpenOCD Would you happen to have an example of something that this PR fixes/makes working? Something like a log or screenshot?

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Jan 10, 2019

@Cypress-OpenOCD @cmonr This exporter is named eclipse_<toolchain> on the command line.

tools/export/cdt/__init__.py Outdated
templates = ['%s.tmpl' % (self.target.lower())] + \
['%s.tmpl' % (label.lower()) for label
in self.toolchain.target.extra_labels] + \
['%s.tmpl' % 'pyocd_settings']

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 10, 2019

Contributor

As there are no target-specific templates, could we use the following instead:

templates = ["pyocd_settings.tmpl", "pyocd_settings_debug.tmpl", "pyocd_settings_program.tmpl"]

This comment has been minimized.

@rbatyuk

rbatyuk Jan 11, 2019

@theotherjimmy, I suggest to have 3 default templates: templates = ["pyocd_settings_debug.tmpl", "pyocd_settings_program.tmpl", "pyocd_settings_erase.tmpl"] . As I understood, the specific parameters should go through JSON file.

tools/export/cdt/__init__.py Outdated
try:
self.gen_file('cdt/%s' % templatefile, ctx, join('eclipse-extras',
'{target}_{project}_{launch}.launch'.format(target=self.target,
project=self.project_name, launch=templatefile)))

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 10, 2019

Contributor

Indentation seems off here. Could you move target= onto it's own line and reflow the other arguments?

tools/export/cdt/__init__.py Outdated
self.gen_file('cdt/%s' % templatefile, ctx, join('eclipse-extras',
'{target}_{project}_{launch}.launch'.format(target=self.target,
project=self.project_name, launch=templatefile)))
except TemplateNotFound:

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 10, 2019

Contributor

Just thought of this: without target-specific templates, there is no chance of missing templates. Could you get rid of the try-except here too?

tools/export/cdt/pyocd_settings_debug.tmpl Outdated
<stringAttribute key="ilg.gnuarmeclipse.debug.gdbjtag.pyocd.gdbServerExecutable" value="${pyocd_path}/${pyocd_executable}"/>
<booleanAttribute key="ilg.gnuarmeclipse.debug.gdbjtag.pyocd.gdbServerFlashFastVerify" value="false"/>
<intAttribute key="ilg.gnuarmeclipse.debug.gdbjtag.pyocd.gdbServerFlashMode" value="2"/>
<intAttribute key="ilg.gnuarmeclipse.debug.gdbjtag.pyocd.gdbServerGdbPortNumber" value="3334"/>

This comment has been minimized.

@rbatyuk

rbatyuk Jan 11, 2019

the default template of pyocd_settings_debug.tml should be the same as pyocd_settings.tmpl. Specific port 3334 should be in the json file

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 11, 2019

Contributor

@rbatyuk Could you specify which json file you're referring to?

tools/export/cdt/pyocd_settings_debug.tmpl Outdated
<booleanAttribute key="ilg.gnuarmeclipse.debug.gdbjtag.pyocd.gdbServerHaltAtHardFault" value="false"/>
<stringAttribute key="ilg.gnuarmeclipse.debug.gdbjtag.pyocd.gdbServerLog" value=""/>
<!-- Remove target name(-t cy8c6xxa) once we have DAPLink support in KP3 -->
<stringAttribute key="ilg.gnuarmeclipse.debug.gdbjtag.pyocd.gdbServerOther" value="-t cy8c6xxa&#13;&#10;-p 3333&#13;&#10;--no-deprecation-warning"/>

This comment has been minimized.

@rbatyuk

rbatyuk Jan 11, 2019

the name of target should not be included in the default template

@Cypress-OpenOCD

This comment has been minimized.

Copy link
Contributor Author

Cypress-OpenOCD commented Jan 11, 2019

@rbatyuk, agree, there are some psoc6-related implementation in the templates. I cannot see any way to make 'generic' template without having some sort of set of target specific parameters somewhere, since some targets may require specific reset sequence etc. I am thinking about the similar approach that is used for the iar exporter. There are a set of defaults which can be redefined for the specific target (tools/export/iar/iar_definitions.json). If we make similar JSON for Eclipse we have single template per each use case (e.g. debug, program, erase) where some parameters may be added or redefined with their own flow.
@cmonr @theotherjimmy is this worth doing this?

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Jan 11, 2019

@Cypress-OpenOCD I prefer having a system like iar_defines.json to having multiple templates.

@Cypress-OpenOCD

This comment has been minimized.

Copy link
Contributor Author

Cypress-OpenOCD commented Jan 11, 2019

@Cypress-OpenOCD I prefer having a system like iar_defines.json to having multiple templates.

I am already working on that, looks like we could end up with a single template.

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Jan 11, 2019

looks like we could end up with a single template.

I'm excited to see that! Will it be one template total? or 3, one each for debug/program/erase?

@Cypress-OpenOCD

This comment has been minimized.

Copy link
Contributor Author

Cypress-OpenOCD commented Jan 11, 2019

.

looks like we could end up with a single template.

I'm excited to see that! Will it be one template total? or 3, one each for debug/program/erase?

Single template for all use cases. The concrete details would be implemented in the json. Hope it will work ;)

@Cypress-OpenOCD

This comment has been minimized.

Copy link
Contributor Author

Cypress-OpenOCD commented Jan 14, 2019

Hi, please check out the reworked version of program/debug launch configurations support. In short: now there is single template for all launch configurations. Default and target-specific data comes from cdt_definitions.json. Thus, if some target require any custom sequence it may define it in the JSON file (like for CY8CPROTO_062_4343W).

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Jan 14, 2019

Please rebase your PR.

A fix for the travis-ci/tools-py2.7 was brought in today (#9371) due to an update to the hypothesis module used in CI: https://github.com/HypothesisWorks/hypothesis/releases/tag/hypothesis-python-4.0.0

@Cypress-OpenOCD Cypress-OpenOCD force-pushed the Cypress-OpenOCD:master branch to 7a85ae7 Jan 15, 2019

@theotherjimmy
Copy link
Contributor

theotherjimmy left a comment

Minor thing, then mention me so that I can approve and we can get this improvement in.

Show resolved Hide resolved tools/export/cdt/__init__.py
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 15, 2019

[X] Functionality change

I labeled this for 5.11.3 , please review @theotherjimmy

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Jan 15, 2019

@0xc0170 I'm already reviewing this PR.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 15, 2019

@0xc0170 I'm already reviewing this PR.

I was asking for review the label (if this can go into 5.11.3 or not)

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Jan 15, 2019

@0xc0170 Thanks for the clarification.

Yes, 5.11.3 is a fine target.

@theotherjimmy
Copy link
Contributor

theotherjimmy left a comment

Ya know what? I can make that change later. As pointed out, I missed the use of self. approval still stands.

@Cypress-OpenOCD

This comment has been minimized.

Copy link
Contributor Author

Cypress-OpenOCD commented Jan 15, 2019

Ok, thanks.

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Jan 15, 2019

@0xc0170

I was asking for review the label (if this can go into 5.11.3 or not)

The files are all focused around a specific vendor's exporter. It's not as clean as the "is the change udner TARGETS/*" soft rule that we use, but this should definitely be fine for 5.11.3.

@cmonr cmonr added the needs: CI label Jan 16, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 17, 2019

CI started

@cmonr cmonr added needs: work and removed needs: CI labels Jan 17, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Jan 17, 2019

Whoops. Started CI too soon, it's being stopped in the backgound.

@Cypress-OpenOCD

This comment has been minimized.

Copy link
Contributor Author

Cypress-OpenOCD commented Jan 17, 2019

We are going to add GNU MCU Eclipse plugin support. It would be better to track the changes here.

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Jan 17, 2019

Test run: FAILED

Summary: 6 of 7 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
@Cypress-OpenOCD

This comment has been minimized.

Copy link
Contributor Author

Cypress-OpenOCD commented Jan 18, 2019

Please review the changes made to support new GNU MCU Eclipse plug-in.
Changes:

  1. Added new template to support GNU MCU Eclipse plug-in
  2. Changed naming convention for new configurations:
    • GNU ARM Eclipse - {target}_{project}_{configuration}_pyocd_settings.launch (remains the same)
    • GNU MCU Eclipse - {target}_{project}_{configuration}.launch (to distinguish from GNU ARM Eclipse)

With these changes mbed export generates two sets of launch configurations: compatible with GNU ARM Eclipse, and GNU MCU Eclipse, therefore users may use plug-in they prefer.

We are going to add few more kits on Monday, which may affect cdt_definitions.json.

@cmonr cmonr added needs: review and removed needs: work labels Jan 18, 2019

Changes:
- added new erase launch configuration
- added new kits support
- code cleanup
@Cypress-OpenOCD

This comment has been minimized.

Copy link
Contributor Author

Cypress-OpenOCD commented Jan 30, 2019

Overview of this pull request:

  1. Extended support of PyOCD launch configurations for Eclipse:
  • added support of GNU MCU Eclipse plug-in (launch configurations generates now for both plug-ins)
  • added 2 new launch configurations: program and erase
  1. Made launch configurations template flexible and extendable:
  • created cdt_definitions.json where launch configuration options may be extended for specific targets
  • __init__.py uses cdt_definitions.json to fill the launch templates

@theotherjimmy please review

@theotherjimmy
Copy link
Contributor

theotherjimmy left a comment

Most of these comments are style nits or online compiler tweaks. The online compiler will load this file once and use it for many, many exporters. Therefore it is important to make sure that you don't modify any global state within the exporter to avoid accidentally copying settings between users exporting.

Show resolved Hide resolved tools/export/cdt/__init__.py Outdated
Show resolved Hide resolved tools/export/cdt/__init__.py Outdated
Show resolved Hide resolved tools/export/cdt/__init__.py Outdated
Show resolved Hide resolved tools/export/cdt/__init__.py Outdated
Show resolved Hide resolved tools/export/cdt/__init__.py Outdated
Show resolved Hide resolved tools/export/cdt/__init__.py Outdated
Show resolved Hide resolved tools/export/cdt/__init__.py Outdated

theotherjimmy and others added some commits Jan 31, 2019

Apply suggestions from code review
Co-Authored-By: Cypress-OpenOCD <39907069+Cypress-OpenOCD@users.noreply.github.com>
class Eclipse(Makefile):
"""Generic Eclipse project. Intended to be subclassed by classes that
specify a type of Makefile.
"""
def get_target_config(self, ctx, configuration):
"""Retrieve info from cdt_definitions.json"""
tgt = deepcopy(TARGET_MAP[self.target])

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 31, 2019

Contributor

I don't think you need this deepcopy. Further, I think you only use tgt.name which is always the same as self.target.

def get_target_config(self, ctx, configuration):
"""Retrieve info from cdt_definitions.json"""
tgt = deepcopy(TARGET_MAP[self.target])
defaults = deepcopy(_CONFIGS_OPTIONS['default'])

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 31, 2019

Contributor

I don't think you modify defaults, so this copy may not be needed.

This comment has been minimized.

@Cypress-OpenOCD

Cypress-OpenOCD Jan 31, 2019

Author Contributor

Ok, I think this may be kept to prevent any issues in case future updates

@theotherjimmy
Copy link
Contributor

theotherjimmy left a comment

Looks good. Previous comments are not blocking.

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Jan 31, 2019

CI started

@cmonr cmonr added needs: CI and removed needs: review labels Jan 31, 2019

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Jan 31, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 3
Build artifacts

@cmonr cmonr added ready for merge and removed needs: CI labels Jan 31, 2019

@0xc0170 0xc0170 merged commit 7ba83a5 into ARMmbed:master Feb 1, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10536 cycles (+1304 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Feb 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.