Skip to content

Get rid of FEATURE_COMMON_PAL and FEATURE_NANOSTACK #6577

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

Merged
merged 5 commits into from
May 7, 2018

Conversation

SeppoTakalo
Copy link
Contributor

@SeppoTakalo SeppoTakalo commented Apr 9, 2018

Description

Get rid of FEATURE_COMMON_PAL and FEATURE_NANOSTACK
Nanostack related files moved under 'feature/nanostack'
Common libraries moved to 'features/frameworks'

Allow FEATURE_COMMON_PAL and FEATURE_NANOSTACK still to be defined in the build so
that we don't break any builds.

This change is required for future when we start enabling network related tests that depend on the common libraries that are otherwise moved to FEATURE_COMMON_PAL or FEATURE_NANOSTACK which in first place was temporary solution. None of those libraries under these two folders are target specific.

Pull request type

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

Effects on RAM/ROM usage

RAM: none
ROM: depend on the linkker.

Tested mbed-os-example-blinky with GCC 6.3.1 20170620 and Flash hit was less than 1kB.

master branch:

Link: mbed-os-example-blinky
Elf2Bin: mbed-os-example-blinky
+------------------+-------+-------+------+
| Module           | .text | .data | .bss |
+------------------+-------+-------+------+
| [fill]           |   126 |     4 | 2573 |
| [lib]/c.a        | 22945 |  2472 |   89 |
| [lib]/gcc.a      |  3144 |     0 |    0 |
| [lib]/misc       |   248 |     8 |   28 |
| [lib]/nosys.a    |    32 |     0 |    0 |
| main.o           |    56 |     0 |    4 |
| mbed-os/drivers  |    46 |     0 |    0 |
| mbed-os/features |    42 |     0 |  184 |
| mbed-os/hal      |  1611 |     4 |   68 |
| mbed-os/platform |  1913 |   260 |   21 |
| mbed-os/rtos     | 10095 |   168 | 6073 |
| mbed-os/targets  |  8127 |    12 |  384 |
| Subtotals        | 48385 |  2928 | 9424 |
+------------------+-------+-------+------+
Total Static RAM memory (data + bss): 12352 bytes
Total Flash memory (text + data): 51313 bytes

After this change

+------------------+-------+-------+------+
| Module           | .text | .data | .bss |
+------------------+-------+-------+------+
| [fill]           |   135 |     4 | 2477 |
| [lib]/c.a        | 22945 |  2472 |   89 |
| [lib]/gcc.a      |  3144 |     0 |    0 |
| [lib]/misc       |   248 |     8 |   28 |
| [lib]/nosys.a    |    32 |     0 |    0 |
| main.o           |    56 |     0 |    4 |
| mbed-os/drivers  |   657 |     0 |    0 |
| mbed-os/features |   142 |     0 |  280 |
| mbed-os/hal      |  1783 |     4 |   68 |
| mbed-os/platform |  1925 |   260 |   21 |
| mbed-os/rtos     | 10095 |   168 | 6073 |
| mbed-os/targets  |  8127 |    12 |  384 |
| Subtotals        | 49289 |  2928 | 9424 |
+------------------+-------+-------+------+
Total Static RAM memory (data + bss): 12352 bytes
Total Flash memory (text + data): 52217 bytes

Future

After this change there will be

  • FEATURE_BLE
  • FEATURE_LWIP
  • FEATURE_UVISOR

And after EMAC work, the FEATURE_LWIP can go as well as it becomes driver independent, and all drivers became stack independent.

@SeppoTakalo
Copy link
Contributor Author

@OPpuolitaival @kjbracey-arm Please review.

@SeppoTakalo SeppoTakalo changed the title Get rid of FEATURE_COMMON_PAL Get rid of FEATURE_COMMON_PAL and FEATURE_NANOSTACK Apr 9, 2018
@SeppoTakalo
Copy link
Contributor Author

CC: @mikaleppanen

Copy link
Contributor

@OPpuolitaival OPpuolitaival 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 and we really need this but I am not right person to look at code level

