Add cpp API for CMSIS OS 2 EventFlags #4517

YarivCol · 2017-06-09T16:09:23Z

Description

Added the ability to set, clear or wait for event flag, it will be useful for building drivers with block like api.
for example building block device that preforms async operations on top of the spi driver to allow the system to do other tasks, in that case event flag will be waited in program, erase, etc... and the event flag will be set in the callback of async operation.

AnotherButler · 2017-06-09T18:48:35Z

@YarivCol Thanks for the PR.

Also, we recommend our contributors follow Chris Beam’s seven rules of great commit messages to keep the commit history clear. To match this format, please change the subject line of your PR to the imperative mood. For example, you could change it to "Add cpp API for CMSIS OS 2 EventFlags".

Thanks for your contribution.

YarivCol · 2017-06-13T21:35:17Z

This PR is not done still need to fix compile errors and test it

sg-

Thanks for the addition. Please make sure to add tests https://github.com/ARMmbed/mbed-os/tree/master/TESTS/mbedmicro-rtos-mbed and to update the code style to follow https://docs.mbed.com/docs/mbed-os-handbook/en/latest/cont/code_style/

geky

It looks great 👍

I think it mostly just needs a few style changes.

geky · 2017-06-16T21:41:28Z

rtos/EventFlags.cpp

+{
+    memset(&_obj_mem, 0, sizeof(_obj_mem));
+    memset(&_attr, 0, sizeof(_attr));
+    _attr.name = name ? name : "aplication_unnamed_eventflags";


Can the name just be NULL by default?

I followed Mutex class implementation, when the user does not give name the Mutex is called aplication_unnamed_mutex

geky · 2017-06-16T21:42:06Z

rtos/EventFlags.cpp

+    MBED_ASSERT(_id);
+}
+
+uint32_t EventFlags::Set (uint32_t flags)


Should be lowercase snake_case EventFlags::set

geky · 2017-06-16T21:42:14Z

rtos/EventFlags.cpp

+    return osEventFlagsSet(_id, flags);
+}
+
+uint32_t EventFlags::Clear (uint32_t flags)


Should be lowercase snake_case EventFlags::clear, the other functions should be lowercase snake_case as well.

geky · 2017-06-16T21:48:06Z

rtos/EventFlags.h

+    /** Get the current Event Flags.
+      @return  current event flags.
+     */
+    uint32_t Get ());


extra paren?

geky · 2017-06-16T21:49:12Z

rtos/EventFlags.h

+      @param   flags  specifies the flags that shall be set.
+      @return  event flags after setting or error code if highest bit set.
+     */
+    uint32_t Set (uint32_t flags);


thought: We could provide a default to set and clear to set and clear all flags

uint32_t set(uint32_t flags=0xffffffff); ... eventflags.set(); // sets all flags

geky · 2017-06-16T21:52:09Z

rtos/EventFlags.h

+      @param   timeout  timeout value or 0 in case of no time-out. (default: osWaitForever)
+      @return  event flags before clearing or error code if highest bit set.
+     */
+    uint32_t Wait (uint32_t flags, uint32_t options, uint32_t timeout);


thought: The cmsis-rtos documentation had some defaults we could use here

uint32_t wait(uint32_t flags=0xffffffff, uint32_t options=osFlagsWaitAny , uint32_t timeout=osWaitForever); ... eventflags.wait(); // waits for any flags to get set

pan-

Could you declare the constructor and copy assignment operator as private. That will disable those two operations.

0xc0170

Can you write tests please? And share results (to be run at least on one platform), we will trigger then CI tests to confirm and run on multiple devices.

0xc0170 · 2017-06-19T12:04:51Z

rtos/EventFlags.cpp

+
+namespace rtos {
+
+EventFlags::EventFlags() {


`{`` on the new line

@0xc0170 I added test for event flags.

I did not fully understand your comment about the styling of the code, could you provide an example?

void function() { //body here }

0xc0170 · 2017-06-22T12:29:54Z

One more , who's omriarad ? Can they sign https://developer.mbed.org/contributor_agreement/ ?

YarivCol · 2017-06-22T12:42:28Z

My computer was used by this user in the past, i forgot to change the user in git to mine.
All the changes in this PR were made by me (YarivCol).

0xc0170 · 2017-06-22T12:54:09Z

My computer was used by this user in the past, i forgot to change the user in git to mine.
All the changes in this PR were made by me (YarivCol).

You should be able to rebase and change the user . That would be good, to avoid any confusion

0xc0170 · 2017-06-26T08:52:57Z

TESTS/mbedmicro-rtos-mbed/event_flags/main.cpp

@@ -0,0 +1,50 @@
+#include "mbed.h"


tests should contain also license header, please add it

Can we add the license header to this file please?

0xc0170 · 2017-06-26T08:53:22Z

TESTS/mbedmicro-rtos-mbed/event_flags/main.cpp

+    printf("Handling %d events...\r\n", EVENT_TO_EMIT);
+
+    while (true) {
+        Thread::wait(2 * EVENT_HANDLE_DELEY);


DELEY - DELAY typo

0xc0170 · 2017-06-26T08:54:10Z

rtos/EventFlags.cpp

+    return osEventFlagsClear(_id, flags);
+}
+
+uint32_t EventFlags::get () {


() should be next to the function/method name. In this case ::get()

0xc0170 · 2017-06-26T08:54:46Z

rtos/EventFlags.cpp

+void EventFlags::constructor(const char *name) {
+    memset(&_obj_mem, 0, sizeof(_obj_mem));
+    memset(&_attr, 0, sizeof(_attr));
+    _attr.name = name ? name : "aplication_unnamed_eventflags";


aplication - application typo

0xc0170 · 2017-06-26T08:58:01Z

@YarivCol I left few minor changes requested. If you can rebase, change the owner, would be appreciated. Thanks for adding the simple test. This will need to be extended to test also clear, wait methods, destructor. Can you do that?

bulislaw

Hi @YarivCol thanks for submitting that! I wasn't aware it's a separate pull request I put some comments here #4580 (comment) and here #4580 (comment) can you please treat them as made against this PR.
Also we are trying to move towards more unit testing approach, please see this PR as a example #4620

theotherjimmy · 2017-07-10T15:36:01Z

@sg- @0xc0170 @bulislaw It looks like some changes have happened on this PR since you last reviewed. Could you take a look?

bulislaw · 2017-07-10T15:40:35Z

Last changes 18 days ago, my comment is from 10 days ago.

theotherjimmy · 2017-07-10T15:53:09Z

@bulislaw a rebase will not change the date in github. If the changes are newer than your comment, they will appear below your comment on github.

bulislaw · 2017-07-10T15:55:02Z

Ok. I've checked the code and my comments are not addressed.

theotherjimmy · 2017-07-10T18:20:56Z

Thanks! @YarivCol Could please you address the comments made by @0xc0170 and @bulislaw?

0xc0170 · 2017-07-11T14:48:07Z

Jenkins CI failed to compile main.cpp file : Error: main.cpp@23,21: request for member ‘wait’ in ‘event_flags’, which is of non-class type ‘rtos::EventFlags()’

theotherjimmy · 2017-07-31T15:06:08Z

@YarivCol Would you like help? We would like to get this PR in.

YarivCol · 2017-07-31T17:15:02Z

Yes it will be helpful, unfortunately i dont have free time to finish this PR in the near future :(.

bulislaw · 2017-07-31T17:35:36Z

@YarivCol thanks for contributing that code. I'll try to finish it up in coming days.

bulislaw · 2017-08-03T10:21:53Z

/morph test

geky · 2017-08-03T15:43:53Z

rtos/EventFlags.h

+      @return  event flags before clearing or error code if highest bit set (@a osFlagsError).
+      @note    flags that are specified to wait for will be automatically cleared.
+     */
+    uint32_t wait_all(uint32_t flags = 0, uint32_t timeout = osWaitForever);


Uhm, how do I pass in osFlagsNoClear now?

You don't. I was thinking that we had issues with the RTOS APIs changing and exposing 95% of functionality in abstracted out, simpler and more intuitive way is better than having C++ shim on rtx object. Do we have a use case that osFlagsNoClear is needed and can't be achieved another way?

We're already using osFlagsNoClear in the C layer:
https://github.com/ARMmbed/mbed-os/blob/master/features/FEATURE_LWIP/lwip-interface/lwip-sys/arch/lwip_sys_arch.c#L167-L168

osFlagsNoClear is useful when you want to flag conditions that are not immediately invalidated by the consumer.

ok, how about I'll add a parameter to wait bool clear = true?

That works.

Why not just have the origin wait function along with wait_any and wait_all? The cmsis-rtos APIs have already gone through these iterations. I wouldn't call a standard set of API flags internal details.

mbed-bot · 2017-08-03T16:41:28Z

Result: FAILURE

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

/morph test

bulislaw · 2017-08-04T08:14:01Z

/morph test

mbed-bot · 2017-08-04T10:17:41Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 936

All builds and test passed!

0xc0170 · 2017-08-04T15:13:22Z

The last round of the review? @sg- and @bulislaw please review again

bulislaw · 2017-08-04T15:14:32Z

That is my fix on top of the original work, someone else needs to review it.

0xc0170

2 small things

👍 for taking over this PR @bulislaw

0xc0170 · 2017-08-04T15:14:04Z

rtos/EventFlags.h

+    /** Get the currently set Event Flags.
+      @return  set event flags.
+     */
+    uint32_t get();


could be uint32_t get() const

0xc0170 · 2017-08-04T15:14:24Z

TESTS/mbedmicro-rtos-mbed/event_flags/main.cpp

@@ -0,0 +1,50 @@
+#include "mbed.h"


Can we add the license header to this file please?

You are now contributing

theotherjimmy · 2017-08-14T16:08:06Z

@bulislaw Could you address @0xc0170's comments?

EventFlags class is a wrapper for Event Flag functionality introduced in RTOS2/RTX5.

bulislaw · 2017-08-16T08:57:01Z

/morph test

mbed-bot · 2017-08-16T11:15:17Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1045

All builds and test passed!

0xc0170 · 2017-08-16T11:22:22Z

This is now ready to be integrated, please send your comments if there are still things to be addressed here.

sg- · 2017-08-21T15:46:44Z

Can we get a todo for API docs on this?

AnotherButler · 2017-08-22T06:33:28Z

@sg- I can make a JIRA ticket and add them to my list of docs. Do they need to go into 5.6?

sg- · 2017-08-22T16:53:54Z

yes!

YarivCol changed the title ~~Cpp api for cmsis os 2 EventFlags~~ Add cpp API for CMSIS OS 2 EventFlags Jun 9, 2017

0xc0170 added the needs: review label Jun 14, 2017

sg- previously requested changes Jun 15, 2017

View reviewed changes

This was referenced Jun 16, 2017

RTOS: Fix semaphore #4579

Merged

TCP socket semaphores changed to binary sempahore #4560

Closed

deepikabhavnani mentioned this pull request Jun 16, 2017

Use EventFlags instead of Semaphores #4580

Merged

2 tasks

geky reviewed Jun 16, 2017

View reviewed changes

pan- reviewed Jun 16, 2017

View reviewed changes

adbridge mentioned this pull request Jun 19, 2017

Introduction of RTX/CMSIS 5 has broken semaphone signalling #4584

Closed

0xc0170 reviewed Jun 19, 2017

View reviewed changes

adbridge added needs: work and removed needs: review labels Jun 23, 2017

0xc0170 requested changes Jun 26, 2017

View reviewed changes

bulislaw previously requested changes Jun 30, 2017

View reviewed changes

theotherjimmy added needs: review and removed needs: work labels Jul 10, 2017

0xc0170 removed the needs: review label Jul 24, 2017

geky reviewed Aug 3, 2017

View reviewed changes

0xc0170 reviewed Aug 4, 2017

View reviewed changes

YarivCol and others added 3 commits August 15, 2017 13:27

RTOS: Add EventFlags class

c532a9a

EventFlags class is a wrapper for Event Flag functionality introduced in RTOS2/RTX5.

RTOS: add tests for EvenFlags class

dfbcbdd

RTOS: Changes to the EvenFlags API, doxy and tests

b0ad73e

0xc0170 approved these changes Aug 16, 2017

View reviewed changes

0xc0170 added ready for merge and removed needs: work labels Aug 16, 2017

theotherjimmy merged commit 698b52d into ARMmbed:master Aug 21, 2017

theotherjimmy added release-version: 5.6.0 and removed ready for merge labels Aug 21, 2017


		namespace rtos {

		EventFlags::EventFlags() {

Add cpp API for CMSIS OS 2 EventFlags #4517

Add cpp API for CMSIS OS 2 EventFlags #4517

Uh oh!

Conversation

YarivCol commented Jun 9, 2017

Description

Uh oh!

AnotherButler commented Jun 9, 2017

Uh oh!

YarivCol commented Jun 13, 2017

Uh oh!

sg- left a comment

Choose a reason for hiding this comment

Uh oh!

geky 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!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 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!

0xc0170 commented Jun 22, 2017

Uh oh!

YarivCol commented Jun 22, 2017

Uh oh!

0xc0170 commented Jun 22, 2017

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!

0xc0170 commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Jul 10, 2017

Uh oh!

bulislaw commented Jul 10, 2017

Uh oh!

theotherjimmy commented Jul 10, 2017

Uh oh!

bulislaw commented Jul 10, 2017

Uh oh!

theotherjimmy commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Jul 11, 2017

Uh oh!

theotherjimmy commented Jul 31, 2017

Uh oh!

0xc0170 commented Jun 26, 2017 •

edited

Loading

theotherjimmy commented Jul 10, 2017 •

edited

Loading

bulislaw commented Jul 31, 2017 •

edited

Loading

geky Aug 3, 2017 •

edited

Loading