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 mcuxpresso exporter #4916

Merged
merged 27 commits into from Sep 8, 2017

Conversation

Projects
None yet
7 participants
@JojoS62
Contributor

JojoS62 commented Aug 15, 2017

Description

Add an exporter for MCUXpresso (NXP), successor of LPCXpresso

Status

IN DEVELOPMENT
needs work

  • base is gnuarmecplipse exporter
  • Works for the targets that LPCXpresso supported + LPC54114
  • LPC54608 compiles as well, but the MCU settings for debugging are not yet available.
  • Supports debug and release builds, develop not yet working
  • flags are read from mbed build system and converted to eclipse tools settings

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

NO APIs changed

Related PRs

#4819

Todos

  • Tests
  • Documentation

Deploy notes

needs installation of MCUXpresso, tested with version 10.0.2

Steps to test or reproduce

create a project file with 'mbed export -i mcuxpresso'
import the created project.zip
** Note: this version always creates a zip file for faster testing. Needs a clean solution for creating a zip file as an option.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 15, 2017

Awesome work @JojoS62

It looks like your rebase included some commits from master somehow. Could you remove those duplicated commits from this PR?

@theotherjimmy

It looks like you copied the contents of the gnuarmeclipse exporter. To prevent duplication of fixes, could you import it and override things instead?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 15, 2017

Needs a clean solution for creating a zip file as an option.

I'll get that for you as another PR real quick.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 15, 2017

@studavekar Can we get the deploy notes installed in CI? Could you let us know when that's Done?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 15, 2017

@JojoS62 Let me know if you would like some help with this. @mmahadevan108 FYI.

@JojoS62

This comment has been minimized.

Contributor

JojoS62 commented Aug 15, 2017

@theotherjimmy I tried git rebase origin but this returned errors:

Applying: Renamed so that we have one configuration for all STM32F439 targets.
Using index info to reconstruct a base tree...
A       features/mbedtls/targets/TARGET_STM/TARGET_STM32F4/TARGET_NUCLEO_F439ZI/mbedtls_device.h
.git/rebase-apply/patch:43: trailing whitespace.
 *  mbedtls_device.h
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
CONFLICT (rename/rename): Rename "features/mbedtls/targets/TARGET_STM/TARGET_STM32F4/TARGET_NUCLEO_F439ZI/mbedtls_device
.h"->"features/mbedtls/targets/TARGET_STM/TARGET_STM32F7/TARGET_NUCLEO_F756ZG/mbedtls_device.h" in branch "HEAD" rename
"features/mbedtls/targets/TARGET_STM/TARGET_STM32F4/TARGET_NUCLEO_F439ZI/mbedtls_device.h"->"features/mbedtls/targets/TA
RGET_STM/TARGET_STM32F4/TARGET_STM32F439xI/mbedtls_device.h" in "Renamed so that we have one configuration for all STM32
F439 targets."
error: Failed to merge in the changes.
Patch failed at 0013 Renamed so that we have one configuration for all STM32F439 targets.
The copy of the patch that failed is found in: .git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

What do have I to do with this patch file or to resolve the problem?

patch.zip

Add support for MCUXpresso inside mbed exporter
Signed-off-by: Mahadevan Mahesh <Mahesh.Mahadevan@nxp.com>
@JojoS62

This comment has been minimized.

Contributor

JojoS62 commented Aug 16, 2017

@theotherjimmy I'm working on deriving from the gae class, but I will have some questions...
POST_BINARY_WHITELIST : for what is this used?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 16, 2017

I'm working on deriving from the gae class

+💯

POST_BINARY_WHITELIST : for what is this used?

That used to help generate a list of supported targets. When using a fixed list of supported targets, you should not have to worry about it. The export API now uses the class method is_target_supported and the class method all_supported_targets to examine support. This elides lots of unused computation (it speed up exporting by a factor of 2 on my machine), and simplifies most exporters supported check implementation. The whitelist is used by apply_supported_whitelist and that method is used by exporters who's supported targets list grows with every new target. Exporters with fixed supported lists should not have to worry about it.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 16, 2017

