feat(mw::com): Implement Field Notifier support for Generic Skeleton by ShoroukRamzy · Pull Request #455 · eclipse-score/communication

ShoroukRamzy · 2026-05-20T09:02:10Z

Summary:
This PR extends the GenericSkeleton to support type-erased Fields and solves this issue #181

Key Changes:

GenericSkeletonField: Introduced as a type-erased facade for fields. It takes ownership of a GenericSkeletonEvent under the hood to handle shared memory interactions and broadcasts.
Initial Value Caching: Calling Update() before OfferService() caches the field payload locally. Once the service is offered, DoDeferredUpdate() automatically allocates memory and dispatches this initial value to subscribers.
Zero-copy Notifications: Added support for Allocate() and Update(SampleAllocateePtr) to push new field values to subscribers after the service is actively running.
Skeleton Orchestration: Updated GenericSkeleton::Create to parse field configurations, spawn the fields, and properly populate the internal fields_ map.

Next Steps / Future Work:

Generic Getters & Setters: Currently, RegisterGetHandler and RegisterSetHandler are stubbed out as WIP and return ComErrc::kCouldNotExecute. In the next step, this generic field will be extended with full set and get method support over shared memory once those features are fully implemented.

github-actions · 2026-06-04T07:52:07Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If this PR is still relevant, please leave a comment or push new changes to keep it open.

ShoroukRamzy · 2026-06-04T08:02:47Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If this PR is still relevant, please leave a comment or push new changes to keep it open.

Hi @crimson11, Could you please have a look when you have time? Thanks!

github-actions · 2026-07-02T07:20:25Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If this PR is still relevant, please leave a comment or push new changes to keep it open.

github-actions · 2026-07-09T07:23:23Z

This pull request has been automatically closed due to inactivity. Feel free to reopen it if you would like to continue working on it.

crimson11 · 2026-07-13T06:56:50Z

+    GenericSkeletonField& operator=(GenericSkeletonField&& other) & noexcept;
+
+    /// @brief Updates the reference to the parent skeleton when the skeleton is moved.
+    void UpdateSkeletonReference(SkeletonBase& skeleton_base) noexcept override;


I guess this makes no sense anymore. The code has changed in the meantime and needs rebase.
EnableReferenceToMoveableFromThis has been introduced in the meantime ...

@crimson11, you are right. I updated the code. Thank you for all your review comments!

crimson11 · 2026-07-13T06:59:05Z

+    /// @param set_handler The user-provided handler callback accepting raw bytes and returning
+    ///        the accepted/modified field value as raw bytes.
+    /// @return A result indicating success or an error code.
+    Result<void> RegisterSetHandler(std::function<std::vector<uint8_t>(score::cpp::span<const uint8_t>)> set_handler);


Signature should look different.
The signature in the typed case is: void(FieldType& new_value)
I.e. the provider side callback to the set-handler gets a mutable reference to the value. It can either accept the new value as is ... or it can write to the value to change it, if it needs to correct the new value provided by the client.

Yes, fully agree with you. done!

crimson11 · 2026-07-13T07:24:40Z

+    /// @brief Registers a handler for answering Get requests from proxies.
+    /// @param get_handler The user-provided handler callback returning the field value as raw bytes.
+    /// @return A result indicating success or an error code.
+    Result<void> RegisterGetHandler(std::function<std::vector<uint8_t>()> get_handler);


In the typed case (normal skeleton), we don't provide the ability to register a "GetHandler" to the user. We shouldn't do it here also. A Get-call should be always handled internally. No need to interact with the user ...

crimson11 · 2026-07-13T07:25:35Z

+    /// @param set_handler The user-provided handler callback accepting raw bytes and returning
+    ///        the accepted/modified field value as raw bytes.
+    /// @return A result indicating success or an error code.
+    Result<void> RegisterSetHandler(std::function<std::vector<uint8_t>(score::cpp::span<const uint8_t>)> set_handler);


Maybe state, that an error is returned, when bein called despite has_setter in the ctor was false.

crimson11 · 2026-07-13T07:41:10Z

+
+        // Update the parent skeleton's reference to point to this newly moved instance
+        SkeletonBaseView skeleton_base_view{skeleton_base_.get()};
+        skeleton_base_view.UpdateField(field_name_, *this);


not needed anymore after you have rebased.

crimson11 · 2026-07-13T07:46:32Z

