Skip to content

vehicle_time: Deliver VehicleClock callbacks from a worker thread - #215

Open
florianfueller wants to merge 5 commits into
eclipse-score:mainfrom
florianfueller:feature/59-vehicle-clock-callback-delivery
Open

vehicle_time: Deliver VehicleClock callbacks from a worker thread#215
florianfueller wants to merge 5 commits into
eclipse-score:mainfrom
florianfueller:feature/59-vehicle-clock-callback-delivery

Conversation

@florianfueller

@florianfueller florianfueller commented Sep 8, 2026 •
edited
Loading

Copy link
Copy Markdown

The Set/Unset callback methods of VehicleClockBackendImpl were no-ops.
Callbacks are now delivered by a worker thread that polls the
TimeDaemon shared memory every 50 ms:

  • The thread starts with the first registered callback after Init()
    and runs until the backend is destroyed. While no callback is
    registered it sleeps.
  • A callback receives the first snapshot polled after its registration
    and afterwards only changes of its part of the snapshot:
    TimeSlaveSyncData and PDelayMeasurementData on any change,
    VehicleTimeStatus only when the flags change (rate deviation
    excluded).
  • Set/Unset are safe against in-flight invocations, also from inside a
    callback; Unset returns only after a running invocation finished.

Add unit tests, update the docs and remove the "not yet delivered"
warnings.

closes #59 (improvement ticket)

@github-project-automation github-project-automation Bot moved this to In Progress in TIM - Time FT Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run //:license-check

Status: ⚠️ Needs Review

Click to expand output 
[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.6.0) and connecting to it...
INFO: Invocation ID: cf66999e-039b-484b-af55-17288360d68b
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
WARNING: For repository 'platforms', the root module requires module version platforms@1.0.0, but got platforms@1.1.0 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'score_platform', the root module requires module version score_platform@0.7.1, but got score_platform@0.7.2 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'rules_oci', the root module requires module version rules_oci@2.2.7, but got rules_oci@2.3.0 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 3 packages loaded
WARNING: Target pattern parsing failed.
ERROR: Skipping '//:license-check': no such target '//:license-check': target 'license-check' not declared in package '' defined by /home/runner/work/time/time/BUILD
ERROR: no such target '//:license-check': target 'license-check' not declared in package '' defined by /home/runner/work/time/time/BUILD
INFO: Elapsed time: 17.264s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

@florianfueller
florianfueller force-pushed the feature/59-vehicle-clock-callback-delivery branch from d4f4e67 to 143d1fb Compare September 9, 2026 07:57
@florianfueller
florianfueller force-pushed the feature/59-vehicle-clock-callback-delivery branch from 143d1fb to d447876 Compare September 9, 2026 11:18
@florianfueller
florianfueller force-pushed the feature/59-vehicle-clock-callback-delivery branch from d447876 to a816147 Compare September 9, 2026 12:37
@florianfueller
florianfueller marked this pull request as ready for review September 9, 2026 12:38
@florianfueller
florianfueller force-pushed the feature/59-vehicle-clock-callback-delivery branch from c811e27 to 041c9b8 Compare September 23, 2026 06:56
@florianfueller
florianfueller force-pushed the feature/59-vehicle-clock-callback-delivery branch from 041c9b8 to 4448f49 Compare September 23, 2026 07:42
@florianfueller
florianfueller force-pushed the feature/59-vehicle-clock-callback-delivery branch from 4448f49 to 756eaad Compare September 23, 2026 08:06
@florianfueller
florianfueller force-pushed the feature/59-vehicle-clock-callback-delivery branch from 756eaad to 28b62ff Compare September 23, 2026 08:32
@florianfueller
florianfueller force-pushed the feature/59-vehicle-clock-callback-delivery branch from e5731ec to 150c89a Compare October 7, 2026 13:03
lavrovvalera
lavrovvalera reviewed Oct 7, 2026
View reviewed changes
Comment thread score/time/vehicle_time/src/details/td_impl/svt_callback_dispatcher.cpp Outdated
const auto status_flags = ConvertPtpStatus(snapshot.value().status);
score::cpp::ignore =
status_slot_.InvokeIfChanged(status_flags, VehicleTimeStatus{status_flags, snapshot.value().rate_deviation});
score::cpp::ignore = sync_data_slot_.TryDeliverChangedData(ConvertSyncData(snapshot.value().sync_fup_data));

