Use EventFlags instead of Semaphores #4580

deepikabhavnani · 2017-06-16T21:35:01Z

Description

Synchronization in sockets required binary semaphores which is not available
in RTX5, hence switching to event flags.
Issue #4575

Migrations

PR should be merged after PR #4517. EventFlags Class implementation is picked up from this PR.

Related PRs

PR #4517
PR #4560

Todos

Merge / Rebase to PR Add cpp API for CMSIS OS 2 EventFlags #4517
Tests

geky

Looking good, yay for reducing a semaphore object : )

geky · 2017-06-16T22:11:56Z

features/netsocket/TCPSocket.cpp

-
-            if (count < 1) {
-                // Semaphore wait timed out so break out and return
+            if (0 == (flag & WRITE_FLAG)) {


One of the surprising annoyances of the new osEventFlags API, is that wait actually returns error codes on timeout:
https://www.keil.com/pack/doc/CMSIS/RTOS2/html/group__flags__error__codes.html

These conditions should probably be something like:

if ((flag & WRITE_FLAG) != WRITE_FLAG) {

@geky: Both conditions are logically same. I prefer comparison with zero over fix values. Any specific reason for other condition?

_event_flag.wait returns 0xFFFFFFFEU on timeout, not zero (I don't really get why but it is what it is):
https://www.keil.com/pack/doc/CMSIS/RTOS2/html/group__flags__error__codes.html

They set the most significant bit to 1 (osFlagsError) to mark error. To check for any error it's enough to check ret | osFlagsError

Ah good to know, that works too 👍

bulislaw · 2017-06-19T11:42:20Z

features/netsocket/TCPSocket.cpp

@@ -18,8 +18,11 @@
 #include "Timer.h"
 #include "mbed_assert.h"

+#define READ_FLAG           0x1u
+#define WRITE_FLAG          (0x1u << 1)


Can we have a final value here? It makes debugging way easier when you can actually grep the code to find what the value is. Also `<< 1`` doesn't help with readability here.

bulislaw · 2017-06-19T14:47:15Z

rtos/EventFlags.h

+
+    /** Set the specified Event Flags.
+      @param   flags  specifies the flags that shall be set.
+      @return  event flags after setting or error code if highest bit set.


Worth mentioning a osFlagsErrorfor all the return values, that's a macro with highest bit set, so people know they can use it rather than defining their own.

bulislaw · 2017-06-19T14:53:43Z

rtos/EventFlags.h

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


I'm not sure about that, it's a very thin wrapper on RTX, it caused us some grief with RTX update (as we are not supposed to break the APIs). I would say we could abstract it out a bit, wait could be for any flags if flag is 0 and for specified flag in other cases. If we do that we can ignore the options parameter (and the remaining 'don't clear' option or add clear parameter defaulting to true.

@bulislaw : User can have multiple flags say 5, and can select any two bits in flag option to wait for any "one" of them to be set in case of "osFlagsWaitAny" If we set flag to "0", we cannot know wait for which specific set of bits.
This option will change the behavior of RTX API's.

I think following is reasonable:

#define flag1 0x1 #define flag2 0x2 /*[...]*/ flag.wait(flag1) /* Waits only for the flag1 */ flag.wait(flag2) /* Waits only for the flag2 to be set */ res = flag.wait(0) /* Waits for anything to be set */ if (res | flag1 || res | flag2) { \*...*/ }

but we could also introduce wait_any or something similar to only interrupt the wait if any one of the flags finish. I think this form of API is easier and more user friendly.

Sounds good. But what should be the option for "osFlagsWaitAll" flag? Wait for flag1 and flag2 both

wait(flag1 | flag2) should translate to wait with osFlagsWaitAll and wait(0) to wait for 0x8FFFFFF flag (msb set to one is an error so it's not allowed) with osFlagsWaitAny.

Only case I don't see getting covered here is: I need to wait for any 1 or 3

#define flag3 0x4 flag.wait(flag1 | flag3) - Will wait for 1 and 3 flag.wait(0) - Will wait for all three (2 is not required in my case)

Also, forgot timeout: It should be wait(flag, timeout). Correct ?

Yes I skipped the timeout for brevity as it has default 0. I think case s you mentioned can be handled by either:

res = flag.wait(0) /* Waits for anything to be set */ if (res | flag1 || res | flag3) { \*...*/ }

or the wait in a loop for that mater. Second option would be to add wait_any member. I think adding second member function is cleaner. Also worth mentioning in docs that flags are 1bit long.

theotherjimmy · 2017-06-29T15:34:36Z

@bulislaw does this most recent commit address your comments?

deepikabhavnani · 2017-06-29T17:12:17Z

@theotherjimmy : Most recent comment by @bulislaw is related to event flag Class.
He is suggesting to change wait API in eventflag Class

bulislaw · 2017-06-30T09:18:27Z

EventFlag Class wasn't updated, I would say that because of 'no API breakage policy' new public classes shouldn't be introduced lightly, there should be wider discussion about them. I don't think the new class is ready.

bulislaw · 2017-06-30T09:19:09Z

@0xc0170 @c1728p9 @geky @pan- can you review the new EvenFlag class please

0xc0170 · 2017-06-30T09:51:16Z

OK now I am confused, so #4517 is adding it , still under review, this is on top of it but doing also changes to the class itself :-) It would be easier to have only that class addition as it is in 4517 and doing edits there (having write access or take over that PR and send a new one ?) ?

bulislaw · 2017-06-30T10:07:17Z

Ah i missed that! I'll have a look at #4517

deepikabhavnani · 2017-08-28T17:30:11Z

@bulislaw - I have rebased the code and using updated API's. Please review

bulislaw

@geky can you review as well.

bulislaw · 2017-08-29T09:49:59Z

features/netsocket/TCPSocket.cpp

-    _write_sem.release();
-    _read_sem.release();
-
+    _event_flag.set(WRITE_FLAG);


Why not to set it in one go? WRITE_FLAG | READ_FLAG?

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

bulislaw

LGTM

geky

Looks good to me 👍

studavekar · 2017-09-03T09:58:18Z

/morph test-nightly

mbed-bot · 2017-09-03T16:37:57Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1187

All builds and test passed!

geky mentioned this pull request Jun 16, 2017

RTOS: Fix semaphore #4579

Merged

deepikabhavnani force-pushed the wifi_34 branch from fa6d673 to 27fab8a Compare June 16, 2017 22:04

geky reviewed Jun 16, 2017

View reviewed changes

theotherjimmy changed the title ~~Using EventFlags instead of Semaphores~~ Use EventFlags instead of Semaphores Jun 16, 2017

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

bulislaw suggested changes Jun 19, 2017

View reviewed changes

theotherjimmy added needs: work component: connectivity and removed needs: review labels Jun 19, 2017

theotherjimmy added the needs: preceding PR label Jul 10, 2017

geky mentioned this pull request Jul 18, 2017

[NETSOCKET] TCPSocket & UDPSocket event handler cannot be run in IRQ context #4343

Closed

deepikabhavnani force-pushed the wifi_34 branch from c75f3c5 to b2315b2 Compare August 28, 2017 17:28

deepikabhavnani added needs: review and removed needs: work labels Aug 28, 2017

bulislaw reviewed Aug 29, 2017

View reviewed changes

theotherjimmy removed needs: preceding PR component: connectivity labels Aug 29, 2017

RTOS: Add EventFlags class

beb5aac

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

deepikabhavnani force-pushed the wifi_34 branch from b2315b2 to beb5aac Compare August 29, 2017 19:08

bulislaw approved these changes Aug 30, 2017

View reviewed changes

theotherjimmy removed the needs: review label Aug 30, 2017

0xc0170 approved these changes Aug 31, 2017

View reviewed changes

0xc0170 added the needs: CI label Aug 31, 2017

geky approved these changes Aug 31, 2017

View reviewed changes

0xc0170 added ready for merge release-version: 5.6.0 and removed needs: CI labels Sep 4, 2017

0xc0170 merged commit abafc6e into ARMmbed:master Sep 4, 2017

0xc0170 removed the ready for merge label Sep 4, 2017

deepikabhavnani deleted the wifi_34 branch December 19, 2017 16:55

Use EventFlags instead of Semaphores #4580

Use EventFlags instead of Semaphores #4580

Uh oh!

Conversation

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

Uh oh!

Description

Migrations

Related PRs

Todos

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!

geky Jun 19, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

geky Jun 19, 2017 • 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!

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!

deepikabhavnani Jun 19, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Jun 29, 2017

Uh oh!

deepikabhavnani commented Jun 29, 2017

Uh oh!

bulislaw commented Jun 30, 2017

Uh oh!

bulislaw commented Jun 30, 2017

Uh oh!

0xc0170 commented Jun 30, 2017

Uh oh!

bulislaw commented Jun 30, 2017

Uh oh!

deepikabhavnani commented Aug 28, 2017

Uh oh!

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

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

studavekar commented Sep 3, 2017

Uh oh!

mbed-bot commented Sep 3, 2017

deepikabhavnani commented Jun 16, 2017 •

edited

Loading

geky Jun 19, 2017 •

edited

Loading

geky Jun 19, 2017 •

edited

Loading

deepikabhavnani Jun 19, 2017 •

edited

Loading