Skip to content

Unification of wait functions #2274

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

Merged
merged 2 commits into from
Aug 9, 2016
Merged

Unification of wait functions #2274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a1507a
Unification of wait functions
Jul 27, 2016
49606cd
Improved accuracy for microsecond delays
Aug 8, 2016
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
8 changes: 8 additions & 0 deletions hal/common/mbed_wait_api.c → hal/common/mbed_wait_api_no_rtos.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// This implementation of the wait functions will be compiled only
// if the RTOS is not present.
#ifndef MBED_CONF_RTOS_PRESENT

#include "wait_api.h"
#include "us_ticker_api.h"

Expand All @@ -28,3 +33,6 @@ void wait_us(int us) {
uint32_t start = us_ticker_read();
while ((us_ticker_read() - start) < (uint32_t)us);
}

#endif // #ifndef MBED_CONF_RTOS_PRESENT

50 changes: 50 additions & 0 deletions hal/common/mbed_wait_api_rtos.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/* mbed Microcontroller Library
* Copyright (c) 2006-2013 ARM Limited
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// This implementation of the wait functions will be compiled only
// if the RTOS is present.
#ifdef MBED_CONF_RTOS_PRESENT

#include "wait_api.h"
#include "us_ticker_api.h"
#include "rtos.h"
#include "critical.h"

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

void wait_ms(int ms) {
wait_us(ms * 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

you might consider having wait and wait_ms use Thread:wait exclusively, since the RTOS will get you very close to ms resoultion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd mean adding core_utils_are_interrupts_enabled checks to each wait function. Not sure this is needed.

}

void wait_us(int us) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to sacrifice accuracy here. If you read the current time before Thread:wait() you should be able to accurately busy wait for the rest of the time:

uint32_t start = us_ticker_read();

// Use RTOS wait for whole milliseconds
int ms = us / 1000;
if (ms > 0) {
    Thread::wait((uint32_t)ms);
}
// Busy wait for remaining time
while ((us_ticker_read() - start) < (uint32_t)us);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that might help in some cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

The accuracy concern still exists with neighboring threads locking interrupts over millisecond boundaries or if preemption occurs in the busy loop. I don't think it's really an issue, just a notable side effect of running an rtos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but writing it like @c1728p9 suggested might still improve accuracy a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good point

uint32_t start = us_ticker_read();
// Use the RTOS to wait for millisecond delays if possible
int ms = us / 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

Guessing there is a bit twiddling shift that is more appropriate to at least have a deterministic execution cycle count. Not to handle now but on M0 this will be a bit off but maybe it always has been.

if ((ms > 0) && core_util_are_interrupts_enabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend asserting if wait is called from an interrupt since this is likely not what the user wants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily. Very short delays in interrupt context might still be OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @bogdanm here. Waiting in irq isn't something we should encourage, but I would expect it to work. It wouldn't play well with the rtos, but sometimes an application simply doesn't care. Asserting in wait would also break backwards-compatiblity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since there really wasn't an alternative to this in 2.0 I can see the need to support interrupts. It would be nice if the user could be warned if there are long delays from interrupts, since they could lead to failures in other components.

Thread::wait((uint32_t)ms);
us -= ms * 1000;
}
// Use busy waiting for sub-millisecond delays, or for the whole
// interval if interrupts are not enabled
if (us > 0) {
while((us_ticker_read() - start) < (uint32_t)us);
}
}

#endif // #if MBED_CONF_RTOS_PRESENT