Mbed-os: Improve logic for RTOS wait_us in the sub-millisecond case

Created on 2 Jul 2018  路  5Comments  路  Source: ARMmbed/mbed-os

Description


Simple question that may lead to an enhancement. In RTOS mode, the wait_us function executes a division there https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_wait_api_rtos.cpp#L40 , executed in all cases, so even for sub-millisecond delays. This is only an integer division so it's not that bad, but considering the sub-millisecond case, wouldn't it preferable to avoid any costly operation, and just test if ( us < 1000 ) for that case, leaving the more complicated logic for the longer waits ? I haven't the equipment to test if it enhances the situation or not, but feel like it could for very small values (<10碌s for example).

Issue request type

[ ] Question
[X] Enhancement
[ ] Bug

closed_in_jira drivers mirrored

Most helpful comment

There are a number of issues with wait() (eg https://github.com/ARMmbed/mbed-os/issues/7154 ), and I've been generally worried about how to improve that without backwards compatibility issues.

You're right, this could be trivially tightened up a little to speed up the sub-millisecond case without affecting behaviour:

if ((us >= 1000) && core_util_are_interrupts_enabled()) {
    sleep_manager_lock_deep_sleep();
    Thread::wait((uint32_t) (us / 1000));
    sleep_manager_unlock_deep_sleep();
}

All 5 comments

ARM Internal Ref: MBOTRIAGE-1035

There are a number of issues with wait() (eg https://github.com/ARMmbed/mbed-os/issues/7154 ), and I've been generally worried about how to improve that without backwards compatibility issues.

You're right, this could be trivially tightened up a little to speed up the sub-millisecond case without affecting behaviour:

if ((us >= 1000) && core_util_are_interrupts_enabled()) {
    sleep_manager_lock_deep_sleep();
    Thread::wait((uint32_t) (us / 1000));
    sleep_manager_unlock_deep_sleep();
}

@LaurentLouf and @kjbracey-arm - This is very reasonable, we will make this change. Thanks.

The PR has been merged.

Was this page helpful?
0 / 5 - 0 ratings