@JojoS62 I forgot to respond to your prior comment. I'm sorry. I'll get right on that.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 16, 2017

Oh Dang, your history looks pretty duplicated. It looks like you rebased then tried to git push but git told you "Your history does not match, do this pull thing first" which you should NOT do. I understand that you may not have known that at the time, so I'm going to see what I can do to help you correct this mishap.

  1. BACK UP THE CURRENT BRANCH This is important because we are going to rewrite history (we're the winners) git checkout -b old-add_MCUXpresso_exporter
  2. Checkout the commit before the merge (It should be HEAD^) git checkout HEAD^
  3. Delete the branch git branch -D add_MCUXpresso_exporter
  4. Recreate the branch on this commit git checkout -b add_MCUXpresso_exporter
  5. Update this PR via force push git push -u <your-remote-here > -f

If something goes horribly wrong from steps 2-5, you can restore your branch to it's former glory with:

  1. Change to the backup branch git checkout old-add_MCUXpresso_exporter
  2. Delete the current monstrosity git branch -D add_MCUXpresso_exporter
  3. Restore former glory from backup git checkout -b add_MCUXpresso_exporer
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 16, 2017

BTW, I tried the steps 1-4, and the history looks good.

@JojoS62

This comment has been minimized.

Contributor

JojoS62 commented Aug 16, 2017

Thanks, ok, I'll try to clean up. I made my old mistake again to merge updates in my branch because the zip export failed. So I'd better use two branches, one with only my work on something and another one for merging this and other stuff?
I have read your answer about the whitelist after I implemented the template file exist method. This works and the list of supported targets can be extended without changing the exporters code. But maybe the filesystem checks are too expensive for the use in the online compiler?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 16, 2017

Correctness over liveness, every time. I'll take a look.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 16, 2017

Don't worry about checking for existence, that's pretty cheap compared to scanning all repos that make up the project and zipping all of that. I bet you would have trouble measuring the difference anyway.

@JojoS62

This comment has been minimized.

Contributor

JojoS62 commented Aug 16, 2017

´(It should be HEAD^) : has ^to be replaced by something? The command line aks for 'more?' after this command.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 16, 2017

Hmm... That works for me. Try the exact hash.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 16, 2017

For example git checkout 3aafb2d

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 16, 2017

It looks like you're still adding commits. You may have to cherry pick them after you are done with the process above.

@JojoS62 JojoS62 force-pushed the JojoS62:add_MCUXpresso_exporter branch to 8ae5eda Aug 16, 2017

@JojoS62

This comment has been minimized.

Contributor

JojoS62 commented Aug 16, 2017

Now the PR contains only the one from 'git checkout sha'. I have added only the one commit to get the working stage clean.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 16, 2017

Looks like a good start. Are you planning on cherry-picking the other commits and pushing them up now?

@JojoS62

This comment has been minimized.

Contributor

JojoS62 commented Aug 16, 2017

I'll try

JojoS62 added some commits Aug 4, 2017

exporter based on lpc54114 template from nxpmicro
- used LPC4088 for testing
- split template for LPC54114 into common and Cortex-Mx dependet parts like in old LPCXpresso exporter
- changed fixed linker script name to name from options
Todo: use flags and configurations from options (like in gae exporter)
changed template to gae exporter style
creates output for debug, develop and release builds
TODO. replaces some fixed flags, develop build not working , linker has no input files
make excluded_folders POSIX conform
change backslash to forward slash in excluded_folders set
created project compiles in MCUX
project is read, but still fixes necessary:
- archticture is fixed entry, read from config
- linker options
- linker script file ?
- linker settings 'managed build' page causes error
- develop still not working
added generated flags
target family, fpu
removed 'develop' proffile, it causes errors in import
added templates for LPC4088 and LPC11H37
target specific templates use a  common template .cproject.tmpl and extend it by adding target information. This is necessary for the flash and debugging tools
@mmahadevan108

This comment has been minimized.

Contributor

mmahadevan108 commented Aug 31, 2017

@JojoS62 Below is a patch for platforms (e.g: K64F, LPC54608) that are supported via the SDK being installed in MCUXpresso. Without this change, build will work but failures will be seen during launch. Can this be included. This is the last patch from me. Thanks.
0002-Add-support-to-specify-SDK-name-as-this-is-needed-fo.zip

JojoS62 added some commits Sep 1, 2017

applied patch from mmahadevan108
- fixes invalid char '#' in linker script
- added 'mbedclean'
add support for K64F target
added template for K64F
added support for targets with extra SDK
some targets need an additional SDK.
@JojoS62

This comment has been minimized.

Contributor

JojoS62 commented Sep 1, 2017

@mmahadevan108

Can this be included. This is the last patch from me. Thanks.

no problem, you are welcome. I'm happy when this exporter gets more attention and support.
I just haven't used the Kinetis boards yet, its great when the exporter works for them too.
Thanks.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

@JojoS62 What is the status of this patch?

@JojoS62

This comment has been minimized.

Contributor

JojoS62 commented Sep 4, 2017

Compiling the K64F target works, thats what I could test.
The review marker is still set because @theotherjimmy wanted to check something in the test procedure, the test for this exporter did not run.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 5, 2017

@0xc0170 @JojoS62 Sorry about the delay. as the saying goes: "when you're up to your neck in alligators, it's hard to remember that your initial objective was to drain the swamp" That's been dealing with CI this past week or two.

@maclobdell

This comment has been minimized.

Contributor

maclobdell commented Sep 5, 2017

@theotherjimmy - pretty please make this a top priority to test and merge when ready. We would like this to be part of the next mbed OS release (in 2 weeks?), so we can use it in internal training in 3 weeks. Thanks a lot @JojoS62 and @mmahadevan108 for all your work on this!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 6, 2017

@JojoS62 @mmahadevan108 @0xc0170 I'm going to run what the CI job would do on my machine, and mark this as ready for merge if that passes.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 6, 2017

Hey guys,

I ran this through a local version of CI on my machine, and it looks good. I had to modify the success check and add some missing imports. I'm going to post the results shortly, and we can elide an export build on this one until we have this eclipse variant installed in CI.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 6, 2017

Examples that passed:

  • mbed-os-example-ble-BatteryLevel
  • mbed-os-example-ble-Beacon
  • mbed-os-example-ble-Button
  • mbed-os-example-ble-EddystoneObserver
  • mbed-os-example-ble-EddystoneService
  • mbed-os-example-ble-GAPButton
  • mbed-os-example-ble-HeartRate
  • mbed-os-example-ble-LED
  • mbed-os-example-ble-LEDBlinker
  • mbed-os-example-ble-Thermometer
  • mbed-os-example-blinky
  • mbed-os-example-bootloader
  • mbed-os-example-cellular
  • mbed-os-example-client
  • mbed-os-example-fat-filesystem
  • mbed-os-example-mesh-minimal
  • mbed-os-example-sockets
  • mbed-os-example-tls-authcrypt
  • mbed-os-example-tls-benchmark
  • mbed-os-example-tls-hashing
  • mbed-os-example-tls-tls-client
  • mbed-os-example-uvisor
  • mbed-os-example-uvisor-thread
  • mbed-os-example-wifi
  • nanostack-border-router

No examples failed to build.

Tested with:

  • K64F
  • LPC54114
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 6, 2017

@0xc0170 marking this as ready for merge speculatively. Please check that the default checks have run on this PR, and then it's ready.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 7, 2017

@theotherjimmy This needs exporters tests again (new commits)

@0xc0170 0xc0170 added needs: CI and removed ready for merge labels Sep 7, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 7, 2017

Uh, nope. This exporter is not yet it CI, and I posted the test report above.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 7, 2017

There is no benefit to running an export-build here, as this exporter is not covered.

@theotherjimmy theotherjimmy merged commit a108adf into ARMmbed:master Sep 8, 2017

3 checks passed

Cam-CI uvisor Build & Test Success
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment