Impl field methods v2 by KrishaDeshkool · Pull Request #296 · eclipse-score/communication

KrishaDeshkool · 2026-04-14T09:44:36Z

No description provided.

crimson11 · 2026-04-15T12:49:31Z

+
+namespace detail
+{
+


Maybe refactor into its own header: field_tag_types.h .
At least add a small comment, that these tag-types are used on ProxyField/SkeletonField level to accomplish overload-resolution for various signatures, which depend on the availability of Get/Set/Notifier.

crimson11 · 2026-04-15T13:16:16Z

    }

-    auto queue_size = GetQueueSize(parent_handle, method_name_str);
+    // TODO : Field Get/Set methods use a fixed queue size of 1 for now, once it has been fully implemented, this should


I would change this:
Imho we currently already have the general restriction, that we only support a queue size of 1. Because we do have synchronous/blocking method calls!
I looked at the configureation-schema. Interesstingly there we already support 1..255 for method-call queue-size. I did NOT check whether/where we WARN if the user configures a queue size > 1?

Bottom-line:
I would like you NOT to do the "special" handling here. Instead extend GetQueueSize with the additional method_type arg here and within GetQueueSize you do the short-cut/special handling to return 1 in case of Get/Set type ...

crimson11 · 2026-04-15T13:51:29Z

+    ProxyBase& parent,
+    const std::string_view field_name) noexcept
+{
+    return ProxyMethodBindingFactory<SampleType()>::Create(


Hm. Now I lost track. Why do we have this asymetric approach? CreateEventBinding some lines above dispatches to CreateProxyServiceElement ... but here we are dispatching to ProxyMethodBindingFactory<SampleType()>::Create ? So - to me it seems, that either CreateProxyServiceElement (and therefore entire proxy_service_element_binding_factory_impl.h should be removed OR we always dispatch to it ... also in the method case!?
A proxy_service_element_binding_factory_impl.h, which only handles events/fields but not methods makes no sense to me - it is rather confusing!
Can you discuss this with Brendan? I find our approach hard to understand right now ...?

ah! i see your point now. let me think about it

hmm! so i moved out only the part that is genuinely shared across all of them like validating the parent is a lola proxy, std visiting the deployment variant, and resolving the element id into an ElementFqId. removed this misleading proxy_service_element_binding_factory_impl header and then every factory events field notifier, and methods now starts with that one call and then does its own make_unique with whatever args its type needs.

crimson11 · 2026-04-15T14:08:53Z

+        if (method_type == MethodType::kGet || method_type == MethodType::kSet)
+        {
+            lola_service_element_id =
+                GetServiceElementId<ServiceElementType::FIELD>(lola_type_deployment, method_name_str);


How do you handle the differentiation between a Get and Set method here?
They will boil down exactly to the same lola_service_element_id . Is this intended?
I mean the enclosing function gets called WITH method_type ... so you could differ here ... but you do not?
So in case this Create-API gets called for a field-method (get/set), method_name_str contains the field name!
Maybe document it here - that is maybe noteworthy ...

... and then some lines down (line 137) you create a ProxyMethodInstanceIdentifier which additionally gets the method_type ... so the resulting ProxyMethodInstanceIdentifier does contain the get/set distinction?

So - maybe all is fine and is intended - then maybe add some small comments (see above), which make it a bit clearer.

i see it could be better documented,
we can already see now this lola_service_element_id is of type LolaMethodOrFieldId so it is intentional that it would have both method and field ids in this.
will add a clarifying comments.

crimson11 · 2026-04-15T14:12:10Z

    using Event = ProxyEvent<SampleType>;

-    template <typename SampleType>
+    // Note : at the moment the get side implementation is not in the branch which means the skeleton and proxy side


.. the SkeletonField::Get implementation is not ...