Moving around a few items within \rtos #13264

ashok-rao · 2020-07-09T15:06:18Z

Summary of changes

Refactoring \rtos as per directory structure proposal.
Mbed OS will soon be changing directory structure to the below:

[component name]
├── mbed_lib.json 
├── CMakeList.txt
├── README.md
├── include 
│   └── [component name]
│       └── internal              
├── source
└── tests
    ├── <framework>  
    │   └── <test suite one> 
    ├── UNITTESTS               
    │   └── <unit test suite one>   
    └── TESTS       
        └── <greentea test suite one>

This PR implements the above new directory structure for the current \rtos directory.

Impact of changes

Migration actions required

Documentation

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Successfully compiled all MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-* tests for NUCLEO_F429ZI using ARMC6 and GCC.

Reviewers

@0xc0170 @LDong-Arm @rajkan01

ashok-rao · 2020-07-09T15:09:52Z

Build successes:
  * NUCLEO_F429ZI::ARMC6::MBED-BUILD
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-BASIC
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-CONDITION_VARIABLE
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-EVENT_FLAGS
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-HEAP_AND_STACK
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-KERNEL_TICK_COUNT
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-MAIL
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-MALLOC
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-MEMORYPOOL
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-MUTEX
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-QUEUE
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-SEMAPHORE
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-SIGNALS
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-SYSTIMER
  * NUCLEO_F429ZI::ARMC6::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-THREADS


Build successes:
  * NUCLEO_F429ZI::GCC_ARM::MBED-BUILD
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-BASIC
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-CONDITION_VARIABLE
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-EVENT_FLAGS
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-HEAP_AND_STACK
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-KERNEL_TICK_COUNT
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-MAIL
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-MALLOC
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-MEMORYPOOL
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-MUTEX
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-QUEUE
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-SEMAPHORE
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-SIGNALS
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-SYSTIMER
  * NUCLEO_F429ZI::GCC_ARM::MBED-OS-RTOS-TESTS-TESTS-MBEDMICRO-RTOS-MBED-THREADS

LDong-Arm

Thanks @ashok-rao, it looks totally good to me.

One thing completely unrelated to this PR: Currently rtos_idle.h is in source/ but its rtos_attach_idle_hook() is a semi-public API to override the default idle task. It's used by a couple of targets (if we grep targets for it). In my opinion it should be either a public header or in internal, depending on whether we allow applications to use it.

Update: Okay, it's the C version (and underlying implementation) of Kernel::attach_idle_hook() which is public and C++. But target drivers are written in C mostly.

Anyway this shouldn't block the PR in my opinion.

ciarmcom · 2020-07-09T17:00:35Z

@ashok-rao, thank you for your changes.
@0xc0170 @rajkan01 @LDong-Arm @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

kjbracey · 2020-07-10T07:09:47Z

Currently rtos_idle.h is in source/ but its rtos_attach_idle_hook() is a semi-public API to override the default idle task.

rtos_attach_idle_hook is marked as non-public by the Doxygen. Kernel::attach_idle_hook remains public, but it's a bit specialised since tickless was placed into the default hook. Replacing the hook loses a load of system bits from tickless, and there's no public way to put them back.

In my opinion it should be either a public header or in internal, depending on whether we allow applications to use it.

The concept was that headers in source could not be on the path at all for applications, and wouldn't need to be provided in a binary library drop. Unless you're changing the build system to remove that distinction, it's in the right place for a totally internal header. Only headers that need to be on the path for applications and provided in binary builds would go into include/internal.

whether we allow applications to use it.

target drivers are written in C mostly

Confusion over application versus target hook there. It's documented as an application hook, which means targets should not be using it.

LDong-Arm · 2020-07-10T08:56:13Z

@kjbracey-arm Totally agreed.

Confusion over application versus target hook there. It's documented as an application hook, which means targets should not be using it.

Idle hooks are used by (or, present in the code bases of) a few targets. I should've raised an issue earlier, and now it's time to do it.

In this case this PR looks totally good to me.

LDong-Arm · 2020-07-10T09:51:36Z

@kjbracey-arm
#13265 created

evedon

In mbed_rtos_types.h, osThreadFlagsSet is declared as a public API. This is used in the bare metal profile to set flags on the main thread (flags can be set from IRQ). Perhaps this function should be declared in a different header, for example ThisThread.h. And you need to udate the doxygen comment in mbed_rtos_types.h.
Alternatively, I would not move the three headers to rtos/internal in this PR and do this separately.

…aders

Pull request has been modified.

evedon · 2020-07-15T17:47:50Z

