Update Mbed 5 boot sequence #7794

c1728p9 · 2018-08-14T22:31:48Z

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 · 2018-08-15T02:53:13Z

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

amq · 2018-08-15T10:50:22Z

Is this related? #7382

c1728p9 · 2018-08-15T14:26:57Z

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!

bridadan · 2018-08-15T15:39:03Z

rtos/TARGET_CORTEX/mbed_boot.h

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


English nit:

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

should probably just be "by".

bridadan · 2018-08-15T15:39:34Z

rtos/TARGET_CORTEX/mbed_boot.h

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


English nit:

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

should probably just be "by".

c1728p9 · 2018-08-15T20:05:47Z

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

deepikabhavnani · 2018-08-20T17:32:44Z

platform/SingletonPtr.h

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


Should we throw an MBED_ERROR here?

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

It does seem to be the case for STM boards...

deepikabhavnani · 2018-08-20T17:36:43Z

rtos/TARGET_CORTEX/TOOLCHAIN_ARM/mbed_boot_arm.c

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


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

deepikabhavnani

Thanks... this was much needed refactoring 👍

bulislaw · 2018-08-21T09:05:39Z

rtos/TARGET_CORTEX/TOOLCHAIN_GCC_ARM/mbed_boot_gcc_arm.c

+
+void mbed_toolchain_init()
+{
+#if defined(MBED_CONF_RTOS_PRESENT)


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

bulislaw · 2018-08-21T09:11:31Z

rtos/TARGET_CORTEX/mbed_boot.h

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


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?

bulislaw · 2018-08-21T09:12:52Z

rtos/TARGET_CORTEX/mbed_boot.h

+ * - 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);


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?

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

bulislaw · 2018-08-21T09:14:18Z

rtos/TARGET_CORTEX/mbed_rtos_rtx.c

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


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

c1728p9 · 2018-08-21T22:24:33Z

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 · 2018-08-22T04:16:30Z

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 · 2018-08-23T01:11:58Z

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 · 2018-08-23T11:36:43Z

@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 · 2018-08-23T13:13:40Z

@0xc0170 It was a new project, fresh clone of mbed os with this branch checked out (0e7c3bfa) 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 · 2018-08-23T13:36:16Z

@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 · 2018-08-23T15:47:42Z

@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 · 2018-08-24T13:12:49Z

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

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

andrewleech · 2018-08-24T21:22:22Z

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 · 2018-08-29T00:29:00Z

Fixed ARM failures

cmonr · 2018-08-29T03:47:44Z

/morph build

mbed-ci · 2018-08-29T04:52:29Z

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 · 2018-08-29T06:40:14Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-08-29T11:04:43Z

Test : FAILURE

Build number : 2693
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7794/2693

0xc0170 · 2018-08-29T11:16:48Z

There's one device that hard faults, please verify

cmonr · 2018-08-29T15:10:51Z

@c1728p9 Fyi ^^^

cmonr · 2018-08-29T16:43:19Z

/morph build

cmonr · 2018-08-29T16:44:35Z

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

mbed-ci · 2018-08-29T18:46:40Z

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 · 2018-08-29T19:46:45Z

Exporter Build : FAILURE

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

cmonr · 2018-08-29T20:43:24Z

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

cmonr · 2018-08-29T21:00:01Z

Reshuffling the Integration Test queue. Will restart shortly.

mbed-ci · 2018-08-29T22:51:17Z

Exporter Build : FAILURE

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

cmonr · 2018-08-29T23:10:07Z

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 · 2018-08-30T00:10:44Z

Exporter Build : FAILURE

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

0xc0170 · 2018-08-30T10:17:52Z

/morph export-build
/morph test

0xc0170 · 2018-08-30T11:35:23Z

/morph test

mbed-ci · 2018-08-30T11:47:24Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-08-30T15:21:46Z

Test : SUCCESS

Build number : 2712
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7794/2712

c1728p9 requested review from mprse and bulislaw August 14, 2018 22:32

c1728p9 mentioned this pull request Aug 15, 2018

Update boot sequence docs ARMmbed/mbed-os-5-docs#662

Merged

c1728p9 force-pushed the boot_overhaul branch from a6c81fe to 4d5bbf8 Compare August 15, 2018 03:18

cmonr added release-version: 5.10.0-rc1 needs: review labels Aug 15, 2018

bridadan reviewed Aug 15, 2018

View reviewed changes

c1728p9 force-pushed the boot_overhaul branch from 4d5bbf8 to 0556c8e Compare August 15, 2018 20:05

deepikabhavnani reviewed Aug 20, 2018

View reviewed changes

bulislaw reviewed Aug 21, 2018

View reviewed changes

c1728p9 force-pushed the boot_overhaul branch from 0556c8e to 0e7c3bf Compare August 21, 2018 22:23

bulislaw approved these changes Aug 22, 2018

View reviewed changes

0xc0170 approved these changes Aug 23, 2018

View reviewed changes

deepikabhavnani approved these changes Aug 24, 2018

View reviewed changes

andrewleech mentioned this pull request Aug 24, 2018

Cannot compile mbed with RTOS disabled #7889

Closed

c1728p9 force-pushed the boot_overhaul branch from 625cab2 to 325565b Compare August 29, 2018 00:28

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

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

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

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

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

pauluap mentioned this pull request Aug 11, 2019

What is the ultimate end effect of setting bare-metal in the configuration #11197

Closed

Update Mbed 5 boot sequence #7794

Update Mbed 5 boot sequence #7794

Uh oh!

Conversation

c1728p9 commented Aug 14, 2018

Description

Pull request type

Uh oh!

c1728p9 commented Aug 15, 2018

Uh oh!

amq commented Aug 15, 2018

Uh oh!

c1728p9 commented Aug 15, 2018

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1728p9 commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1728p9 commented Aug 21, 2018

Uh oh!

andrewleech commented Aug 22, 2018

Uh oh!

andrewleech commented Aug 23, 2018

Uh oh!

0xc0170 commented Aug 23, 2018

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

andrewleech commented Aug 23, 2018

Uh oh!

bridadan commented Aug 23, 2018

Uh oh!

deepikabhavnani commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewleech commented Aug 24, 2018

Uh oh!

c1728p9 commented Aug 29, 2018

Uh oh!

cmonr commented Aug 29, 2018

Uh oh!

mbed-ci commented Aug 29, 2018

deepikabhavnani commented Aug 23, 2018 •

edited

Loading

0xc0170 commented Aug 24, 2018 •

edited

Loading