C++ Chrono support #12425

kjbracey · 2020-02-13T11:54:08Z

Summary of changes

Add Chronos versions of time-related APIs to RTOS classes, Ticker-related classes and EventQueue.

Kernel::Clock, HighResClock, LowPowerClock and RealTimeClock act as the Chrono clocks. (C++ standard clocks system_clock, steady_clock and high_resolution_clock are not supported, due to difficulty in consistent retargetting across toolchains).

Deprecate old non-Chrono APIs.

Impact of changes

Existing timing code will generate deprecation warnings, but there should be no functional change.

A follow-up PR will clean-up warnings in rest of code base.

Migration actions required

Users should migrate to use Chrono-based APIs as directed by the deprecation messages

Documentation

Design document included.

Basic document updates: ARMmbed/mbed-os-5-docs#1302

Basic example updates: ARMmbed/mbed-os-examples-docs_only#112

Design document needs to be reworked into a docs section.

General Chrono background:

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

Reviewers

@pan-, @bulislaw

ciarmcom · 2020-02-13T12:00:39Z

@kjbracey-arm, thank you for your changes.
@bulislaw @pan- @ARMmbed/mbed-os-core @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-test @ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2020-02-13T13:03:48Z

travis-ci/doxy-spellcheck

Our doxy needs a fix (chrono word), please review

ladislas · 2020-02-13T16:39:55Z

If I understand correctly, the goal is to move away from 5000 to 5ms?

Is that what the deprecation message is for? Can't we have both at the same time and not deprecation?

kjbracey · 2020-02-14T08:33:59Z

Can't we have both at the same time and not deprecation?

The deprecation does give you both at the same time, for a while.

The ultimate aim is to prevent confusion about units and resolution, and avoid potential errors like that fixed in #12401 (passing milliseconds to a seconds parameter).

So ultimately the non-Chrono forms will be removed to force safety. If the int forms remain, that allows you to accidentally type 5 instead of 5s and be off by a thousand again.

It's not hard to adapt existing non-Chrono code at the bottom layer with a simple cast-like construct:

   void my_func(int timeout_ms)
   {
        ...
        sem->try_acquire_for(std::chrono::milliseconds(timeout_ms));
   }

Although it's ultimately better to convert the entire calltree. (That is as potentially unsafe as any cast - if the int parameter wasn't actually milliseconds, it's a potential error point. Period typing should be extended throughout as much of the code as possible).

ladislas · 2020-02-14T10:03:57Z

thanks @kjbracey-arm! that makes perfect sense :)
We'll update our code base as soon as this is released!

bulislaw · 2020-02-21T15:45:50Z

drivers/Ticker.h

