Suya add get and set and notification support to the fields in the configuration by soldier-sky · Pull Request #313 · eclipse-score/communication

soldier-sky · 2026-04-16T11:38:27Z

Add useGetIfAvailable/useSetIfAvailable to LoLa field deployment

Introduce dedicated LolaFieldInstanceDeployment class replacing the
former LolaEventInstanceDeployment type alias, adding field-specific
members use_get_if_available_ and use_set_if_available_.
Fix mw_com_config_schema.json: move useGetIfAvailable and
useSetIfAvailable into the field "properties" object so they are
correctly validated when additionalProperties is false.
Adapt existing test and add new tests for LolaFieldInstanceDeployment
Update PUML diagram and readme document

Issue: SWP-250429

- Introduce dedicated LolaFieldInstanceDeployment class replacing the former LolaEventInstanceDeployment type alias, adding field-specific members use_get_if_available_ and use_set_if_available_. - Fix mw_com_config_schema.json: move useGetIfAvailable and useSetIfAvailable into the field "properties" object so they are correctly validated when additionalProperties is false. Issue: SWP-250429

- Update config_parser.cpp to read optional field values useGetIfAvailable/useSetIfAvailable - Fix existing unit test of loloa field instance deployment. Issue: SWP-250429

Update existing tests which refer LolaFieldInstanceDeployment to support additional optional useGetIfAvailable and useSetIfAvailable Issue: SWP-250429

- Add tests to verfiy behaviour of option field values useGetIfAvailable/useSetIfAvailable Issue: SWP-250429

- Update ReadMe and UML diagram to refelct updated changes optional field values useGetIfAvailable/useSetIfAvailable Issue: SWP-250429

bemerybmw · 2026-04-27T11:51:19Z

 {

-using LolaFieldInstanceDeployment = LolaEventInstanceDeployment;
+class LolaFieldInstanceDeployment


A field is a composition of an event and 0 to 2 methods. I would reflect this in the config object. i.e instead of dupcliating LolaEventInstanceDeployment here and adding field specific stuff, why not simply store a LolaEventInstanceDeployment as a member here. e.g.

LolaEventInstanceDeployment lola_event_instance_deployment_{}; bool use_get_if_available_{false}; // coverity[autosar_cpp14_m11_0_1_violation] bool use_set_if_available_{false};

I'd thought over this point and IMHO using composition approach will make coupling tight and LolaFieldInstanceDeployment will do more than 1 task (SRP breaks). Any changes in LolaEventInstanceDeployment e.g. member addition etc will appear in LolaFieldInstanceDeployment as well. Also Serialization part cant be shared cleanly due to use of different JSON key (eventName vs fieldName), plus acess pattern will not be clean e.g. field.max_subscribers_ ---> field.event_.max_subscribers_ .

These classes are tightly coupled either way. The question is just whether we duplicate all the code in LolaEventInstanceDeployment in LolaFieldInstanceDeployment. If we extend LolaEventInstanceDeployment then we want it to be also extended in LolaFieldInstanceDeployment because a field composes an event, and so this should be reflected in the configuration object IMO.

LolaFieldInstanceDeployment will do more than 1 task (SRP breaks)

What additional task is it doing when composing a LolaEventInstanceDeployment vs duplicating all the code within LolaFieldInstanceDeployment?

Also Serialization part cant be shared cleanly due to use of different JSON key

IMO, this should not be the deciding factor about how we structure this code. Once we make a decision about the above point, we can discuss how the serialization code might look.

plus acess pattern will not be clean e.g. field.max_subscribers_ ---> field.event_.max_subscribers_ .

IMO, that actually makes sense. I think it's exactly what we want to have something like:

field.event.max_subscribers_; field.method.queue_size_; ...

@crimson11 what do you think about this?

I'm also all in for "aggregation" ;)
Why: Because a field in mw::com IS already/explicitly an aggregate of a event plus optional methods. So why should this be "different" only on config level. ... and as you implied: It is the rule, that if we extend the event-config, we also need to extend the field-event config. I.e. updating the event-config and NOT updating the event-config part of a field would be an absolute exception to the rule.

As a side-note ... with the property naming eventName, fieldName, methodName ... we messed up imho :p - if it would be simpy name everywhere ... the serialization code would be generic ... but I guess it is too late already to change because our config structure is a "public interface", which is hard to change.

bemerybmw · 2026-04-27T11:52:40Z

+                                            "useGetIfAvailable": {
+                                                "type": "boolean",
+                                                "title": "Use Field Getter If Available",
+                                                "description": "When true, the getter for this field will be used if the service type declares one.",


"...if the service type declares one in the C++ service interface"

Corrected the description

bemerybmw · 2026-04-27T11:52:50Z

+                                            "useSetIfAvailable": {
+                                                "type": "boolean",
+                                                "title": "Use Field Setter If Available",
+                                                "description": "When true, the setter for this field will be used if the service type declares one.",


"...if the service type declares one in the C++ service interface"

Corrected the description

bemerybmw · 2026-04-27T11:54:17Z

+    // coverity[autosar_cpp14_m11_0_1_violation]
+    bool enforce_max_samples_;
+    // coverity[autosar_cpp14_m11_0_1_violation]
+    bool use_get_if_available_{false};


No need for this here. the value is initialized in the constructor.

bemerybmw · 2026-04-27T11:54:24Z

+    // coverity[autosar_cpp14_m11_0_1_violation]
+    bool use_get_if_available_{false};
+    // coverity[autosar_cpp14_m11_0_1_violation]
+    bool use_set_if_available_{false};


No need for this here. the value is initialized in the constructor.

bemerybmw · 2026-04-27T12:06:07Z

@@ -2103,6 +2103,262 @@ TEST(ConfigParser, LolaFieldOptionalEnforceMaxSamples)
    EXPECT_EQ(deploymentInfo.fields_.at("CurrentTemperatureFrontLeft").enforce_max_samples_, false);
 }



Please also modify ParseExampleJson test

I've added assertions to ParseExampleJson for the defaults as example json omits the flags. Note: dedicated tests for those flags (LolaFieldUseGetIfAvailableSetToTrue, LolaFieldUseSetIfAvailableSetToTrue, LolaFieldOmittingBothFlagsDefaultsBothToFalse) and hence only assertion test in ParseExampleJson with existing json file.

See my other comment ... I feel, that updating/extending the example/mw_com_config.json with some of the new properties makes sense. Why - because many people (including me) are sometimes too lazy to look up properties in the schema and check the example/mw_com_config.json instead ... it is more human-readable ;)

bemerybmw · 2026-04-27T12:07:32Z

+    ]
+  }
+)"_json;
+    const auto config = score::mw::com::impl::configuration::Parse(std::move(j2));


