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

mbed sdk boot: copy vectors addition #4503

Merged
merged 2 commits into from
Jun 9, 2017

Conversation

0xc0170
Copy link
Contributor

@0xc0170 0xc0170 commented Jun 8, 2017

This was a breakage from the latest cmsis5 boot process unification. However, this PR does not completely fixes it. It fixes for non cortex-m0. We will need to address removal of this functionality for cortex-m0.

Without this patch, ticker test would end up in the hard fault..

With this patch, interrupts are properly running.

Note: if anyone test only this patch with debug profile and with ARM toolchain, there is another problem at least with ticker test. This test invokes fflush from ISR and we now define MBED_TRAP_ERRORS_ENABLED, thus this ends calling exit() via error() function :/ Not saying this is correct - https://github.com/ARMmbed/mbed-os/blob/master/features/unsupported/tests/mbed/ticker/main.cpp#L7 :-) Just that I did not realized fflush is related to this changeset (adding trap errors!).

cc @LMESTM @bcostm This should help to pass some tests for mbed 2. Please read the note, there might be more problems like this in our tests !

I added here also mbed_sdk_boot object file that should be there (I sent PR to cmsis5 to have it fixed, seems like one of the rebased we did removed this).

copy nvic function is a duplication with mbed 5 - no it is not, uvisor is involved first, then we might do some other things there, therefore I created own in the mbed 2 boot process. I expect this won't change in mbed 2 , but might change in mbed 5.

@c1728p9 Please review, and lets discuss how to fix cortex-m0 targets

@YarivCol
Copy link
Contributor

YarivCol commented Jun 8, 2017

hi,
I think it would be better to put bare metal boot code to all toolchains in the same file, so the file cmain.s will be removed.
I think it will make the boot code more maintainable.

also there some code that could be shared between rtos boot code and bare metal code like nvic_cpy and the declaration of sdk_init, mbed_main.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Jun 8, 2017

I think it would be better to put bare metal boot code to all toolchains in the same file, so the file cmain.s will be removed.
I think it will make the boot code more maintainable.

How for IAR ? I would like to eliminate cmain.s . Because I could not find any hooks for IAR besides the one RTX uses (low level code is not good for this one, I excluded it). Thus cmain.s is in the code base. We could use weak linkage for software hook?

also there some code that could be shared between rtos boot code and bare metal code like nvic_cpy and the declaration of sdk_init, mbed_main.

Thats correct I had a version unified but first, did not look that clean (look how iar startup does things , it is split to two stages), and second, would need to push update to rtx to change this. If you have ideas, please provide details. Third, the startup for mbed 2 and mbed 5 differs and will even more, so for mbed 2 it was kept as it was, unified for mbed OS 5 (this one can be changed as there are other modules and features involved).

I realized that copy nvic in here would work only for bare metal builds, we need to fix that.

0xc0170 added 2 commits June 8, 2017 17:19
Latest cmsis files provide virtual nvic implementation, therefore all nvic
set/get vectors were removed. As the result, we did not reallocate vectors
for mbed SDK. This should fix it for most of the platforms (cortex m0 and
cortex a9 need to provide own if they need it).
This was probably deleted during the rebase, as mbed sdk boot needs to be
own object file due to using weak functions.
@c1728p9 c1728p9 force-pushed the fix_issue_sdk_vectors branch from b2d1cc4 to 278634a Compare June 8, 2017 22:21
Copy link
Contributor

@LMESTM LMESTM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good at first glance - will need to test.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Jun 9, 2017

@LMESTM Let me know the test results. I'll continue as well as Russ fixed the IAR . If you combine this PR, with other 2 that are there ( trap enabled and cortex m0 realloc, mbed 2 tests should be green)

@0xc0170
Copy link
Contributor Author

0xc0170 commented Jun 9, 2017

I merged locally all 3 related PR to fix mbed 2 builds, run test on one cortex m0 device and m4 (random projects for random toolchains plus run some tests , sharing results here).

Results

