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 mbed-os 5 build support for LPC11C24 #6541

Merged
merged 2 commits into from Apr 16, 2018

Conversation

Projects
None yet
5 participants
@jorisa
Contributor

jorisa commented Apr 4, 2018

Description

Fix support for building LPC11C24.

Based on: https://os.mbed.com/blog/entry/Reducing-memory-usage-by-tuning-RTOS-con/

# .gitignore
rtos/*
features/FEATURE_CLIENT/*
features/FEATURE_COMMON_PAL/*
features/FEATURE_UVISOR/*
features/frameworks/*
features/cellular/*
features/lorawan/*
features/net/*
features/filesystem/*
features/netsocket/*
features/storage/*

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@0xc0170 0xc0170 requested a review from ashok-rao Apr 5, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 5, 2018

@jorisa Can you share the tests you run (using mbed test)

The change is minimal. what is actually fixing? an error?

@0xc0170 0xc0170 removed the request for review from ashok-rao Apr 5, 2018

@ashok-rao

This comment has been minimized.

Contributor

ashok-rao commented Apr 5, 2018

@jorisa .. how much RAM/Flash is present on the LPC11C24? Why do you want to remove the RTOS component here (via .mbedignore)? That optimisation needs to happen in the application based on requirements..
If this PR is only adding support for LPC11C24, then the change LGTM in mbed_rtx.h ..

Can you please clarify the need for this PR and also attach greentea logs from all 3 compilers here? Thanks.

@0xc0170 0xc0170 requested a review from ashok-rao Apr 5, 2018

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 5, 2018

@jorisa

This comment has been minimized.

Contributor

jorisa commented Apr 9, 2018

All, apologies for the lack of additional information with this pull-request.

The LPC11C24 has 32kB Flash / 8kB RAM. We are porting some projects from mbed2 to the latest mbed-os. By following the advice in the mentioned blog post we removed all rtos dependancies on the application level using mbedignore, making it resemble the classic mbed library. The only error that we ran in to was:

[Error] mbed_boot.c@219,6: #error "no target defined"
[Error] mbed_boot.c@245,55: 'INITIAL_SP' undeclared (first use in this function)
[ERROR] libs\mbed-os\rtos\TARGET_CORTEX\mbed_boot.c:219:6: error: #error "no target defined"
     #error "no target defined"

This value is also defined for TARGET_LPC11U24 which is a very similar part (also 32/8Kb). I don't have the infrastructure to test on all 3 compilers, but I verified that this fixes the error on gcc, and that the application runs as expected.

I amended the pull request to default to hex output, since this is what FlashMagic accepts (for the CAN bootloader).

Let me know if additional information is necessary.

As a separate note, would it be an idea to create a target config to exclude all the rtos stuff? I don't know how many other small targets exists that currently still need need to rely on the old library.

@0xc0170 0xc0170 added needs: review and removed needs: work labels Apr 9, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 10, 2018

I checked LPC11C24 it's not in the release (no 2 or 5 release version label in targets.json file).
The headline Add mbed-os 5 build support for LPC11C24 is misleading - it does not make much sense to add this target to mbed OS 5, does it?

@jorisa

This comment has been minimized.

Contributor

jorisa commented Apr 10, 2018

I don't know how you define 'supported'. Currently you cannot build this target with mbed-os, with this fix you can. We use a bunch of targets on mbed5 and would prefer to build everything from the same mbed-os library version. I am happy to change the title to something you find more appropriate.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 11, 2018

I don't know how you define 'supported'.

release_version in targets.json file for a target defined

We use a bunch of targets on mbed5 and would prefer to build everything from the same mbed-os library version. I am happy to change the title to something you find more appropriate.

Sounds fine to me

@0xc0170

Just a recheck - hex is supported by target?

@ashok-rao

This comment has been minimized.

Contributor

ashok-rao commented Apr 11, 2018

with this fix you can.

@0xc0170 , without release_version defined in targets.json for this target, how is it able to build for OS 5? release_version must be defined, correct? Or is it the case that build system takes OS 5 as default even if release_version is not specified?

@jorisa

This comment has been minimized.

Contributor

jorisa commented Apr 13, 2018

@0xc0170 hex is supported on LPC11C24,

This is how we invoke the build:

[mbed] Exec "C:\Python27\python.exe -u libs\mbed-os\tools\make.py -D -t GCC_ARM -m LPC11C24 --profile profiles\debug.json --source projects\blinky --source libs\mbed-os --build projects\blinky\build\debug --app-config projects\blinky\mbed_app.json"

I assume it defaults to supporting OS 5.

Again to clarify, this is to allow building both "classic" (rtos-less) applications and os5 applications from the same library code base.

Is there a reason why you would want the classic library at all considering this seems to work fine?

Do I need to do something to run the remaining checks?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 13, 2018

@0xc0170 , without release_version defined in targets.json for this target, how is it able to build for OS 5? release_version must be defined, correct? Or is it the case that build system takes OS 5 as default even if release_version is not specified?

That specifies if it is released or not (target part of mbed 2 lib for instance or published in the targets supported, etc) - that is my understanding. You can still build locally using tools for instance, as @jorisa is doing.

@jorisa This patch looks fine to fine, will run CI. Out of curiosity, with the footprint this target has - what are you going to run there? I would imagine just having single threaded app and even then its limited?

/morph build

@jorisa

This comment has been minimized.

Contributor

jorisa commented Apr 13, 2018

It's for a simple CAN based I/O expander, indeed single thread. We use the ROM CAN bootloader with MagicFlash to load the firmware.
We are migrating to mbed-os 5 on more powerful targets, but we like to run our CI for all our projects on the same version of mbed library/build tools, so this would let us build the "legacy" projects.

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 13, 2018

Build : SUCCESS

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

Triggering tests

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

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Apr 13, 2018

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 14, 2018

/morph mbed2-build

1 similar comment
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 16, 2018

/morph mbed2-build

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Apr 16, 2018

@cmonr cmonr merged commit 4ff1a49 into ARMmbed:master Apr 16, 2018

12 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build 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
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 8849 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Apr 16, 2018

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