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

Fix storage rtos types - remove including internal header file #7364

Merged
merged 10 commits into from Jul 5, 2018

Conversation

Projects
None yet
7 participants
@0xc0170
Member

0xc0170 commented Jun 28, 2018

Description

Remove internal types from our storage rtos header file. And do some refactor (some files still use internal types). Plus some other fixes as I removed one internal rtx from our include, I discovered 2 files not including needed header files!

As result, this also fix some warnings about redefinition STRINGIFY for some platforms (nordic at least).

@JonatanAntoni who I discussed the issue with

Pull request type

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

0xc0170 added some commits Jun 28, 2018

rtos: use RTX identifier
os_thread_t and family are internal and should not be used (thus we included
internal header file). Instead, use those that are exposed via rtx_os.h file.

@0xc0170 0xc0170 requested review from bulislaw and ARMmbed/mbed-os-core Jun 28, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 28, 2018

Fixed few errors (I noticed those internal types are used across the code base). This PR should clean it all

@@ -40,17 +40,17 @@ extern "C" {
implementation specific, header file, therefore limiting scope of possible changes.
*/
#include "rtx_lib.h"
#include "rtx_os.h"

This comment has been minimized.

@pingdan32

pingdan32 Jun 28, 2018

Contributor

Looks good. I like it.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Jun 28, 2018

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_storage_rtos branch from 20b738e to 4d8baa7 Jun 29, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 29, 2018

One more fix incoming, another internal symbol not exposed (jenkins failure). This is a good exercise !

Update: more than one. More internal symbols in our codebase. Fixing it all in once! 🔥 🚒

@0xc0170 0xc0170 referenced this pull request Jun 29, 2018

Closed

Redefined macros #7301

@@ -430,6 +430,18 @@ void __rt_entry (void) {
*/
#if defined(RTX_NO_MULTITHREAD_CLIB)
// These 2 macros are taken from RTX Config (internal header)

This comment has been minimized.

@bulislaw

bulislaw Jun 29, 2018

Member

That should probably go into our config file?

This comment has been minimized.

@0xc0170

0xc0170 Jun 29, 2018

Member

I can move it there with default values set. Based on our config, there would be just one
#define OS_THREAD_LIBSPACE_NUM 4, will check

This comment has been minimized.

@0xc0170

0xc0170 Jun 29, 2018

Member

Latest rebase fixed it, moved to conf with default value as it was (4)

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_storage_rtos branch from 4d9d292 to fec82aa Jun 29, 2018

0xc0170 added some commits Jun 29, 2018

boot: fix libspace num macro
RTX Config header file is internal and not exposed. With latest fixes to use
only public header files from RTX, we need to add these 2 new macros for
RTX ARMC lib configuration.
tests: fix internal symbols from RTX
As we do not include rtx_lib header file anymore, these symbols are not available.
Use core util for if ISR is active. And for OS tick, pull in os_tick header file.
test: fix os tick linkage
os_tick header file is C only, mangling would cause problems here

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_storage_rtos branch from fec82aa to dd33247 Jun 29, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Jun 29, 2018

@0xc0170

Update: more than one. More internal symbols in our codebase. Fixing it all in once! 🔥 🚒

You are on a roll! 😄

@deepikabhavnani

👍

@cmonr cmonr added needs: CI and removed needs: review labels Jul 3, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 3, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 3, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 4, 2018

uvisor example failed - @alzix shall I update the example to fix the error or?

Error:

[DEBUG] Output: ./source/led3.cpp:65:5: error: 'os_thread_t' was not declared in this scope
[DEBUG] Output:      os_thread_t thread_def = {0};
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 4, 2018

Fix for uvisor example proposed ARMmbed/mbed-os-example-uvisor-thread#104

Update: PR merged

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 4, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 4, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added needs: review and removed needs: CI labels Jul 4, 2018

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-core Jul 4, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 4, 2018

Waiting for final reviews for this bugfix. I labeled is as 5.10 due as apps could use these internal types we exposed. By 5.10, the types should be updated to the one we are providing as storage types for rtos.

@cmonr

cmonr approved these changes Jul 5, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 5, 2018

+1 on targeting 5.10 due to the changes. LGTM.

@cmonr cmonr merged commit 93233c4 into ARMmbed:master Jul 5, 2018

14 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/astyle Passed, 913 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10342 cycles (+1404 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 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the needs: review label Jul 5, 2018

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