@lavrovvalera lavrovvalera Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we need still to ignore the result?
regarding the returning result.
I think, the point, the callback was executed or nto is indeed not interesting.
but if the callback will return some result and we can react on it - that might be interesting.
but do we need this thing?
one of the use case - the callback could return if the registration shall still be hold.
so, if it will return true - continue calling it, false - drop the subscription.

if not, I would propose to drop the return at all.

@florianfueller florianfueller Oct 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Return dropped.

Comment thread score/time/vehicle_time/src/details/td_impl/svt_callback_dispatcher.cpp

void SvtCallbackDispatcher::PollAndDispatch() noexcept
{
const auto snapshot = svt_receiver_->Receive();

@lavrovvalera lavrovvalera Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

then we need to investigate before we will merge it

Comment thread score/time/vehicle_time/src/details/td_impl/svt_callback_wrapper.h Outdated
template <typename Timebase>
bool IsSameForDelivery(const TimeSlaveSyncData<Timebase>& first, const TimeSlaveSyncData<Timebase>& second) noexcept
{
const bool same_precise_origin_timestamp = (first.precise_origin_timestamp == second.precise_origin_timestamp);

@lavrovvalera lavrovvalera Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why the default instantiation doesn't fit here?

@florianfueller florianfueller Oct 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Implemented operator==

Comment thread score/time/vehicle_time/src/details/td_impl/svt_callback_wrapper.h Outdated
bool IsSameForDelivery(const PDelayMeasurementData<Timebase>& first,
const PDelayMeasurementData<Timebase>& second) noexcept
{
const bool same_request_origin_timestamp = (first.request_origin_timestamp == second.request_origin_timestamp);

@lavrovvalera lavrovvalera Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why the default instantiation doesn't fit here?

@florianfueller florianfueller Oct 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Implemented operator==

Comment thread score/time/vehicle_time/src/details/td_impl/svt_callback_wrapper.h Outdated
/// @brief Thread-safe holder for a single move-only callback that is invoked from a dedicated worker thread
/// @brief Delivery comparison for types that already provide @c operator==.
template <typename Value>
bool IsSameForDelivery(const Value& first, const Value& second) noexcept

@lavrovvalera lavrovvalera Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure, I can understand the function name

@florianfueller florianfueller Oct 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed to IsSame

Comment thread score/time/vehicle_time/src/details/td_impl/svt_callback_wrapper.h Outdated
/// @return @c true if the callback was invoked, @c false if none is installed or @p data is unchanged.
template <typename Argument>
bool InvokeIfChanged(const Data& data, const Argument& argument) noexcept
bool TryDeliverChangedData(const Data& data) noexcept

@lavrovvalera lavrovvalera Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it a delivery?
Maybe TryToEnvoke() or somehting?
also the user of this function doesn't know, what the "new" data is.
he jsut has the "data"

@florianfueller florianfueller Oct 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Renamed to TryToEnvoke

Comment thread score/time/vehicle_time/src/details/td_impl/svt_callback_wrapper.h Outdated
std::recursive_mutex mutex_;
std::shared_ptr<Callback> callback_{};
std::optional<Data> last_data_{};
// Mirrors callback_ != nullptr. Written only under mutex_, read lock-free by IsSet().

@lavrovvalera lavrovvalera Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think, we need this comment

@florianfueller florianfueller Oct 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed

@florianfueller
florianfueller force-pushed the feature/59-vehicle-clock-callback-delivery branch from 22c6f71 to 0eee42c Compare October 8, 2026 11:37
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Documentation preview for this pull request is available at:
pr-215: https://eclipse-score.github.io/time/pr-215/

This branch is waiting to be deployed

1 waiting deployment
workflow-approval — 26c4f7bb Waiting Oct 9, 2026 by florianfueller via qnx-build (x86_64-qnx) / approval #940
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lavrovvalera lavrovvalera Awaiting requested review from lavrovvalera

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

TIM - Time FT
Status: In Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improvement: Implement the callbacks envoking

2 participants

@florianfueller @lavrovvalera