USTICKER can not be removed from compilation

[jeromecoutant]

Description

Discussion started here:
#9221 (comment)

@ARMmbed/mbed-os-psa

Issue request type

[ ] Question
[ ] Enhancement
[x] Bug

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-923

[0xc0170]

We shall fix this asap as its blocking PR #9221

cc @ARMmbed/mbed-os-hal @kjbracey-arm

[0xc0170]

Our Mbed Enabled defines ticker as a must: https://www.mbed.com/en/about-mbed/mbed-enabled/requirements/

ticker is considered as a basic functionality. If we have a valid use case, we might need to send a fix.

[mikisch81]

From the Musca PR:

When moving us_ticker.c into the NS target I get the following linker errors:

Error: L6218E: Undefined symbol us_ticker_clear_interrupt (referred from BUILD/ARM_MUSCA_A1_S/ARMC6/mbed-os/hal/mbed_us_ticker_api.o).
Error: L6218E: Undefined symbol us_ticker_disable_interrupt (referred from BUILD/ARM_MUSCA_A1_S/ARMC6/mbed-os/hal/mbed_us_ticker_api.o).
Error: L6218E: Undefined symbol us_ticker_fire_interrupt (referred from BUILD/ARM_MUSCA_A1_S/ARMC6/mbed-os/hal/mbed_us_ticker_api.o).
Error: L6218E: Undefined symbol us_ticker_free (referred from BUILD/ARM_MUSCA_A1_S/ARMC6/mbed-os/hal/mbed_us_ticker_api.o).
Error: L6218E: Undefined symbol us_ticker_init (referred from BUILD/ARM_MUSCA_A1_S/ARMC6/mbed-os/hal/mbed_us_ticker_api.o).
Error: L6218E: Undefined symbol us_ticker_read (referred from BUILD/ARM_MUSCA_A1_S/ARMC6/mbed-os/hal/mbed_us_ticker_api.o).
Error: L6218E: Undefined symbol us_ticker_set_interrupt (referred from BUILD/ARM_MUSCA_A1_S/ARMC6/mbed-os/hal/mbed_us_ticker_api.o).
Finished: 0 information, 2 warning and 7 error messages.

When looking into mbed_us_ticker_api.c I see that there is a static struct of function pointers - us_interface which the linker will not throw, therefor even that the secure target does not use us_ticker at all, it needs to resolve those symbols - hence the empty implementation.
I agree this is not the right behaviour, but eventhough the target is using the bare-metal prfofile to build, hal folder is still built entirely.
@alzix @donatieng @theotherjimmy @bridadan @jeromecoutant

[mikisch81]

It appears the problem is that mbed_die() implementation calls wait_us() which calls get_us_ticker_data() which is defined in mbed_us_ticker_api.c.
So just guarding mbed_us_ticker_api.c with #if DEVICE_USTICKER is not enough.

[mikisch81]

@orenc17 @alzix

[kjbracey]

The fact that DEVICE_USTICKER exists suggests you might be allowed to not provide it, but as far as I'm aware it is assumed that it must exist in various places, and we've not coded to code with a platform without it. DEVICE_LPTICKER is certainly optional.

As a quick workaround you could conditionalise wait_us to have a fallback to the lp ticker if DEVICE_USTICKER is not defined, I guess.

[mikisch81]

And if lp ticker is not defined as well?

[kjbracey]

So this is the secure side or something - super limited environment, so no timers at all?

In that case you'll could have a condition in mbed_die to avoid the blinky LED that uses the timing. Or maybe we could have wait_us fallback to wait_ns, which is in a pending PR now and uses a software loop, calculating iterations based on SystemCoreClock.

[0xc0170]

In that case you'll could have a condition in mbed_die to avoid the blinky LED that uses the timing. Or maybe we could have wait_us fallback to wait_ns, which is in a pending PR now and uses a software loop, calculating iterations based on SystemCoreClock.

Was considering this, the units used now in mbed_die would be OK for wait_ns ?

[mikisch81]

When is #9812 expected to be merged? 5.12.0 as well?

[kjbracey]

Conceivably you could unconditionally use wait_ns I guess, to avoid pulling in the us_ticker code, even if available. The timing isn't at all critical.

wait_ns can overflow its maths though - it's documented as "don't try passing more than 1000000". And we can't realistically make it use wider maths or the overhead becomes infeasible for something that's supposed to be a "nanoseconds" delay. The test case for wait_ns breaks it up by calling wait_ns(1000000) once for each millisecond.

[mikisch81]

@kjbracey-arm I saw that mbed_die() uses wait_us(400000), which means it should be wait_ns(400000000).
But you said that we should pass bigger than 1000000, so what can we do then?

[kjbracey]

Call wait_ns(1000000) 400 times.

[mikisch81]

@kjbracey-arm Maybe change the wait_us() implementation?

void wait_us(int us)
{
#if DEVICE_USTICKER
    const ticker_data_t *const ticker = get_us_ticker_data();
    uint32_t start = ticker_read(ticker);
    while ((ticker_read(ticker) - start) < (uint32_t)us);
#else
    uint32_t iterations = us / 1000;
    uint32_t last_ns = us % 1000;
    for (uint32_t i = 0; i < iterations; ++i) {
        wait_ns(1000000);
    }
    if (last_ns > 0) {
        wait_ns(last_ns);
    }
#endif
}

[kjbracey]

Yes, that would make sense. I would save the division by just doing

while (us > 1000) {
    us -= 1000;
    wait_ns(1000000);
}
if (us > 0) {
    wait_ns(us * 1000);
}

Still, accuracy of wait_us will be greatly diminished like that. But I guess you don't expect timing accuracy on a platform with no timers?

[mikisch81]

Still, accuracy of wait_us will be greatly diminished like that. But I guess you don't expect timing accuracy > on a platform with no timers?

Exactly.. therefor the ifdef #DEVICE_USTICKER

[kjbracey]

My concern is what is the caller of wait_us expecting? Do they know they're on a platform with no DEVICE_USTICKER? We're kind of breaking a contract here for the application API. I'm still not 100% sure what the use case here.

[mikisch81]

The use-case are constrained devices (in our case the secure part of a PSA target).

I am completely fine with putting this logic in mbed_die() implementation.

[kjbracey]

Mind you, we do have existing examples of tolerating poor timing. There's a config option for STM targets that makes the lpticker be based on some circuit that has maybe 20% error rather than a proper 32.768kHz crystal...

[kjbracey]

I'm fine putting this in wait_us as long as we aren't calling any target with DEVICE_USTICKER off a suitable target for general-purpose user applications. If it's only happening for the secure side of PSA, that's fine.

[mikisch81]

I still get errors building a secure target without USTICKER, now Timer.cpp is failing):

Error: L6218E: Undefined symbol get_us_ticker_data (referred from BUILD/ARM_MUSCA_A1_S/ARMC6/mbed-os/drivers/Timer.o).