Time parameters in equeue.h are still using the int millisecond style

[adbridge]

Description of defect

While checking the Events library I found that times used in functions for equeue are expressed as integer milliseconds. Should these not be in Chrono durations to be consistent with events ?

Target(s) affected by this defect ?

All

Toolchain(s) (name and version) displaying this defect ?

All

What version of Mbed-os are you using (tag or sha) ?

mbed-os-6.5.0

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

n/a

How is this defect reproduced ?

n/a

[adbridge]

@kjbracey-arm was this missed from the Chrono update or is there another reason why this is still using ints ?

[ciarmcom]

Thank you for raising this detailed GitHub issue. I am now notifying our internal issue triagers.
Internal Jira reference: https://jira.arm.com/browse/IOTOSM-2881

[kjbracey]

equeue is a C library with a C API, so can't use Chrono.

I'm not sure if it's viewed as part of the Mbed OS API proper. The Event and EventQueue classes are our C++ view of it with the Chrono stuff.

I guess it's similar to our Semaphore versus osSemaphoreAcquire or whatever. Or even HAL C functions like us_ticker_read.

(Okay, you could still give equeue.h extra Chrono overloads inside #ifdef __cplusplus - something I've done for some C/C++ things - but I don't think we want apps to be using that layer anyway. They should use EventQueue).

[kjbracey]

Mind you, there are some C++ APIs that have not been updated. Quite a few classes like cellular or socket things have 1 or 2 "timeout" settings that still take int milliseconds.

And those do cause friction due to the need to switch between Chrono and non-Chrono style.

[adbridge]

I think the way to fix this properly (as I think equeue itself is internal) is to update Eventqueue.dispatch() to move to using a Chrono parameter so that the user facing APIs are consistent and then inside the dispatch implementation do the conversion from Chrono to int milliseconds ?

[kjbracey]

Right, EventQueue::dispatch needs sorting at a minimum to use Chrono. I've actually been looking at that method in a (pre-Chrono) customer branch, as it (and equeue_dispatch) has a problematic API, impeding optimisation by pulling in a whole bunch of blocking code that is never used in that project, as that project only uses dispatch(0).

There are 3 distinct modes - dispatch(0) (non-blocking), dispatch(-1) (blocking forever) and dispatch(n) (blocking for a while).

To match other APIs and allow code separation, they should be distinct functions, maybe dispatch_current(), dispatch_forever() and dispatch_for(duration), and I guess add dispatch_until(time_point).

Even if that split doesn't (yet) occur in equeue.c itself, a revised C++ API can open the door to that in future. (In my customer branch, I added a config option that disables anything other than equeue_dispatch(0), for code size, but that can't be checked at compile-time without a split API).

There are a few APIs that take Chrono and accept magic (or negative) values to mean forever, mainly for internal use in glue code, but I'd rather separate them in public APIs.

[adbridge]

Yes I was just trying to work out how we could simulate the dispatch forever (currently initiated by the default value being passed into equeue as -1 . I couldn't see how we could do the same thing with a Chrono time so I think you're right , we need separate wrappers into queue .

We already have :

void dispatch_forever()
    {
        dispatch();
    }

Where the default for dispatch is -1. So I think we need to fix that to make dispatch_forever() directly call into equeue with the -1 value. That allows us to change the default dispatch to use a Chrono time...

[adbridge]

So I assume dispatch with a value of 0, will only dispatch any events that have already been queued and then will terminate ?

[kjbracey]

-1 . I couldn't see how we could do the same thing with a Chrono time

You can do it, but I'd rather not.

dispatch(duration d = duration{-1})

dispatch_forever already exists - it seems it was added after the original "just pass -1 as default parameter" concept, from the comment.

So I assume dispatch with a value of 0, will only dispatch any events that have already been queued and then will terminate ?

Yes.

And from a code exclusion point of view, if you know at compile time that you're only ever doing that, all the code in the dispatcher to wait for an event to be queued isn't needed - only dispatch_forever or dispatch_for would have to wait.

With the current set-up, that's a run-time test for a parameter value, no linker exclusion possible.

[adbridge]

So maybe a separate function wrapper for that would be better called dispatch_now() rather than a dispatch_current() ?

[kjbracey]

I haven't thought of a good name for that one.

The pattern for other calls is usually xxx, try_xxx, try_xxx_for and try_xxx_until, but they're doing (or attempting) one operation, and the try versions indicate success or not.

That "try" pattern doesn't really work for this "do as many as you can" without any return value.

ConditionVariable has "wait", "wait_for" and "wait_until", but that doesn't give us an answer for the immediate...

I don't think dispatch_now or dispatch_current are doing it for me. Any third-party suggestions, anyone?

[kjbracey]

Something about "nonblocking" might be key - it is the call that's safe to use in some sort of other (nested) event loop or preemption context, where you should be doing what you have, and then return without waiting.

But "dispatch_nonblocking()" itself sounds clunky.