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

Update Mbed 5 boot sequence #7794

Merged
merged 2 commits into from Aug 30, 2018

Conversation

Projects
None yet
@c1728p9
Contributor

c1728p9 commented Aug 14, 2018

Description

Refactor the Mbed 5 boot process to make is simpler and more modular. This is done by breaking the boot sequence into 4 distinct steps - Target setup, Toolchain setup, RTOS setup and Mbed setup. This patch also move toolchain specific code into a per toolchain folder to make it more maintainable.

Pull request type

[ ] Fix
[x] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change

@c1728p9 c1728p9 requested review from mprse and bulislaw Aug 14, 2018

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Aug 15, 2018

Docs for this change can be found here:
ARMmbed/mbed-os-5-docs#662

@amq

This comment has been minimized.

Contributor

amq commented Aug 15, 2018

Is this related? #7382

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Aug 15, 2018

this is unrelated

@bridadan

I realize I'm not on the reviewers list but I was perusing some of the PRs and noticed a couple of comment oopsies in this one! Very cool change though!

/**
* SDK hook for running code before ctors or OS
*
* This is a weak function which can be overridden at by a target's

This comment has been minimized.

@bridadan

bridadan Aug 15, 2018

Contributor

English nit:

This is a weak function which can be overridden at by a target's

should probably just be "by".

/**
* Application hook for running code before main
*
* This is a weak function which can be overridden at by an application

This comment has been minimized.

@bridadan

bridadan Aug 15, 2018

Contributor

English nit:

This is a weak function which can be overridden at by an application

should probably just be "by".

@c1728p9 c1728p9 force-pushed the c1728p9:boot_overhaul branch from 4d5bbf8 to 0556c8e Aug 15, 2018

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Aug 15, 2018

Thanks for the review @bridadan. I update the PR to fix those typos.

@@ -43,6 +43,10 @@ extern osMutexId_t singleton_mutex_id;
inline static void singleton_lock(void)
{
#ifdef MBED_CONF_RTOS_PRESENT
if (!singleton_mutex_id) {
// RTOS has not booted yet so no mutex is needed
return;

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 20, 2018

Contributor

Should we throw an MBED_ERROR here?

This comment has been minimized.

@c1728p9

c1728p9 Aug 21, 2018

Contributor

nope, this case is expected to be hit early in the boot sequence when when the RTOS has not yet started.

/* Define stack sizes if they haven't been set already */
#if !defined(ISR_STACK_SIZE)
#define ISR_STACK_SIZE ((uint32_t)1024)

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 20, 2018

Contributor

Define is used by ARM and GCC_ARM toolchain, can we move it to "mbed_boot.h" ?

This comment has been minimized.

@c1728p9

c1728p9 Aug 21, 2018

Contributor

done

@deepikabhavnani

Thanks... this was much needed refactoring 👍

void mbed_toolchain_init()
{
#if defined(MBED_CONF_RTOS_PRESENT)

This comment has been minimized.

@bulislaw

bulislaw Aug 21, 2018

Member

I don't think we need this checks, nothing in rtos/ folder will be compiled in if the RTOS is disabled.

This comment has been minimized.

@c1728p9

c1728p9 Aug 21, 2018

Contributor

done

@@ -0,0 +1,159 @@
/* mbed Microcontroller Library

This comment has been minimized.

@bulislaw

bulislaw Aug 21, 2018

Member

You are using doxy, but it's not going to be anchored nicely. Is it worth creating a group and turning all the comments (even the file global one) into doxy?

This comment has been minimized.

@c1728p9

c1728p9 Aug 21, 2018

Contributor

done

* - The RTOS has been started by a call to mbed_rtos_start
* - The toolchain has been initialized by a call to mbed_toolchain_init
*/
void mbed_main(void);

This comment has been minimized.

@bulislaw

bulislaw Aug 21, 2018

Member

I'm guessing for compatibility reasons that need to stay, but from the conceptual point of view it doesn't have much sense as it's run from the same context than main. Would it make sense to add app entry point before RTOS and target entry point after RTOS?

This comment has been minimized.

@c1728p9

c1728p9 Aug 21, 2018

Contributor

It would be easy to add these hooks, although I would be hesitant to add them without having a use case in mind.

}
osKernelStart();
while (1); // Code should never get here

This comment has been minimized.

@bulislaw

bulislaw Aug 21, 2018

Member

Actually should we assert/error out here, rather than spin forever in case something goes wrong in RTX?

This comment has been minimized.

@c1728p9

c1728p9 Aug 21, 2018

Contributor

done

@c1728p9 c1728p9 force-pushed the c1728p9:boot_overhaul branch from 0556c8e to 0e7c3bf Aug 21, 2018

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Aug 21, 2018

