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

mbed_wait_api: add comments to warn the func will lock deep sleep #6645

Merged
merged 5 commits into from May 8, 2018

Conversation

@woodsking2

woodsking2 commented Apr 16, 2018

Description

Add some comments to warn the wait() will lock deep sleep.

Pull request type

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

discuss: #6574

@0xc0170 0xc0170 requested review from bulislaw, c1728p9 and SenRamakri Apr 16, 2018

@cmonr

Minor grammer nits.

@@ -53,18 +53,27 @@ extern "C" {
* the accuracy of single precision floating point).
*
* @param s number of seconds to wait
*
* @note
* This func will lock the deep sleep, if you want device entry deep sleep mode, please use Thread::wait()

This comment has been minimized.

@cmonr

cmonr Apr 17, 2018

Contributor

Minor grammer nits.

"This func will lock deep sleep. If you want device entry deep sleep mode, please use Thread::wait()"

James Wang
@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 17, 2018

I'd still rather we fixed this than documented it.

If documenting it, I'd want the other feature documented, which is that wait_ms() and wait(float) always spin to get an exact number of microseconds, potentially impacting power and multi-thread performance. Again, avoided by Thread::wait().

James Wang
Accept Kevin Bracey's review, "wait_ms() and wait(float) always spin …
…to get an exact number of microseconds, potentially impacting power and multi-thread performance. Again, avoided by Thread::wait()".
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 17, 2018

Thanks @woodsking2 for the update

@AnotherButler Please review the docs update

*/
void wait_ms(int ms);
/** Waits a number of microseconds.
*
* @param us the whole number of microseconds to wait
*
* @note
* This func always spin to get an exact number of microseconds, potentially

This comment has been minimized.

@AnotherButler

AnotherButler Apr 17, 2018

Contributor

Please change to "This function always spins to get the exact number of microseconds, which potentially affects power (such as preventing deep sleep) and multithread performance."

*/
void wait(float s);
/** Waits a number of milliseconds.
*
* @param ms the whole number of milliseconds to wait
*
* @note
* This func always spin to get an exact number of microseconds, potentially

This comment has been minimized.

@AnotherButler

AnotherButler Apr 17, 2018

Contributor

Please change to "This function always spins to get the exact number of microseconds, which potentially affects power (such as preventing deep sleep) and multithread performance. You can avoid it by using Thread::wait()."

@@ -53,18 +53,32 @@ extern "C" {
* the accuracy of single precision floating point).
*
* @param s number of seconds to wait
*
* @note
* This func always spin to get an exact number of microseconds, potentially

This comment has been minimized.

@AnotherButler

AnotherButler Apr 17, 2018

Contributor

Please change to "This function always spins to get the exact number of microseconds, which potentially affects power (such as preventing deep sleep) and multithread performance. You can avoid it by using Thread::wait()."

James Wang
accept Amanda Butler's review. change to "This function always spins …
…to get the exact number of microseconds, which potentially affects power (such as preventing deep sleep) and multithread performance. You can avoid it by using Thread::wait()."
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 18, 2018

I've left a few changes. Also, should we leave a note here: https://os-doc-builder.test.mbed.com/docs/development/reference/wait.html?

Yes, we should update.

@woodsking2 Can you send a patch to docs to update this once this is integrated?

Testing now

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Apr 18, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 18, 2018

I already see a note:

Avoiding OS delay

When you call wait, your board's CPU is in a loop waiting for the required time to pass. Using the Arm Mbed RTOS, you can make a call to Thread::wait instead.

That's not 100% accurate - it will sleep in the RTOS for the whole number of milliseconds, then spin as needed to make the necessary fraction of a millisecond. But it blocks the platform deep sleep for the entire duration.

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 18, 2018

Build : SUCCESS

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

Triggering tests

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

@woodsking2

This comment has been minimized.

woodsking2 commented Apr 18, 2018

@0xc0170 Sorry, I don't know how send a patch and I can't open https://os-doc-builder.test.mbed.com

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 18, 2018

@orenc17 CAn you please review last 10 runs in uvisor (one device failed few times there)

/morph uvisor-test

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 19, 2018

/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added needs: review and removed needs: CI labels Apr 19, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 20, 2018

I've left a few changes. Also, should we leave a note here: https://os-doc-builder.test.mbed.com/docs/development/reference/wait.html?

@AnotherButler Can you update it based on this ? This one - approved or changes required?

@adbridge

This comment has been minimized.

Contributor

adbridge commented Apr 30, 2018

@woodsking2 Please do not modify the structure of the PR body. I have edited it to put it back to how it should be. Thanks

@adbridge

This comment has been minimized.

Contributor

adbridge commented Apr 30, 2018

@AnotherButler Could you please address @0xc0170 comments above? Thanks

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented Apr 30, 2018

So, if I'm understanding correctly, I should change the Handbook Wait page warning from:

"When you call wait, your board's CPU is in a loop waiting for the required time to pass. Using the Arm Mbed RTOS, you can make a call to Thread::wait instead."

to:

"When you call wait, your board's CPU will sleep in the RTOS for the whole number of milliseconds and then spin to make the necessary fraction of a millisecond. However, it blocks the platform deep sleep for the entire duration."

Is that right?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented May 2, 2018

That's accurate for the current RTOS implementation.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 2, 2018

@bulislaw / @SenRamakri Please review the latest update.

* @note
* This function always spins to get the exact number of microseconds,
* which potentially affects power (such as preventing deep sleep) and multithread performance.
* You can avoid it by using Thread::wait().

This comment has been minimized.

@SenRamakri

SenRamakri May 2, 2018

Contributor

Hi @woodsking2 - The rtos based implementation of this function does call Thread::wait. So this comment is not broadly applicable.

@woodsking2 woodsking2 dismissed stale reviews from 0xc0170 and kjbracey-arm via c9b2640 May 3, 2018

@woodsking2

This comment has been minimized.

woodsking2 commented May 3, 2018

@SenRamakri
Add "If the RTOS is present, " comments.
Please review.

@0xc0170

0xc0170 approved these changes May 3, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 8, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: review labels May 8, 2018

@cmonr

cmonr approved these changes May 8, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented May 8, 2018

Build : SUCCESS

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

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

This comment has been minimized.

Member

0xc0170 commented May 8, 2018

/morph mbed2-build

@cmonr cmonr merged commit 68ad00f into ARMmbed:master May 8, 2018

13 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, 582 warnings (+0 warnings)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10372 cycles (+333 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10112B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label May 8, 2018

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