~~Could you also simplify the path for greentea tests as we don't need the extra mbedmicro-rtos-mbed. So we can have rtos/tests/TESTS/mutex/main.cpp directly.~~

It seems that our build tools don't support this and require a parent directory under TESTS so we don't do this now.

0xc0170 · 2020-07-16T11:55:31Z

@ashok-rao Before we run CI, can you extend the first commit message - add a reason why are we moving ? Copy-paste from PR description would be good.

adbridge · 2020-07-17T09:51:12Z

CI started

mbed-ci · 2020-07-17T10:43:31Z

Test run: FAILED

Summary: 1 of 3 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

Pull request has been modified.

0xc0170 · 2020-07-20T08:07:43Z

CI restarted

mbed-ci · 2020-07-20T09:08:18Z

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 2
Build artifacts

Moving around a few items within \rtos

3a1b074

LDong-Arm approved these changes Jul 9, 2020

View reviewed changes

ciarmcom requested review from 0xc0170, LDong-Arm, rajkan01 and a team July 9, 2020 17:00

ciarmcom added the needs: review label Jul 9, 2020

LDong-Arm approved these changes Jul 10, 2020

View reviewed changes

evedon previously requested changes Jul 14, 2020

View reviewed changes

mergify bot added needs: work and removed needs: review labels Jul 14, 2020

Incorporating reveiw comments - Moving mbed_rtos_types.h to public he…

04fac35

…aders

ashok-rao requested a review from evedon July 15, 2020 12:55

evedon previously approved these changes Jul 16, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Jul 16, 2020

adbridge added release-type: patch Indentifies a PR as containing just a patch and removed needs: CI labels Jul 17, 2020

Fix UNITTESTS issue

f58c563

0xc0170 added the needs: CI label Jul 20, 2020

0xc0170 approved these changes Jul 20, 2020

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Jul 20, 2020

0xc0170 merged commit 2b60c87 into ARMmbed:master Jul 20, 2020

mergify bot removed the ready for merge label Jul 20, 2020

toyowata mentioned this pull request Jul 28, 2020

mbedmicro-rtos-mbed-basic test failed on master #13359

Closed

LDong-Arm mentioned this pull request Jul 28, 2020

RTOS idle hooks overridden by some targets #13265

Closed

gpsimenos mentioned this pull request Jul 29, 2020

Drivers directory restructuring #13356

Merged

mbedmain added release-version: 6.2.1 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Aug 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Moving around a few items within \rtos #13264

Moving around a few items within \rtos #13264

Uh oh!

ashok-rao commented Jul 9, 2020 •

edited

Loading

Uh oh!

ashok-rao commented Jul 9, 2020

Uh oh!

LDong-Arm left a comment •

edited

Loading

Uh oh!

ciarmcom commented Jul 9, 2020

Uh oh!

kjbracey commented Jul 10, 2020

Uh oh!

LDong-Arm commented Jul 10, 2020 •

edited

Loading

Uh oh!

LDong-Arm commented Jul 10, 2020

Uh oh!

evedon left a comment

Uh oh!

evedon commented Jul 15, 2020 •

edited

Loading

Uh oh!

0xc0170 commented Jul 16, 2020 •

edited

Loading

Uh oh!

adbridge commented Jul 17, 2020

Uh oh!

mbed-ci commented Jul 17, 2020

Uh oh!

0xc0170 commented Jul 20, 2020

Uh oh!

mbed-ci commented Jul 20, 2020

Uh oh!

Uh oh!

Moving around a few items within \rtos #13264

Moving around a few items within \rtos #13264

Uh oh!

Conversation

ashok-rao commented Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ashok-rao commented Jul 9, 2020

Uh oh!

LDong-Arm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciarmcom commented Jul 9, 2020

Uh oh!

kjbracey commented Jul 10, 2020

Uh oh!

LDong-Arm commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LDong-Arm commented Jul 10, 2020

Uh oh!

evedon left a comment

Choose a reason for hiding this comment

Uh oh!

evedon commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adbridge commented Jul 17, 2020

Uh oh!

mbed-ci commented Jul 17, 2020

Test run: FAILED

Uh oh!

0xc0170 commented Jul 20, 2020

Uh oh!

mbed-ci commented Jul 20, 2020

Test run: SUCCESS

Uh oh!

Uh oh!

ashok-rao commented Jul 9, 2020 •

edited

Loading

LDong-Arm left a comment •

edited

Loading

LDong-Arm commented Jul 10, 2020 •

edited

Loading

evedon commented Jul 15, 2020 •

edited

Loading

0xc0170 commented Jul 16, 2020 •

edited

Loading