@SeppoTakalo
Copy link
Contributor Author

Doxygen failures are because I have renamed some folder without touching its configuration files. I'll fix those once review of this work has been approved.

@0xc0170 0xc0170 requested a review from mikaleppanen April 9, 2018 14:49
kjbracey
kjbracey previously approved these changes Apr 10, 2018
Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

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

Approve - only caveat is that in the past I've seen git subtree pull refuse to work when the destination has moved, so that might cause problems updating some of the components in future.

If that's still the case, I believe @mikaleppanen should still have a copy of a tiny tweak to the git-subtree.sh script to avoid that.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 10, 2018

This change is required for future when we start enabling network related tests that depend on the common libraries that are otherwise moved to FEATURE_COMMON_PAL or FEATURE_NANOSTACK which in first place was temporary solution. None of those libraries under these two folders are target specific.

👍

@SeppoTakalo
Copy link
Contributor Author

Rebased on top of master.

Can somebody explain why PR verification job does this:

rm -r rtos features/cellular features/netsocket features/frameworks BUILD

Why does it MODIFY the Mbed OS repository?

I just moved the stuff there, and the build is now broken.
Should we fix the test job not to modify the repository, or should I move that stuff to another folder?

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 16, 2018

@geky Can you review the events Travis failure? ^^ related to the rm command

@geky
Copy link
Contributor

geky commented Apr 16, 2018

Ah, that test trys to compile without the rtos. Currently the netsocket API depends on the rtos so it must also be removed to compile. Does nanostack depend on the netsocket API? If so it needs to be added to the "rm" command to test for the rtos-less build.

Currently rtos-less mbed is used by an annoying amount of users:
https://os.mbed.com/blog/entry/Reducing-memory-usage-by-tuning-RTOS-con/

@SeppoTakalo
Copy link
Contributor Author

Is this method documented somewhere?

Hacking the build system should not be one of the official supported method.

@SeppoTakalo
Copy link
Contributor Author

And yes, Nanostack provides implementation so it depends on the Socket API.

@sg-
Copy link
Contributor

sg- commented Apr 16, 2018

Tested mbed-os-example-blinky with GCC 6.3.1 20170620 and Flash hit was less than 1kB.

Do we know where that extra 1k is from?

@SeppoTakalo
Copy link
Contributor Author

When running diff pr-6577.txt master.txt from the final build output:

Left (RED) diff is this PR. Right(green) is the master

+-------------------------------------------------------------------------------------------------------------+-------+-------+------+
| Module                                                                                                      | .text | .data | .bss |
+-------------------------------------------------------------------------------------------------------------+-------+-------+------+

4c4
< | [fill]                                                                                                      |    85 |     3 |   21 |
---
> | [fill]                                                                                                      |   116 |     3 |   13 |
66d65
< | mbed-os/drivers/InterruptIn.o                                                                               |    16 |     0 |    0 |
69,72d67
< | mbed-os/drivers/Ticker.o                                                                                    |   200 |     0 |    0 |
< | mbed-os/drivers/Timeout.o                                                                                   |   165 |     0 |    0 |
< | mbed-os/drivers/Timer.o                                                                                     |   106 |     0 |    0 |
< | mbed-os/drivers/TimerEvent.o                                                                                |   124 |     0 |    0 |
74d68
< | mbed-os/features/nanostack/nanostack-hal-mbed-cmsis-rtos/arm_hal_timer.o                                    |   100 |     4 |   96 |
79c73
< | mbed-os/hal/mbed_ticker_api.o                                                                               |  1034 |     0 |    0 |
---
> | mbed-os/hal/mbed_ticker_api.o                                                                               |   862 |     0 |    0 |
87c81
< | mbed-os/platform/mbed_retarget.o                                                                            |  1258 |   260 |   16 |
---
> | mbed-os/platform/mbed_retarget.o                                                                            |  1246 |   260 |   16 |
129c123
< | Subtotals                                                                                                   | 49802 |  2940 | 6896 |
---
> | Subtotals                                                                                                   | 48938 |  2936 | 6792 |
131,133c125,126
< Total Static RAM memory (data + bss): 9836 bytes
< Total Flash memory (text + data): 52742 bytes
< 
---
> Total Static RAM memory (data + bss): 9728 bytes
> Total Flash memory (text + data): 51874 bytes

@kjbracey
Copy link
Contributor

Okay, looks like some SingletonPtr required in arm_hal_timer.cpp

@SeppoTakalo
Copy link
Contributor Author

Cherry picked Kevin's fix for using SingletonPtr when referencing those C++ objects.
Now linker is able to drop references that are not used, and therefore this PR does not anymore cause any extra RAM or Flash usage.

Current Mbed OS master

+------------------+-------+-------+------+
| Module           | .text | .data | .bss |
+------------------+-------+-------+------+
| [fill]           |   118 |     3 |   13 |
| [lib]/c.a        | 24981 |  2472 |   89 |
| [lib]/gcc.a      |  3144 |     0 |    0 |
| [lib]/misc       |   252 |    16 |   28 |
| main.o           |    68 |     4 |   28 |
| mbed-os/drivers  |   182 |     4 |  100 |
| mbed-os/hal      |  1597 |     4 |   68 |
| mbed-os/platform |  1901 |   260 |   21 |
| mbed-os/rtos     | 10099 |   168 | 6073 |
| mbed-os/targets  |  6596 |     5 |  324 |
| Subtotals        | 48938 |  2936 | 6744 |
+------------------+-------+-------+------+
Total Static RAM memory (data + bss): 9680 bytes
Total Flash memory (text + data): 51874 bytes

This pull request

+------------------+-------+-------+------+
| Module           | .text | .data | .bss |
+------------------+-------+-------+------+
| [fill]           |   118 |     3 |   13 |
| [lib]/c.a        | 24981 |  2472 |   89 |
| [lib]/gcc.a      |  3144 |     0 |    0 |
| [lib]/misc       |   252 |    16 |   28 |
| main.o           |    68 |     4 |   28 |
| mbed-os/drivers  |   182 |     4 |  100 |
| mbed-os/hal      |  1597 |     4 |   68 |
| mbed-os/platform |  1901 |   260 |   21 |
| mbed-os/rtos     | 10099 |   168 | 6073 |
| mbed-os/targets  |  6596 |     5 |  324 |
| Subtotals        | 48938 |  2936 | 6744 |
+------------------+-------+-------+------+
Total Static RAM memory (data + bss): 9680 bytes
Total Flash memory (text + data): 51874 bytes

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 18, 2018

Great news, thanks @kjbracey-arm

@geky
Copy link
Contributor

geky commented Apr 18, 2018

Hacking the build system should not be one of the official supported method.

I should point out this is also the only test with events + mbed 2. I'm open to an alternative approach but I'm not aware of any.

@SeppoTakalo
Copy link
Contributor Author

Rebased on top of master.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 27, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Apr 27, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Apr 27, 2018

@mbed-ci
Copy link

mbed-ci commented Apr 27, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented May 2, 2018

Master contains the fix for the flash test failed the previous week, restarting

/morph test

@0xc0170
Copy link
Contributor

0xc0170 commented May 3, 2018

Restarting tests (been a week), the latest test did not get triggered due to CI outage.

/morph build

@mbed-ci
Copy link

mbed-ci commented May 3, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented May 3, 2018

@mbed-ci
Copy link

mbed-ci commented May 4, 2018

adbridge pushed a commit that referenced this pull request May 4, 2018
* Updates driver library to v2.3.1 (2018q1) for bugfixes and convenience functions
* Provides library in correct format (2-byte wchar_t flag) for compiling with ARMCC (#6695 uncovered by #6577)
* Reverts to using a statically-allocated packet buffer since malloc is not thread-safe (and the asserts have been turned on)
@0xc0170
Copy link
Contributor

0xc0170 commented May 4, 2018

This should be now ready for integration 🎉

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.

10 participants