I made the following changes:

  • Removed detailed boot procedure from mbed_boot.c since this is no longer relevent
  • Added doxygen group to mbed_boot.h
  • Added error if RTOS initialization fails
  • Moved ISR_STACK_SIZE into mbed_boot.h
  • Remove define MBED_CONF_RTOS_PRESENT
@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Aug 22, 2018

I really like your goal and work you've done here.
I've often found it incredibly hard to trace through the startup procedure, especially on new boards/targets when there's problems getting to main for any variety of reasons.

This is probably outside your desired scope, but would it take much effort to add an mbed config system entry to disable rtos aka: #7800

I regularly need to run mbed without the rtos on smaller targets that do not have enough ram/flash to support the os.

Currently the only way I know of to do this is adding the rtos and events libraries to .mbedignore

mbed-os/rtos/*
mbed-os/events/*

Unfortunately, I just tested this with this PR and it no longer works.

Compile [  0.3%]: main.cpp
[Fatal Error] Mutex.h@26,23: cmsis_os2.h: No such file or directory
[ERROR] In file included from ./mbed-os/features/netsocket/InternetSocket.h:25:0,
                 from ./mbed-os/features/netsocket/UDPSocket.h:23,
                 from ./mbed-os/features/netsocket/nsapi.h:40,
                 from ./mbed-os/mbed.h:26,
                 from .\main.cpp:1:
./mbed-os/rtos/Mutex.h:26:23: fatal error: cmsis_os2.h: No such file or directory
 #include "cmsis_os2.h"
                       ^
compilation terminated.

It would be great if you could make a better supported method of disabling the rtos!

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Aug 23, 2018

In case it wasn't clear in my last message,
This PR breaks the ability to run mbed with rtos disabled.

This may not be a particularly supported use case, but it is a critical need for many applications.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 23, 2018

@andrewleech Thanks for the feedback. This is supported use case (this approach could be found in the issues/questions - its being used).

From your error log, what does main.cpp contain? The error comes from there. How to reproduce this failure? Looking at the changes here, I do not see where this would break it.

@0xc0170

Looks fine - nice cleanup!

From the changes, this only touches rtos, not certain how this affects non rtos build as reported above. Please review it

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Aug 23, 2018

@0xc0170 It was a new project, fresh clone of mbed os with this branch checked out (0e7c3bf) and the basic hello world main.cpp

#include "mbed.h"
 
DigitalOut myled(LED1);
 
int main() {
    while(1) {
        myled = 1;
        wait(0.2);
        myled = 0;
        wait(0.2);
    }
}

I tried compiling (I think for NRF52_DK) with .mbedignore as above and got that error. Stuck a '#' in front of both lines and it compiled fine.

Just tried it again, mbed compile --clean -m NRF52_DK with and without the lines commented, same result.

@bridadan

This comment has been minimized.

Contributor

bridadan commented Aug 23, 2018

@0xc0170 The problem is probably the inclusion of rtos related headers in the toolchain boot source files. These rtos headers won't be present if the rtos directory is being ignored.

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Aug 23, 2018

@andrewleech

I tried non-rtos build with blinky and shared .mbedignore and noted issue exists on the master branch as well. Our network interfaces are dependent on rtos, agree we have not done great job in separating those and adding appropriate errors.

You can add a separate issue to mbed-os to take care of this in future. For now non-rtos blinky works with below:

mbed-os/rtos/*
mbed-os/events/*
mbed-os/features/*
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 24, 2018

Thanks @deepikabhavnani . Please review and leave feedback here. We can start CI soon

@andrewleech Please create an issue, we shall fix it !

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Aug 24, 2018

I'm so sorry I forgot to check master build as well, @c1728p9 sorry for the distraction. I love what you've done here.

@c1728p9 c1728p9 force-pushed the c1728p9:boot_overhaul branch from 625cab2 to 325565b Aug 29, 2018

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Aug 29, 2018

Fixed ARM failures

@cmonr cmonr added needs: CI and removed needs: work labels Aug 29, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 29, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 29, 2018

There's one device that hard faults, please verify

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Aug 29, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

@c1728p9 Fyi ^^^

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

/morph build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

Crud. Only meant to retest... Oh well.

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 29, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

GnuArmEclipse had an error initializing storage... Huh.
/morph export-build

@cmonr cmonr added needs: CI and removed needs: work labels Aug 29, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

Reshuffling the Integration Test queue. Will restart shortly.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

Fatal error: A1023E: File "/tmp/filetQiJyd" could not be opened: No such file or directory

This is less than useful...

/morph export-build

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 30, 2018

/morph export-build
/morph test

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 30, 2018

/morph test

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 88eb8aa into ARMmbed:master Aug 30, 2018

14 checks passed

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
jenkins-ci/cloud_client_smoke_test Test job was successful
Details
travis-ci/astyle Passed, 560 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9830 cycles (+25 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the needs: CI label Aug 30, 2018

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