PLease also add when, then sections https://cc-github.bmwgroup.net/swh/safe-posix-platform/blob/master/platform/aas/mw/com/test/testing_guidelines.md#given-when-expecting--then

Done. Added appropriate comments

bemerybmw · 2026-04-27T12:11:01Z

+    const LolaFieldInstanceDeployment reconstructed_unit{serialized_unit};
+
+    // Then use_get_if_available is preserved and use_set_if_available is unaffected
+    EXPECT_TRUE(reconstructed_unit.use_get_if_available_);


Better to extract values passed into unit constructor into variables and check the reconstructed value is the same. e.g. EXPECT_EQ(reconstructed_unit.use_get_if_available_, is_get_enabled)

Updated the tests with use of local vars to validate against serialized <-> de-serialized values.

bemerybmw · 2026-04-27T12:13:04Z

+
+TEST_F(LolaFieldInstanceDeploymentFixture, UseGetAndUseSetFlagsAreIndependentOfEachOther)
+{
+    // Given four deployments covering all flag combinations


this duplicates tests above? IMO, we just need these tests:

UseGetIfAvailableIsTrueAfterRoundTripSerialisation

UseSetIfAvailableIsTrueAfterRoundTripSerialisation

BothFlagsDefaultToFalseAfterRoundTripSerialisation is checked in CanCreateFromSerializedObjectWithoutOptionals

bemerybmw · 2026-04-27T12:14:39Z

-    + enforce_max_samples_ : score::cpp::optional<bool>
-    + is_set_configured: bool
-    + is_get_configured: bool
+    {static} + constexpr serializationVersion : std::uint32_t


The class diagrams are not meant to reflect the c++ code exactly. We only add as much detyail as is helpful. You can leave it as it was before IMO

Yes LolaFieldInstanceDeployment appears to be too verbose. I've reduced boiler plate info and make it aligned to LolamethodInstanceDeployment.

Fix review comments and remove redundant testcases Issue: SWP-250429

crimson11 · 2026-04-28T16:09:39Z

+                                            "useGetIfAvailable": {
+                                                "type": "boolean",
+                                                "title": "Use Field Getter If Available",
+                                                "description": "When true, the getter for this field will be used if the service type declares one in the C++ service interface.",


please annotate, that this property is only "relevant" for the proxy side/consumer side.
And should we re-think whether default is "false"? Maybe default should be "true" and you should explicitly disable the use?

And please also extend the example/mw_com_confg.json with the new properties.

crimson11 · 2026-04-28T16:10:32Z

+                                                "description": "When true, the getter for this field will be used if the service type declares one in the C++ service interface.",
+                                                "default": false
+                                            },
+                                            "useSetIfAvailable": {


crimson11 · 2026-04-28T16:31:59Z

 {

-using LolaFieldInstanceDeployment = LolaEventInstanceDeployment;
+class LolaFieldInstanceDeployment


I'm also all in for "aggregation" ;)
Why: Because a field in mw::com IS already/explicitly an aggregate of a event plus optional methods. So why should this be "different" only on config level. ... and as you implied: It is the rule, that if we extend the event-config, we also need to extend the field-event config. I.e. updating the event-config and NOT updating the event-config part of a field would be an absolute exception to the rule.

As a side-note ... with the property naming eventName, fieldName, methodName ... we messed up imho :p - if it would be simpy name everywhere ... the serialization code would be generic ... but I guess it is too late already to change because our config structure is a "public interface", which is hard to change.

crimson11 · 2026-04-28T16:35:38Z

@@ -2103,6 +2103,262 @@ TEST(ConfigParser, LolaFieldOptionalEnforceMaxSamples)
    EXPECT_EQ(deploymentInfo.fields_.at("CurrentTemperatureFrontLeft").enforce_max_samples_, false);
 }



See my other comment ... I feel, that updating/extending the example/mw_com_config.json with some of the new properties makes sense. Why - because many people (including me) are sometimes too lazy to look up properties in the schema and check the example/mw_com_config.json instead ... it is more human-readable ;)

soldier-sky added 5 commits April 15, 2026 14:52

Update config parser to read useGetIfAvailable/useSetIfAvailable

88382fc

- Update config_parser.cpp to read optional field values useGetIfAvailable/useSetIfAvailable - Fix existing unit test of loloa field instance deployment. Issue: SWP-250429

Adapt existing tests to support fields with optional setter/getter

2eef269

Update existing tests which refer LolaFieldInstanceDeployment to support additional optional useGetIfAvailable and useSetIfAvailable Issue: SWP-250429

Add unit tests to verify optional setter and getter field values

01fc4a4

- Add tests to verfiy behaviour of option field values useGetIfAvailable/useSetIfAvailable Issue: SWP-250429

Update configuration documentation

fc98eba

- Update ReadMe and UML diagram to refelct updated changes optional field values useGetIfAvailable/useSetIfAvailable Issue: SWP-250429

soldier-sky force-pushed the suya_Add_get_and_set_and_notification_support_to_the_fields_in_the_configuration branch from 1090c8b to fc98eba Compare April 17, 2026 09:00

soldier-sky marked this pull request as ready for review April 17, 2026 09:00

soldier-sky requested review from LittleHuba, bemerybmw, castler, crimson11, hoe-jo and limdor as code owners April 17, 2026 09:00

bemerybmw requested changes Apr 27, 2026

View reviewed changes

Fix review comments

f514835

Fix review comments and remove redundant testcases Issue: SWP-250429

crimson11 requested changes Apr 28, 2026

View reviewed changes

		@@ -2103,6 +2103,262 @@ TEST(ConfigParser, LolaFieldOptionalEnforceMaxSamples)
		EXPECT_EQ(deploymentInfo.fields_.at("CurrentTemperatureFrontLeft").enforce_max_samples_, false);
		}

Conversation

soldier-sky commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bemerybmw Apr 28, 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!

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!

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!

3 participants

soldier-sky commented Apr 16, 2026 •

edited

Loading

bemerybmw Apr 28, 2026 •

edited

Loading