Skip to content
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.

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

Consider making libcyphal::MonotonicClock meet the requirements of TrivialClock #399

Open
thirtytwobits opened this issue Nov 14, 2024 · 3 comments

Comments

@thirtytwobits
Copy link
Contributor

See https://en.cppreference.com/w/cpp/named_req/TrivialClock

This is a design change, not a bug.

Using chrono becomes tricky when implementing the platform layer because C++ chrono treats system time as a global resource. Furthermore, on some systems chrono clocks are implemented by lower-level (read -> C) clocks also used by the platform implementation. We may want to remove ITimeProvider in favor of simply designating a chrono clock all parts of libcyphal will use as a compile-time configuration (with defaults) allowing a given platform to ensure the same timebase by providing a custom chrono clock backed by the low-level implementation for said platform.

If libcyphal always uses a compile-time-configured clock then unit testing is just as easy to handle as a dummy clock can be configured for tests when control over time is needed.

There doesn't seem to be another other benefit to supporting polymorphism in our timekeeping mechanism, unless I'm missing something.

@serges147
Copy link
Collaborator

serges147 commented Nov 15, 2024

Scott, sorry but I don't get what is the problem currently. Could you please elaborate and give concrete example where currently implemented approach doesn't fit.

If libcyphal always uses a compile-time-configured clock then unit testing is just as easy to handle as a dummy clock...

In my numerous unit tests I have everywhere artificial control over time very easy (see VirtualTimeScheduler and its scheduleAt method), so I don't quite understand your unit testing concern either.

ITimeProvider interface and its now() method were introduced naturally (extracted from initial IExecutor) when it became obvious that there are presentation level entities (server and subscriber) which don't need whole executor functionality but only its time providing part.

@pavel-kirienko
Copy link
Member

As we discussed at the call, one way forward is to define our own MonotonicClock as described in the design doc which by default simply wraps std::steady_clock. There will be an ifdef configuration parameter that allows the user to provide their own libcyphal::MonotonicClock defined elsewhere; this could be done via a compile time macro that works like this:

#ifndef LIBCYPHAL_CUSTOM_CLOCK_HEADER
// the default implementation that proxies to std::steady_clock
#else
#   include LIBCYPHAL_CUSTOM_CLOCK_HEADER
#endif

@pavel-kirienko
Copy link
Member

An alternative is to define our own MonotonicClock that declares but does not define static MonotonicClock::now(), so that the user provides their own definition somewhere in any translation unit. This approach does not require preprocessor configs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants