Add GetLatestSlot and GetLatestSample functionality by muhseth · Pull Request #208 · eclipse-score/communication

muhseth · 2026-03-18T12:59:58Z

No description provided.

LittleHuba · 2026-03-24T09:27:10Z

@muhseth please fix conflicts.

crimson11 · 2026-03-26T09:32:33Z

@@ -38,6 +38,55 @@ EventDataControlCompositeImpl<AtomicIndirectorType>::EventDataControlCompositeIm
    CheckForValidDataControls();
 }

+template <template <class> class AtomicIndirectorType>
+score::cpp::optional<SlotIndexType> EventDataControlCompositeImpl<AtomicIndirectorType>::FindLatestReadableSlotIndex(


You should instead change existing EventDataControlCompositeImpl::GetLatestTimestamp()! You are mostly duplicating code here. The new/refactored function should be named GetLatestSlot and it should return the index PLUS the timestamp!

And please take into account, what I have written here: Ticket-253464

Done.
I have removed GetLatestTimeStamp function and corresponding upper layer code.
As I see it is not used any where.

crimson11 · 2026-03-26T09:43:35Z

+         slot_index < static_cast<SlotIndexType>(control.state_slots_.size());
+         ++slot_index)
+    {
+        SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD(static_cast<std::size_t>(slot_index) < control.state_slots_.size());


? Why this assert? I do not get it.

I have removed it. As I see it there is no needed of it.
I have kept it as it was the part of previous code.

crimson11 · 2026-04-08T13:04:22Z

@@ -39,6 +39,53 @@ EventDataControlComposite<AtomicIndirectorType>::EventDataControlComposite(Event
    CheckForValidDataControls();
 }

+template <template <class> class AtomicIndirectorType>


You have to be aware, that you have to align your changes with Brenbdans massive change here: #268

I.e. imho it makes sense, that you base your work on Brendans PR!

Already Done

crimson11 · 2026-04-08T13:40:13Z

@@ -39,6 +39,53 @@ EventDataControlComposite<AtomicIndirectorType>::EventDataControlComposite(Event
    CheckForValidDataControls();
 }

+template <template <class> class AtomicIndirectorType>
+score::cpp::optional<SlotIndexType> EventDataControlComposite<AtomicIndirectorType>::FindLatestReadableSlotIndex(


What does "readable" mean?
Just: FindNewestSlot ?

I think what Wahaj means is "FindLatestReferencedSlotIndex". I think it makes sense because just "newest" could also mean newest available slot to allocate in which isn't what we want.

Already Done

crimson11 · 2026-04-08T13:51:57Z

@@ -43,7 +40,7 @@ auto EventSlotStatus::GetTimeStamp() const noexcept -> EventTimeStamp

 auto EventSlotStatus::IsInvalid() const noexcept -> bool
 {
-    return data_ == INVALID_EVENT;
+    return data_ == static_cast<value_type>(InvalidTimestamp);


This cast looks "odd" to me!
Our EventSlotStatus::value_type is an uint64 ... the timestamp is a uint32!
An invalid event-slot status technically means:
The timestamp part is 0 (upper32 bit)
the ref-count part is 0 (lower 32 bit)
Just casting the timestamp part does miss this a bit ... if you want to be more "expressive" you could write something like:
constexpr EventSlotStatus::value_type INVALID_EVENT = (InvalidTimestamp <<32) | ZeroRefCount)

but this is "overdone" -> you could simply document INVALID_EVENT = 0u means an invalid timestamp and zero refcount ...

i have made the changes as suggested.

crimson11 · 2026-04-08T16:47:17Z

@@ -58,7 +55,7 @@ auto EventSlotStatus::MarkInWriting() noexcept -> void

 auto EventSlotStatus::MarkInvalid() noexcept -> void
 {
-    data_ = INVALID_EVENT;
+    data_ = static_cast<value_type>(InvalidTimestamp);


crimson11 · 2026-04-08T17:07:10Z

@@ -251,6 +250,38 @@ Result<impl::SampleAllocateePtr<SampleType>> SkeletonEvent<SampleType>::Allocate
    }
 }

+template <typename SampleType>
+Result<SampleType> SkeletonEvent<SampleType>::GetLatestSample() noexcept


Ouch. Why are you returning the sample by value here??? We never do this. A sample can be several megabytes in size ... so why are you copying it here - all our zero-copy architecture goes through the roof then ... and applications may crash, because the value will end up on stack, which will overflow ...

I guess you will use/need this functionality, when implementing the Skeleton-side get() method? Or even/also the set-method, where you need to work on the latest existing sample ...?

But then you need to apply SamplePtr logic!
I.e. you refcount the latest sample ... and create a SamplPtr from it, which you return from here ...
At your caller-side (your get/set impl) you then copy the data from the SamplePtr to your method ReturnType/InArg and once you are done with the copying, you destroy the SamplPtr freeing the slot.

This way you have just ONE copy! With your approach taken here, you copy TWICE! You copy out from shm to your sampl variable and then you copy from that variable back intro the call-queue! And this already expects, that you have RVO (which I guess is mandatory right now) ... but again - your caller side will crash, because the returned value (with RVO) will land most likely on stack and might explode there (if it is large).

crimson11 · 2026-04-08T17:24:18Z

+    {
+        const auto sample = event_data_storage_->at(static_cast<std::uint64_t>(slot.GetIndex()));
+
+        if (slot.IsValidQM())


OK - so here you are doing "manually", what should be the job of the SamplePtr dtor

bemerybmw · 2026-07-02T13:17:54Z

@@ -236,7 +236,7 @@ template <template <class> class AtomicIndirectorType>
 // coverity[autosar_cpp14_a15_5_3_violation : FALSE]
 EventSlotStatus::EventTimeStamp EventDataControlComposite<AtomicIndirectorType>::GetLatestTimestamp() const noexcept
 {
-    EventSlotStatus::EventTimeStamp latest_time_stamp{1U};
+    EventSlotStatus::EventTimeStamp latest_time_stamp{EventSlotStatus::UNINITIALIZED_TIMESTAMP};


…ctionLog

All comments were addressed.

muhseth requested review from LittleHuba, bemerybmw, castler, crimson11, hoe-jo and limdor as code owners March 18, 2026 12:59

castler changed the title ~~Muse skeleton get~~ Add GetLatestSlot and GetLatestSample functionality Mar 19, 2026

LittleHuba marked this pull request as draft March 24, 2026 09:27

crimson11 requested changes Mar 26, 2026

View reviewed changes

muhseth force-pushed the muse_skeleton_get branch 3 times, most recently from c4ffb3a to f0c0b32 Compare April 2, 2026 09:58

muhseth marked this pull request as ready for review April 2, 2026 14:14

muhseth requested a review from crimson11 April 7, 2026 07:44

crimson11 previously requested changes Apr 8, 2026

View reviewed changes

bemerybmw requested changes Apr 9, 2026

View reviewed changes

muhseth force-pushed the muse_skeleton_get branch from f0c0b32 to a65dc2c Compare April 16, 2026 14:43

muhseth marked this pull request as draft April 16, 2026 14:44

muhseth force-pushed the muse_skeleton_get branch from a65dc2c to 636762e Compare April 16, 2026 15:24

bemerybmw reviewed Apr 17, 2026

View reviewed changes

Comment thread score/mw/com/impl/bindings/lola/skeleton_event_common.h Outdated

bemerybmw reviewed Apr 17, 2026

View reviewed changes

Comment thread score/mw/com/impl/bindings/lola/skeleton_event_common.h

bemerybmw reviewed Apr 17, 2026

View reviewed changes

Comment thread score/mw/com/impl/bindings/lola/skeleton_event_common.cpp Outdated

muhseth force-pushed the muse_skeleton_get branch 2 times, most recently from 85d878a to 4f7e4d4 Compare May 5, 2026 12:40

Tejveerpratap2803 force-pushed the muse_skeleton_get branch 3 times, most recently from 7dbf113 to e29d097 Compare May 25, 2026 09:09

Tejveerpratap2803 requested review from bemerybmw and crimson11 May 26, 2026 11:57

bemerybmw requested changes Jul 2, 2026

View reviewed changes

bemerybmw self-assigned this Jul 6, 2026

Tejveerpratap2803 force-pushed the muse_skeleton_get branch 4 times, most recently from 6b35406 to 0c33ec5 Compare July 8, 2026 07:11

Tejveerpratap2803 requested a review from bemerybmw July 8, 2026 08:19

bemerybmw requested changes Jul 9, 2026

View reviewed changes

Tejveerpratap2803 requested a review from bemerybmw July 14, 2026 05:39

bemerybmw requested changes Jul 14, 2026

View reviewed changes

Tejveerpratap2803 requested a review from bemerybmw July 16, 2026 07:28

Tejveerpratap2803 force-pushed the muse_skeleton_get branch 3 times, most recently from 4f9d931 to ba5411b Compare July 16, 2026 11:55

Add GetLatestSample in LoLa SkeletonEvent with field_getter_enabled

e2ff15f

Tejveerpratap2803 force-pushed the muse_skeleton_get branch from ba5411b to 526ed04 Compare July 16, 2026 12:07

Tejveerpratap2803 added 2 commits July 16, 2026 18:41

Register TransactionLog for field getter

0118662

Update design diagrams for GetLatestSample and RegisterSkeletonTransa…

4c69ed4

…ctionLog

Tejveerpratap2803 force-pushed the muse_skeleton_get branch from 526ed04 to 4c69ed4 Compare July 16, 2026 13:13

bemerybmw approved these changes Jul 16, 2026

View reviewed changes

bemerybmw enabled auto-merge July 16, 2026 13:16

Tejveerpratap2803 self-assigned this Jul 16, 2026

bemerybmw added this pull request to the merge queue Jul 16, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 16, 2026

bemerybmw added this pull request to the merge queue Jul 16, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 16, 2026

bemerybmw added this pull request to the merge queue Jul 17, 2026

Merged via the queue into eclipse-score:main with commit c8a018d Jul 17, 2026
8 checks passed

Uh oh!

Conversation

muhseth commented Mar 18, 2026

Uh oh!

LittleHuba commented Mar 24, 2026

Uh oh!

crimson11 Mar 26, 2026 • edited by LittleHuba Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muhseth Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tejveerpratap2803 May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muhseth Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crimson11 Mar 26, 2026 •

edited by LittleHuba

Loading

muhseth Apr 2, 2026 •

edited

Loading

Tejveerpratap2803 May 19, 2026 •

edited

Loading

muhseth Apr 9, 2026 •

edited

Loading