@@ -90,7 +93,8 @@ class Ticker : public TimerEvent, private NonCopyable<Ticker> {
 #endif
    void attach(F &&func, float t)


Shoulnd't that be deprecated?

bulislaw · 2020-02-21T15:54:25Z

drivers/Ticker.h

+     *  for threads scheduling.
+     *
+     */
+    void attach(Callback<void()> func, TickerClock::duration t);


why are we hiding this behind TickerClock::duration, wouldn't it be more readable as std::chrono::microseconds?

This is possibly the most important design question. It applies both to TickerClock::duration and Kernel::Clock::duration[_u32].

Writing it like this potentially leaves open the door for changing what the API frequency is. I can envisage doing that. For the ticker, it could be done to matching hardware frequency, which would reduce run-time conversions; for the OS it might be part of changing the tick speed for power reasons, and then minimising conversions at run-time again.

Actually doing such a change would not be smooth though - for example if the tick rate was lowered to 100Hz, then you can't just change the period, as

ThisThread::sleep_for(100ms);

would fail to compile, as you can't implicitly assign milliseconds to centiseconds, even if an exact constant like that.

You would need to probably need to change the APIs to be template form to handle rounding if you did that.

We did previously rule that the RTOS tick rate was fixed at milliseconds, and documented that. This is something of a step back from that.

And it is just to provide scope to minimise run-time conversions. We could alternatively lock these APIs and clocks as Chrono milliseconds and microseconds, with commitment to never change (regardless of actual RTOS tick rate or hardware rate), and accept run-time conversion cost.

Oh, there is also a separate symmetry issue, in that time points must be expressed as TickerClock::time_point. So whether or not TickerClock::duration is fixed, writing it as TickerClock::duration just looks neat next to the time_point, and makes explicit that this matches the value you'd get by, say, subtracting two TickerClock::now values. No conversion needed.

That makes sense, thanks.

drivers/Ticker.h

bulislaw · 2020-02-21T16:00:54Z

drivers/TickerClock.h

+ * operators for `->` and conversion to `ticker_data_t *` are provided allowing
+ * TickerClock to be easily substituted in place of a `ticker_data_t *`.
+ */
+struct TickerClock {


I noticed you are using struct instead of class. I know it's ok, but are you following some guidelines/convention or just because?

It has no semantic meaning at all, apart from the minor detail of what the default member/base visibility is. You can use it just for that, just to save typing public:.

But in my head, I tend to feel happy calling "non-owning" copyable things "structs". "Class" has a vague suggestion of a deeper structure.

I think I apply a similar thing for class versus typename in templates. "Class" usually hints at something deep - this is likely a fancy object type, "typename" suggests it's likely a primitive type.

But not sure that it's worth having a hard and fast rule here.

I thought it's worth seeing others' view. The C++ core guidelines disagree and say

C.8: Use class rather than struct if any member is non-public
Reason
Readability. To make it clear that something is being hidden/abstracted. This is a useful convention.

Fluent C++ has a good take on this as well:

https://www.fluentcpp.com/2017/06/13/the-real-difference-between-struct-class/

Quoting:

The C++ Core Guidelines
The above has been inspired by the C++ Core Guideline (which is a great read by the way), in particular the following:

C.1: Organize related data into structures (structs or classes)
C.2: Use class if the class has an invariant; use struct if the data members can vary independently
C.3: Represent the distinction between an interface and an implementation using a class
C.8: Use class rather than struct if any member is non-public

I like the idea of C.2 and of struct equivalents to bundle, whereas a class can do things.

Also doing a lot of Swift development, structs are value types whereas classes are reference types. Which tends to agree with C.2 --> use struct for things that can live independently.

I don't mind having a struct do things as well and not just store data.

It's worth noting that in this particular case, there is a private member, but it's actually not really hidden, as there are an implicit conversion and -> operators that give it straight to you.

This really supposed to just be an enhanced ticker_data * that is usable as that, but also adds the Clock API.

A more simple clock is actually a bit odd anyway, as they usually have no members, and only typedefs and a static now function. So they technically don't hit C.8, but then they're not a "bundle" either, they're just an interface (to now).

bulislaw · 2020-02-21T16:06:10Z

drivers/Timer.h

    operator float();

    /** Get in a high resolution type the time passed in microseconds.
     *  Returns a 64 bit integer.
     */
    us_timestamp_t read_high_resolution_us();

+    /** Get in a high resolution type the time passed in microseconds.


What's the design choice behind leaving read_us and read_ms?

It's me being soft. I am inclined to remove them, but that forces people to understand how to do duration_cast<milliseconds>(t.read_duration()).count() for what is probably a fairly common operation (at least in the sort of glue code where this is used to provide a "HAL" for something using plain ints).

I only went half-way and removed the float form, as I didn't want floats to be used casually.

If you're okay with removing read_us and read_ms, I am too.

I think it's fair to get rid of it now. I don't really like the read_duration I'd really like to have it just read especially that we are deprecating the float read(). I know it's against our own rules but for the sake of clarity and cleanliness maybe it's worth just replacing them?

I'm also prefer to have something short like read. read is generic, so I'm not a huge fan. Read what? Is there something better and short? It's not now, because it's not a time_point.

Maybe elapsed_time? ~~That is what time_points have to return their duration~~. This isn't a time point, with an elapsed real-time since an epoch, but it does measure elapsed time like a stop-watch (with start/stop and reset functions). Still a bit long though.

Correction - time_points call theirs time_since_epoch. The equivalent for this would be elapsed_time.

bulislaw · 2020-02-21T16:13:49Z

drivers/Timer.h

@@ -92,21 +97,27 @@ class Timer : private NonCopyable<Timer> {

    /** An operator shorthand for read()
     */
+    MBED_DEPRECATED_SINCE("mbed-os-6.0.0", "Floating point operators should normally be avoided for code size. If really needed, you can use `duration<float>(read_duration()).count()`")
    operator float();

    /** Get in a high resolution type the time passed in microseconds.


From a design point of view, what's the purpose of this function? It's the same as read_us but the return type.

Well, it's the strongly-typed Chrono return. You need it when using Chrono APIs. The question is should we deprecate read_us? See other comment.

bulislaw · 2020-02-21T16:26:21Z

platform/source/SysTimer.cpp

@@ -29,6 +29,10 @@ extern "C" {
 #endif
 }

+using namespace std::chrono;
+
+const milliseconds deep_sleep_latency{MBED_CONF_TARGET_DEEP_SLEEP_LATENCY};


For my information: Any particular reason that's not constexpr?

No, no particular reason. It should be.

You don't have to put constexpr instead of const on variables, as const-qualified variables with constant initialisers are permitted in constant expressions, and hence usable in constexpr context.

This retains compatibility with the traditional C++ extension over C that const int x = 4 can be used in constant expressions.

This is different to function calls, which can never be used in constant expressions if they're not marked constexpr.

But it's good to stick the constexpr on the variable as it forces an immediate check that deep_sleep_latency is usable in a constant expression - you'd get a compile error if milliseconds' constructor wasn't constexpr.

bulislaw · 2020-02-21T16:27:33Z

platform/source/mbed_os_timer.h

@@ -29,32 +30,65 @@ namespace mbed {
 namespace internal {

 #if MBED_CONF_RTOS_PRESENT
-#define OS_TICK_US (1000000 / OS_TICK_FREQ)
+typedef SysTimer<std::ratio<1, OS_TICK_FREQ>> OsTimer;


Are you following some kind of standard for using vs typedef?

No. Original code used typedef, and I didn't change it.

I'm quite happy changing to using - my only niggling reservation is that Eclipse's outline view doesn't recognise it. (https://bugs.eclipse.org/bugs/show_bug.cgi?id=509120) Parser and auto-complete recognises it fine, but just is annoyingly absent from the file/class outline, unlike the (T) you see for a typedef.

bulislaw

Kevin thanks for this awesome change! I started reviewing it, but I quickly realised that it's missing some context. I think we need to have a design document around all the different tickers and clocks and timers in the system: systimer, ostimer, osclock, clock, tickerclock ...more?. I'm personally lost at this point and to figure this out will take me some time and I'm sure other reviewers will be in the same situation. Not to mention people that will like to understand it in the future. I'll keep going through the review, but it would be great if you could create a document describing the clock/timer situation in Mbed especially in context of this changes.

bulislaw · 2020-02-21T17:49:14Z

I had a quick check of how much all this will cost us in terms of memory and it seems it's true to its word :) barely any difference!

kjbracey · 2020-02-24T11:18:04Z

I think we need to have a design document around all the different tickers and clocks and timers in the system: systimer, ostimer, osclock, clock, tickerclock ...more?

Doing this certainly helped me figure it out. It's actually not as complicated as I thought :)

There are a grand total of 3 clocks, and just different ways of working with them. Will put something together.

barely any difference!

Ideally it would be no difference, but little things creep in. Like maybe something ending up 64-bit (possibly rightly so) when it was previously 32-bit.

Biggest annoyance is that ARM EABI won't return structures in registers, so a duration- or time_point-returning function works as if it was void now(uint64_t *out), rather than returning in R0/R1 like uint64_t now(). The "no change to generated x86 assembly" slide from Hinnant's talk isn't true for ARM.

That cost is avoided by making the clocks' now methods be inline, so the private external function is actually just returning an integer.

In a couple of places I assumed that durations and time_points always zero-initialised. This is incorrect - they act as if they were their representation, so integer durations only zero-init when static. You can zero init (like integers) by having `{}` initialisation.

kjbracey · 2020-04-27T07:20:50Z

Evelyne's corrections integrated.

ladislas · 2020-04-27T12:00:48Z