update examplea replace deprecated wait and wait_ms api #59

jamesbeyond · 2019-12-03T22:05:18Z

wait() and wait_ms() been deprecated, so in the example repo. replace those function call with either ThisThread::sleep_for where sleep can happen and wait_us() where we don't want sleep

fkjagodzinski

Everything, apart from that one changed value, looks OK.

APIs_RTOS/EventFlags/main.cpp

bulislaw · 2019-12-04T13:59:12Z

Tutorials_UsingAPIs/Flow-Control-Busy-Wait-Button/main.cpp

@@ -9,7 +9,7 @@ int main() {
    while(1) {
        if(BUTTON_PRESS == button){
           led = !led;
-           wait(1);
+           wait_us(1000000);


Why is that a busy wait? The example is called busy-wait because we don't use semaphore etc, but we shouldn't push users towards busy loops.

I convert it to the wait_us() because the whole example page is talking about busy wait. if we change it to sleep, I think we need to chnage som contents to docs page as well:
https://os.mbed.com/docs/mbed-os/v5.14/tutorials/application-flow-control.html

I agree with Bartek that we should limit usage of wait_us so we will have to rewrite the application flow control tutorial. I will add a task to work on this next sprint.

For this PR, I propose that we use wait_us only in the first example (Tutorials_UsingAPIs/Flow-Control-Busy-Wait/main.cpp ) and that we emphasize that busy-wait is to be avoided. All other examples should use ThisThread::sleep_for.

Here we should use ThisThread::sleep_for(1000) and rename the example Flow-Control-Active-Polling-Button.

bulislaw · 2019-12-04T13:59:40Z

Tutorials_UsingAPIs/Flow-Control-Busy-Wait/main.cpp

@@ -6,9 +6,9 @@ int main() {
    while(1) {
        myled = 1;
        // printf("LED On\r\n");
-        wait(0.2);
+        wait_us(200000);


Same here, no need to busy wait.

As per previous comment, I propose that this is the only example that uses wait_us. Let's emphasize further by adding a comment

wait_us(200000); // Busy wait

When updating the tutorial, we should add a forth option : "sleep" option.

bulislaw · 2019-12-04T13:59:48Z

Tutorials_UsingAPIs/Flow-Control-Interrupt-Button/main.cpp

@@ -12,6 +12,6 @@ int main() {
    button.rise(&toggle);  // call toggle function on the rising edge
    while(1) {             // wait around, interrupts will interrupt this!
        heartbeat = !heartbeat;
-        wait(0.25);
+        wait_us(250000);


Let's use ThisThread::sleep_for(250)

bulislaw · 2019-12-04T14:00:00Z

Tutorials_UsingAPIs/Flow-Control-Ticker/main.cpp

@@ -15,6 +15,6 @@ int main() {
    // spin in a main loop. flipper will interrupt it to call flip
    while(1) {
        led1 = !led1;
-        wait(0.2);
+        wait_us(200000);


And here :)

fkjagodzinski

No more comments from me. 👍

jamesbeyond · 2020-03-05T11:51:34Z

ARMmbed/mbed-os#12572

evedon

Could you also run AStyle - some files don't follow the formatting guidelines.

evedon · 2020-03-11T14:41:48Z

Tutorials_UsingAPIs/Flow-Control-Busy-Wait-Button/main.cpp

@@ -9,7 +9,7 @@ int main() {
    while(1) {
        if(BUTTON_PRESS == button){
           led = !led;
-           wait(1);
+           wait_us(1000000);


I agree with Bartek that we should limit usage of wait_us so we will have to rewrite the application flow control tutorial. I will add a task to work on this next sprint.

For this PR, I propose that we use wait_us only in the first example (Tutorials_UsingAPIs/Flow-Control-Busy-Wait/main.cpp ) and that we emphasize that busy-wait is to be avoided. All other examples should use ThisThread::sleep_for.

Here we should use ThisThread::sleep_for(1000) and rename the example Flow-Control-Active-Polling-Button.

evedon · 2020-03-11T14:50:07Z

Tutorials_UsingAPIs/Flow-Control-Busy-Wait/main.cpp

@@ -6,9 +6,9 @@ int main() {
    while(1) {
        myled = 1;
        // printf("LED On\r\n");
-        wait(0.2);
+        wait_us(200000);


As per previous comment, I propose that this is the only example that uses wait_us. Let's emphasize further by adding a comment

wait_us(200000); // Busy wait

When updating the tutorial, we should add a forth option : "sleep" option.

evedon · 2020-03-11T14:51:20Z

Tutorials_UsingAPIs/Flow-Control-Interrupt-Button/main.cpp

@@ -12,6 +12,6 @@ int main() {
    button.rise(&toggle);  // call toggle function on the rising edge
    while(1) {             // wait around, interrupts will interrupt this!
        heartbeat = !heartbeat;
-        wait(0.25);
+        wait_us(250000);


Let's use ThisThread::sleep_for(250)

evedon · 2020-03-11T14:58:02Z

Short copyrights need to be added as well.

Co-Authored-By: Filip Jagodziński <[email protected]>

jamesbeyond · 2020-03-11T18:35:09Z

Could you also run AStyle - some files don't follow the formatting guidelines.

all of the comments been addressed

jamesbeyond · 2020-03-11T18:36:37Z

Short copyrights need to be added as well.

I'll add the copyright to all the files missing in a separate PR, possibly adding a CI to check the license as well

jamesbeyond · 2020-03-12T09:31:15Z

@evedon, could you review again

jamesbeyond requested review from fkjagodzinski and bulislaw December 3, 2019 22:05

fkjagodzinski suggested changes Dec 4, 2019

View reviewed changes

APIs_RTOS/EventFlags/main.cpp Outdated Show resolved Hide resolved

bulislaw reviewed Dec 4, 2019

View reviewed changes

fkjagodzinski approved these changes Dec 6, 2019

View reviewed changes

jamesbeyond mentioned this pull request Mar 5, 2020

Remove mbed wait deprecated APIs ARMmbed/mbed-os#12572

Merged

evedon requested changes Mar 11, 2020

View reviewed changes

Qinghao Shi and others added 4 commits March 11, 2020 18:25

update examplea replace deprecated wait and wait_ms api

dd07f0d

Update APIs_RTOS/EventFlags/main.cpp

e012ce4

Co-Authored-By: Filip Jagodziński <[email protected]>

address github comments

c604891

rename example

f73860b

jamesbeyond force-pushed the depr_wait branch from 3755a8c to f73860b Compare March 11, 2020 18:34

Qinghao Shi added 2 commits March 11, 2020 18:43

update ci scripts to ignored renamed file

f57c7d0

fix a typo

4c62ada

jamesbeyond force-pushed the depr_wait branch from 505361e to 4c62ada Compare March 12, 2020 09:16

evedon approved these changes Mar 12, 2020

View reviewed changes

jamesbeyond merged commit d5c93cd into ARMmbed:master Mar 12, 2020

update examplea replace deprecated wait and wait_ms api #59

update examplea replace deprecated wait and wait_ms api #59

Uh oh!

Conversation

jamesbeyond commented Dec 3, 2019

Uh oh!

fkjagodzinski left a comment

Choose a reason for hiding this comment

Uh oh!

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!

fkjagodzinski left a comment

Choose a reason for hiding this comment

Uh oh!

jamesbeyond commented Mar 5, 2020

Uh oh!

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

evedon commented Mar 11, 2020

Uh oh!

jamesbeyond commented Mar 11, 2020

Uh oh!

jamesbeyond commented Mar 11, 2020

Uh oh!

jamesbeyond commented Mar 12, 2020

Uh oh!

Uh oh!