Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Wait API updated to remove deepsleep lock #8189

Merged
merged 4 commits into from
Nov 22, 2018
Merged

Wait API updated to remove deepsleep lock #8189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
957f240
Remove deep sleep lock from wait_ms
Oct 15, 2018
f5e28fe
Edit mbed_wait_api.h
Oct 30, 2018
5a87f04
Update doc for wait_ms
Oct 30, 2018
c58c80e
Make minor copy edits to mbed_wait_api.h
Oct 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions platform/mbed_wait_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ extern "C" {
* @param s number of seconds to wait
*
* @note
* If the RTOS is present, 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 ThisThread::sleep_for().
* If the RTOS is present, this function spins to get the exact number of microseconds for
* microsecond precision up to 10 milliseconds. If delay is larger than 10 milliseconds and not in ISR, it is the same as
* `wait_ms`. We recommend `wait_us` and `wait_ms` over `wait`.
*/
void wait(float s);

Expand All @@ -66,9 +66,8 @@ void wait(float s);
* @param ms the whole number of milliseconds to wait
*
* @note
* If the RTOS is present, 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 ThisThread::sleep_for().
* If the RTOS is present, it calls ThisThread::sleep_for(), which is same as CMSIS osDelay().
* You can't call this from interrupts, and it doesn't lock hardware sleep.
*/
void wait_ms(int ms);

Expand All @@ -77,8 +76,9 @@ void wait_ms(int ms);
* @param us the whole number of microseconds to wait
*
* @note
* If the RTOS is present, this function always spins to get the exact number of microseconds,
* which potentially affects power (such as preventing deep sleep) and multithread performance.
* This function always spins to get the exact number of microseconds.
* If RTOS is present, this will affect power (by preventing deep sleep) and
* multithread performance. Therefore, spinning for millisecond wait is not recommended.
*/
void wait_us(int us);

Expand Down
44 changes: 34 additions & 10 deletions platform/mbed_wait_api_rtos.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,18 @@
#include "rtos/rtos.h"
#include "platform/mbed_critical.h"
#include "platform/mbed_power_mgmt.h"
#include "platform/mbed_error.h"
#include "rtos/ThisThread.h"

void wait(float s)
{
wait_us(s * 1000000.0f);
}

void wait_ms(int ms)
{
wait_us(ms * 1000);
}
if ((s >= 0.01f) && core_util_are_interrupts_enabled()) {
wait_ms(s * 1000.0f);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If wait is used from an interrupt handler it will only trap if the delay is >= 10ms. Is this acceptable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.. wait will not trap

return;
}

void wait_us(int us)
{
uint32_t us = (s * 1000000.0f);
const ticker_data_t *const ticker = get_us_ticker_data();

uint32_t start = ticker_read(ticker);
if ((us >= 1000) && core_util_are_interrupts_enabled()) {
// Use the RTOS to wait for millisecond delays if possible
Expand All @@ -50,5 +47,32 @@ void wait_us(int us)
while ((ticker_read(ticker) - start) < (uint32_t)us);
}

/* The actual time delay may be up to one timer tick less - 1 msec */
void wait_ms(int ms)
{
if (core_util_is_isr_active() || !core_util_are_interrupts_enabled()) {
#if defined(MBED_TRAP_ERRORS_ENABLED) && MBED_TRAP_ERRORS_ENABLED
MBED_ERROR(MBED_MAKE_ERROR(MBED_MODULE_PLATFORM, MBED_ERROR_INVALID_OPERATION),
"Deprecated behavior: milli-sec delay should not be used in interrupt.\n");
#else
wait_us(ms * 1000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated by Kevin, we could use the low power ticker in that case.
It would be useful to at least print a warning here as behaviour with and without MBED_TRAP_ERRORS_ENABLED sets are too very different (trap or execute the intended action).
Note that this is a breaking change and it should be documented as one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't want to use additional ticker for backwards compatibility, we should be discouraging users from using wait_ms in interrupt mode.

Note that this is a breaking change and it should be documented as one.

I am not sure how we do this, I have updated the API description and will make sure code does not get in patch release. Do let me know how we can communicate this in better way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok we either trap or not, having it error out in one mode, but happily work without even a warning, in other doesn't make much sense to me.

I don't get this part and I really hope every use of MBED_ERROR doesn't forces users to add this if:

#if defined(MBED_TRAP_ERRORS_ENABLED) && MBED_TRAP_ERRORS_ENABLED
        MBED_ERROR(MBED_MAKE_ERROR(MBED_MODULE_PLATFORM, MBED_ERROR_INVALID_OPERATION),
                    "Deprecated behavior: milli-sec delay should not be used in interrupt.\n");
#else

I'm happy to show a warning and change the behaviour for next major.

#endif
} else {
rtos::ThisThread::sleep_for(ms);
}
}

/* The actual time delay may be 1 less usec */
void wait_us(int us)
{
if (us > 10000) {
Copy link
Author

@deepikabhavnani deepikabhavnani Sep 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjbracey-arm - 0.1s / 0.01s? I would like to keep it same for https://github.com/ARMmbed/mbed-os/pull/8189/files#diff-5a106edf0944ae7ba2d1bf95153fb0a0R30

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, 0.01s makes more sense. I think I probably meant to type that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.01s seems fine

MBED_WARNING(MBED_MAKE_ERROR(MBED_MODULE_PLATFORM, MBED_ERROR_UNKNOWN),
"wait_us blocks deep sleep, wait_ms recommended for long delays\n");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was suggesting dropping the sleep functionality from this entirely - restoring it to be the same as the non-RTOS version, hence the message above. What's your thought on that?

Copy link
Author

@deepikabhavnani deepikabhavnani Sep 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes can do that. Thinking of this again, do you mean remove Thread::wait and just spin?
Instead of polling continuously like non-RTOS version https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_wait_api_no_rtos.c#L36. I would prefer yielding to other threads for RTOS

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's kind of my design point. I also prefer yielding to other threads, but I want to define whether a call yields or not - and we would already have the yielding call, which is wait_ms.

This is a call which already spins for up to a millisecond to do its business, so is already inherently thread-unfriendly - I want people to be using wait_ms to avoid any spin.

Uses of this should be rare. (Can we do a quick audit - how many actual direct wait_us users are there in tree? Most examples I can think of are GPIO "reset" pulses on init.)

And there are some occasions where it is important not to yield, so I'd like to have an API explicitly defined as that, not dependent on parameter.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets get more opinions over here
@TacoGrandeTX @c1728p9 @geky @bulislaw @stevew817 @NeilMacMullen - Please share your thoughts for wait_us

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and this behaviour was my original reason to have my value warning above - we're saying "you do realise you're spinning for 10ms, right?". Not just the deep sleep issue.

I also think you've got a better chance of getting the time you were hoping each time with the spin than if you yield, where you might suddenly be hit with a full 10ms timeslice from another running thread.

Basically anyone wanting sub-millisecond resolution is having special requirements that can't really be catered for if you start dragging the RTOS into it. My feeling is that the current wait_us behaviour is purely to handle the wait_ms and wait built on top of it, rather than something I'd want an explicit wait_us to do.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the intent here to have wait_us as spinning only, but to be true I am not sure of all use cases it is used for currently, hence will like to know from others as well.

Also agree that current wait_us behavior is to handle wait built on top of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, just searched through the in-tree source, and I can see around a 100 calls to wait_us, but I can't immediately identify any calls that might ask for >=1000us, except for wait_ms and wait themselves.

Actually, no, I see a couple in one of the "onboard_modem_api.c" files, in "wiggle the reset line" code. They're a full second, so would hit the error/warning.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its wait_ms - Should not have issue, unless called from interrupt
https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_NXP/TARGET_LPC176X/TARGET_UBLOX_C027/onboard_modem_api.c#L32

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thought, we can change wait_us to be always spinning, and update wait to current behavior of wait_us

const ticker_data_t *const ticker = get_us_ticker_data();
uint32_t start = ticker_read(ticker);
while ((ticker_read(ticker) - start) < (uint32_t)us);
}

#endif // #if MBED_CONF_RTOS_PRESENT