+
+Result<void> GenericSkeletonField::Update(SampleAllocateePtr<void> sample) noexcept
+{
+    // If the field is not configured with a notifier, pushing updates is a no-op.


This is not true.
Let's assume, we have a field with NO notifier, but WITH Get()! You expect, that the Update being done here is visible to the next Get() call! This you are violating. The updated field data is stored by the binding. The Get() call, which is completely handled internally (see my comment above - there is NO user provided Get-handler), will return the last updated field-value! In our case (LoLa binding) this will be fetched form shared memory! But you have to forward the Update call to the binding! Otherwise this all would not work.

Thank you @crimson11 for the clarification, Done!

crimson11 · 2026-07-13T07:53:49Z

+Result<void> GenericSkeletonField::Update(score::cpp::span<const uint8_t> raw_value) noexcept
+{
+    // Cache the initial value if we are updating from a new raw_value payload
+    if (raw_value.data() != initial_field_value_.data())


This I do not get. You are doing a pointer comparison here, to find out what?
If you want to find out, whether this is the 1st Update (to set the initial value), why aren't you checking:
initial_field_value_.size ?

I refactored this part @crimson11.

crimson11 · 2026-07-13T07:57:41Z

@ShoroukRamzy
Sorry for the super late review/feedback! I was on sick leave for a longer time :(

Bottom line: The underlying code-base has changed a bit. So you need to rebase. Also the signature of method-handlers got changed. If you rebase and update your changes ... I promise to be more responsive next time ;)

anmittag · 2026-07-21T10:16:59Z

@ShoroukRamzy some checks are failing and there is a comment, please review and adapt

ShoroukRamzy · 2026-07-21T11:08:11Z

@ShoroukRamzy some checks are failing and there is a comment, please review and adapt
Hi @anmittag and @crimson11, Thank you for all your feedbacks. If I couldn't handle this PR reviews this week, I will handle them next week. is it suitable for you?

ShoroukRamzy · 2026-07-30T11:16:41Z

@ShoroukRamzy Sorry for the super late review/feedback! I was on sick leave for a longer time :(

Bottom line: The underlying code-base has changed a bit. So you need to rebase. Also the signature of method-handlers got changed. If you rebase and update your changes ... I promise to be more responsive next time ;)

Hi @crimson11, welcome back and thank you for all your review comments.

crimson11 · 2026-07-30T21:41:56Z

@ShoroukRamzy Sorry for the super late review/feedback! I was on sick leave for a longer time :(
Bottom line: The underlying code-base has changed a bit. So you need to rebase. Also the signature of method-handlers got changed. If you rebase and update your changes ... I promise to be more responsive next time ;)

Hi @crimson11, welcome back and thank you for all your review comments.

No problem! :D - I see there are still some build issues. Ping me when you want a review.

crimson11 · 2026-08-03T15:30:29Z

@ShoroukRamzy just saw that you had formatting-errors ... IDK, if it is obvious, but I always do a
bazel run //:format in the communication repo, before pushing ... it fixes/formats both:

c++ (clang format)
BUILD files (starlark)

ShoroukRamzy · 2026-08-03T16:28:09Z

@ShoroukRamzy just saw that you had formatting-errors ... IDK, if it is obvious, but I always do a bazel run //:format in the communication repo, before pushing ... it fixes/formats both:

c++ (clang format)

BUILD files (starlark)

Yes @crimson11, I was verifying an issue on CI that I observed while solving unit test on this PR. I opened a separate PR for it kindly check #825
Thanks!

crimson11 · 2026-08-04T15:25:06Z

+    std::vector<uint8_t> initial_field_value_;
+    bool has_initial_value_{false};
+
+    [[maybe_unused]] bool has_getter_{false};


Are these initializers needed? I guess you have initialization for them in your ctors initializer list! And I did not seen any other ctor, which hasn't.
So - I would expect also that some quality-tools would warn, because this "default" initialization is dead code - will never happen and thus is confusing.

crimson11 · 2026-08-04T15:28:54Z

+
+GenericSkeletonEvent* GenericSkeletonField::GetGenericEvent() const noexcept
+{
+    auto* const typed_event = dynamic_cast<GenericSkeletonEvent*>(skeleton_event_dispatch_.get());


typed_event is a funny name here ... where we implement an untyped/type-agnostic generic event ;)

I guess generic_event or generic_skeleton_event would be more suitable.

crimson11 · 2026-08-04T15:30:36Z

+namespace score::mw::com::impl
+{
+
+/// @brief Represents a generic, type-erased skeleton field.


not that we have a "golden rule" here ... but I guess we normally use "" instead "@" as doxygen tag prefix ... maybe replace globally in your changed files.

crimson11 · 2026-08-04T15:35:04Z

+    bool has_getter{false};
+    bool has_setter{false};
+    bool has_notifier{false};
+    score::cpp::span<const uint8_t> initial_value{};


Is this needed here? AFAIS FieldInfo is only "used" accessed in GenericSkeleton ctor .. and there it is not used. So typically the user-code, which created the GenericSkeleton will later expliucitly call Field::Update() before doing the Offer() call ... so - I see no use/need in having this initial_value member here?

crimson11 · 2026-08-04T15:50:24Z

+    ///         with a setter (i.e. has_setter in the constructor was false).
+    Result<void> RegisterSetHandler(std::function<void(score::cpp::span<uint8_t>)> set_handler);
+
+  private:


No mocking support for GenericSkeletonField? I guess you should add - people need to do unit testing with GenericSkeletonFields exactly like with "typed" SkeletonField ...

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

+            return MakeUnexpected(ComErrc::kBindingFailure);
+        }
+
+        auto field_binding_result =


it is rather a event_binding_result since on binding level there is no "field" ... on binding level, we have only representations for the composite-members of the field-composite ... when you later create the bindings for getters/setters ... you would also name thzem method_binding_result

crimson11 · 2026-08-04T16:21:27Z

 }
+
+// Helper to fetch the stable field name from the Configuration
+std::string_view GetFieldName(const InstanceIdentifier& identifier, std::string_view search_name)


I guess you need to extend generic_skeleton_test.cpp for testing this?

crimson11 · 2026-08-04T16:22:02Z

        }
    }