+--------+---------------------+-----------+-------------+---------------------------------------+--------------------+---------------+-------+
| Result | Target              | Toolchain | Test ID     | Test Description                      | Elapsed Time (sec) | Timeout (sec) | Loops |
+--------+---------------------+-----------+-------------+---------------------------------------+--------------------+---------------+-------+
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | DTCT_1      | Simple detect test                    |        0.68        |
       10      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | EXAMPLE_1   | /dev/null                             |        3.62        |
       20      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_10     | Hello World                           |        0.56        |
       5       |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_11     | Ticker Int                            |       11.49        |
       15      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_12     | C++                                   |        1.55        |
       10      |  1/1  |
| FAIL   | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_16     | RTC                                   |        5.56        |
       20      |  0/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_2      | stdio                                 |        0.95        |
       20      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_23     | Ticker Int us                         |       11.48        |
       15      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_24     | Timeout Int us                        |       11.83        |
       15      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_25     | Time us                               |       11.51        |
       15      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_26     | Integer constant division             |        1.57        |
       20      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_34     | Ticker Two callbacks                  |       11.52        |
       15      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_37     | Serial NC RX                          |        7.13        |
       20      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_38     | Serial NC TX                          |        6.1         |
       20      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_A1     | Basic                                 |        1.55        |
       20      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_A21    | Call function before main (mbed_main) |        1.63        |
       20      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_A9     | Serial Echo at 115200                 |        1.56        |
       20      |  1/1  |
| OK     | NUCLEO_F030R8[F07B] | GCC_ARM   | MBED_BUSOUT | BusOut                                |        2.47        |
       30      |  1/1  |
+--------+---------------------+-----------+-------------+---------------------------------------+--------------------+---------------+-------+
Result: 1 FAIL / 17 OK


Test summary:
+--------+------------+-----------+-------------+---------------------------------------+--------------------+---------------+-------+
| Result | Target     | Toolchain | Test ID     | Test Description                      | Elapsed Time (sec) | Timeout (sec) | Loops |
+--------+------------+-----------+-------------+---------------------------------------+--------------------+---------------+-------+
| OK     | K64F[e3f5] | GCC_ARM   | DTCT_1      | Simple detect test                    |        0.49        |       10
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | EXAMPLE_1   | /dev/null                             |        3.45        |       20
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_10     | Hello World                           |        0.36        |       5
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_11     | Ticker Int                            |       11.39        |       15
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_12     | C++                                   |        1.38        |       10
      |  1/1  |
| FAIL   | K64F[e3f5] | GCC_ARM   | MBED_16     | RTC                                   |        4.58        |       20
      |  0/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_2      | stdio                                 |        0.73        |       20
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_23     | Ticker Int us                         |       11.36        |       15
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_24     | Timeout Int us                        |       11.55        |       15
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_25     | Time us                               |       11.33        |       15
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_26     | Integer constant division             |        1.4         |       20
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_34     | Ticker Two callbacks                  |       11.38        |       15
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_37     | Serial NC RX                          |        6.91        |       20
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_38     | Serial NC TX                          |        5.88        |       20
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_A1     | Basic                                 |        1.36        |       20
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_A21    | Call function before main (mbed_main) |        1.43        |       20
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_A9     | Serial Echo at 115200                 |        1.28        |       20
      |  1/1  |
| OK     | K64F[e3f5] | GCC_ARM   | MBED_BUSOUT | BusOut                                |        2.28        |       30
      |  1/1  |
+--------+------------+-----------+-------------+---------------------------------------+--------------------+---------------+-------+
Result: 1 FAIL / 17 OK

vtor realloc should be restored and tests functional as they were (mbed 2).

@0xc0170
Copy link
Contributor Author

0xc0170 commented Jun 9, 2017

/morph test

@LMESTM
Copy link
Contributor

LMESTM commented Jun 9, 2017

Tested OK on L476RG

Copy link
Contributor

@LMESTM LMESTM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review ok (and tested ok)

@0xc0170
Copy link
Contributor Author

0xc0170 commented Jun 9, 2017

Thanks @LMESTM !

@mbed-bot
Copy link

mbed-bot commented Jun 9, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 515

All builds and test passed!

@LMESTM
Copy link
Contributor

LMESTM commented Jun 12, 2017

@0xc0170 -today I tested on master after this PR was merged. MBED2 tests boot ok on NUCLEO_L476RG as I reported last week, but I still fail to boot on NUCLEO_F334R8. If I roll back before CMSIS5 update, this is ok again ...

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

Successfully merging this pull request may close these issues.

6 participants