Skip to content

Add extra delta latency on common tickers lp ticker speed greentea test #12880

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

Closed
wants to merge 1 commit into from
Closed

Add extra delta latency on common tickers lp ticker speed greentea test #12880

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 10 additions & 4 deletions TESTS/mbed_hal/common_tickers/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ extern "C" {

#define MAX_FUNC_EXEC_TIME_US 20
#define DELTA_FUNC_EXEC_TIME_US 5
#if defined(__MICROLIB)
Copy link
Contributor

Choose a reason for hiding this comment

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

we dont have small std rather but specific one (microlib) only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Microlib slow division causes this issue of slow execution and perfectly works with newlib-nano

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be an option to disable microlib on this platform, pending resolution of the speed issue?

Copy link
Contributor Author

@rajkan01 rajkan01 Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@evedon @MarceloSalazar please comment on your view to disable microlib on this platform

Copy link

@MarceloSalazar MarceloSalazar Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not keen on adding specific target dependencies as the code becomes a bit of a mess and it's hard to maintain/scale to support other targets.
But we can detect the situation that prevents code from being executed and skip accordingly (for now). Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant not support use of microlib on this target - I believe it can be flagged as unsupported in targets.json?

Copy link

Choose a reason for hiding this comment

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

Probably a question for @jeromecoutant on how he thinks this could/should be handled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think I will "unsupport" some specific targets, before we understand the real reason!
Please correct first #12221

Copy link
Contributor

@kjbracey kjbracey Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Another approach would be to optimise HAL_GetTick. Once the OS is running, it is going via the generic ticker_read_us, which is a fully-generalised routine to get microseconds.

And I believe it's only ever used to get small timing delays. A suggestion would be to use us_ticker_read directly, as you do when mbed_sdk_initted is false. I don't see what the if condition is gaining you there - why can't you always use the "uninited" code?

I'm busy space/speed optimising ticker_read_us, but still, you can just bypass it.

And then the next trick is to optimise the division for locality. When speed matters, you know little time has elapsed. So divide the difference by 1000, not the total. The standard divide routines should terminate fast for small inputs, but if they don't you could code them yourself. And it's a 32-bit division, not a 64-by-32 one.

(Were you using ticker_read_us to get a properly wrapping 32-bit tick by dividing down the 64-bit microseconds? I guess the non-initted code doesn't give you that because it's dividing down 32-bits, but the below code deals with it).

Something like

 uint32_t prev_time, remainder;
 uint32_t new_time = us_ticker_read();
 uint32_t elapsed_time = ((new_time - prev_time) & US_TICKER_MASK) + remainder;
 uint32_t elapsed_ticks = elapsed_time / 1000;
 // XXX compiler should terminate above line fast. Probably worth helping it with
 //     if (elapsed_time < 1000) { elapsed_ticks = 0; }
 //     else if (elapsed_time < 2000) { elapsed_ticks = 1; }
 //     else { elapsed_ticks = elapsed_ticks / 1000; }
 // or even do the division totally manually?
 //     remainder = elapsed_time;
 //     elapsed_ticks = 0;
 //     while (elapsed_time >= 1000) {
 //         elapsed_ticks++; remainder -= 1000;
 //     }
 // Or hybrid - do the loop if elapsed_time < 65536? (Which would be always if 16-bit)
 remainder = elapsed_time % 1000;
 // XXX compiler should optimise the above line when immediately following `/ 1000`
 // if it doesn't, or not immediately following `/ 1000`  use this instead:
 //     remainder = elapsed_time - elapsed_ticks * 1000;
 // could special-case remainder inside the ifs suggested above
 return prev_time += elapsed_ticks;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As Kevin suggestion, HAL_GetTick API source has optimized to achieve better performance on all profile and test results on one-time execution as below

Ticker API HAL_GetTick without optimization on bare metal profile with small C lib HAL_GetTick with optimization on bare metal profile with small C lib
clear_interrupt 51us 5us
disable_interrupt 51us 5us

#define ADD_EXTRA_DELTA_FUNC_EXEC_TIME_US 25
#else
#define ADD_EXTRA_DELTA_FUNC_EXEC_TIME_US 0
#endif
#define NUM_OF_CALLS 100

#define NUM_OF_CYCLES 100000
Expand Down Expand Up @@ -452,6 +457,7 @@ void ticker_increment_test(void)
}

/* Test that common ticker functions complete with the required amount of time. */
template<int extra_latency>
void ticker_speed_test(void)
{
int counter = NUM_OF_CALLS;
Expand All @@ -477,7 +483,7 @@ void ticker_speed_test(void)
}
stop = us_ticker_read();

TEST_ASSERT(diff_us(start, stop, us_ticker_info) < (NUM_OF_CALLS * (MAX_FUNC_EXEC_TIME_US + DELTA_FUNC_EXEC_TIME_US)));
TEST_ASSERT(diff_us(start, stop, us_ticker_info) < (NUM_OF_CALLS * (MAX_FUNC_EXEC_TIME_US + DELTA_FUNC_EXEC_TIME_US + extra_latency)));

/* ---- Test ticker_set_interrupt function. ---- */
counter = NUM_OF_CALLS;
Expand Down Expand Up @@ -510,7 +516,7 @@ void ticker_speed_test(void)
}
stop = us_ticker_read();

TEST_ASSERT(diff_us(start, stop, us_ticker_info) < (NUM_OF_CALLS * (MAX_FUNC_EXEC_TIME_US + DELTA_FUNC_EXEC_TIME_US)));
TEST_ASSERT(diff_us(start, stop, us_ticker_info) < (NUM_OF_CALLS * (MAX_FUNC_EXEC_TIME_US + DELTA_FUNC_EXEC_TIME_US + extra_latency)));

}

Expand Down Expand Up @@ -611,7 +617,7 @@ Case cases[] = {
Case("Microsecond ticker fire interrupt", us_ticker_setup, ticker_fire_now_test, us_ticker_teardown),
Case("Microsecond ticker overflow test", us_ticker_setup, ticker_overflow_test, us_ticker_teardown),
Case("Microsecond ticker increment test", us_ticker_setup, ticker_increment_test, us_ticker_teardown),
Case("Microsecond ticker speed test", us_ticker_setup, ticker_speed_test, us_ticker_teardown),
Case("Microsecond ticker speed test", us_ticker_setup, ticker_speed_test<0>, us_ticker_teardown),
#if DEVICE_LPTICKER
Case("lp ticker init is safe to call repeatedly", lp_ticker_setup, ticker_init_test, lp_ticker_teardown),
Case("lp ticker info test", lp_ticker_setup, ticker_info_test, lp_ticker_teardown),
Expand All @@ -621,7 +627,7 @@ Case cases[] = {
Case("lp ticker fire interrupt", lp_ticker_setup, ticker_fire_now_test, lp_ticker_teardown),
Case("lp ticker overflow test", lp_ticker_setup, ticker_overflow_test, lp_ticker_teardown),
Case("lp ticker increment test", lp_ticker_setup, ticker_increment_test, lp_ticker_teardown),
Case("lp ticker speed test", lp_ticker_setup, ticker_speed_test, lp_ticker_teardown),
Case("lp ticker speed test", lp_ticker_setup, ticker_speed_test<ADD_EXTRA_DELTA_FUNC_EXEC_TIME_US>, lp_ticker_teardown),
#endif
};

Expand Down