+    // 3. Create fields directly in the map


I guess you need to extend generic_skeleton_test.cpp for testing this extended Create() functionality?

crimson11 · 2026-08-04T16:30:36Z

@@ -0,0 +1,354 @@
+/********************************************************************************


Looking at the tests here, they seem to be structurally a bit different than the tests in generic_skeleton_event_test.cpp
Why? Doesn't it make sense to "duplicate" structure/approach?
And then please also use always in tests the given-when-expect pattern to document the test as we (hopefully) do in all tests.

ShoroukRamzy requested review from LittleHuba, bemerybmw, castler, crimson11, hoe-jo and limdor as code owners May 20, 2026 09:02

ShoroukRamzy force-pushed the generic_skeleton_field_notifier branch from 1cb68f8 to e8cdd9f Compare May 20, 2026 10:58

github-actions Bot added the stale label Jun 4, 2026

github-actions Bot removed the stale label Jun 5, 2026

ShoroukRamzy force-pushed the generic_skeleton_field_notifier branch 2 times, most recently from ae65f9d to aa82c73 Compare June 8, 2026 18:00

anmittag assigned ShoroukRamzy Jun 17, 2026

anmittag added this to COM - Communication FT Jun 17, 2026

github-project-automation Bot moved this to Backlog in COM - Communication FT Jun 17, 2026

github-actions Bot added the stale label Jul 2, 2026

github-actions Bot closed this Jul 9, 2026

github-project-automation Bot moved this from Backlog to Done in COM - Communication FT Jul 9, 2026

crimson11 reopened this Jul 13, 2026

crimson11 requested changes Jul 13, 2026

View reviewed changes

anmittag moved this from Done to In Progress in COM - Communication FT Jul 21, 2026

anmittag moved this from In Progress to Ready in COM - Communication FT Jul 21, 2026

ShoroukRamzy force-pushed the generic_skeleton_field_notifier branch from aa82c73 to 700ba79 Compare July 30, 2026 12:10

ShoroukRamzy force-pushed the generic_skeleton_field_notifier branch from 700ba79 to 11c6443 Compare July 30, 2026 12:16

LittleHuba removed the stale label Jul 31, 2026

ShoroukRamzy force-pushed the generic_skeleton_field_notifier branch 8 times, most recently from be23028 to 02a03a6 Compare August 3, 2026 15:07

ShoroukRamzy force-pushed the generic_skeleton_field_notifier branch from 02a03a6 to 3df0ea6 Compare August 3, 2026 16:00

ShoroukRamzy mentioned this pull request Aug 3, 2026

fix(ci): Prevent runner permission collisions in log uploader #825

Merged

crimson11 requested changes Aug 4, 2026

View reviewed changes

ShoroukRamzy added 11 commits August 5, 2026 11:59

feat: implement generic skeleton field with notifier support

9ae1989

fix compilation issue

22e64ed

add comments and fix IsSetHandlerRegistered check

66a76a8

fix field double registeration issue and add unit test

4aa9fc4

Adapt the code to use the new ReferenceToMovable class

0d4f2a5

remove RegisterGetHandler and update the update function logic

10fe011

update RegisterSetHandler API signature

8a14b70

update generic_skeleton design doc

cd7a56c

fix unit test in generic_skeleton_field

b5bda9a

suppress unused private field warnings in GenericSkeletonField

2ec16dd

trigger ci

b632800

ShoroukRamzy force-pushed the generic_skeleton_field_notifier branch from 1f88edb to b632800 Compare August 5, 2026 09:19

		@@ -0,0 +1,354 @@
		/********************************************************************************

Uh oh!

Conversation

ShoroukRamzy commented May 20, 2026

Uh oh!

github-actions Bot commented Jun 4, 2026

Uh oh!

ShoroukRamzy commented Jun 4, 2026

Uh oh!

github-actions Bot commented Jul 2, 2026

Uh oh!

github-actions Bot commented Jul 9, 2026

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crimson11 commented Jul 13, 2026

Uh oh!

anmittag commented Jul 21, 2026

Uh oh!

ShoroukRamzy commented Jul 21, 2026

Uh oh!

ShoroukRamzy commented Jul 30, 2026

Uh oh!

crimson11 commented Jul 30, 2026

Uh oh!

crimson11 commented Aug 3, 2026

Uh oh!

ShoroukRamzy commented Aug 3, 2026

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants