mw/com: Add Requirements for the Method extension of Fields#55
Conversation
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
There was a problem hiding this comment.
Several requirement descriptions need clarification and safety tagging fixes before approval.
Status: Changes Suggested | Risk: Medium | Readiness: 6/10 (Needs Polish)
Issues Identified & Suggestions
- Clarify zero-copy Set signature and ownership semantics:
score/mw/com/requirements/component_requirements/component_requirements_ipc_fields.trlc - Correct Set result description; should not reference get():
score/mw/com/requirements/component_requirements/component_requirements_ipc_fields.trlc - Add missing ASIL-B safety fields for derived requirements:
score/mw/com/requirements/component_requirements/component_requirements_ipc_fields.trlc - Fix unbalanced backtick in description string:
score/mw/com/requirements/component_requirements/component_requirements_ipc_fields.trlc
Review Details
📁 2 files reviewed | 💬 4 comments
👍 / 👎 individual comments to help improve reviews for you
| ScoreReq.CompReq ZeroCopySetMethodSignature { | ||
| description="""The signature of the `Set` method shall be: `bmw::Result<MethodReturnTypePtr<SampleDataType>> Set(MethodInArgPtr<SampleDataType>)`""" |
There was a problem hiding this comment.
[Logic] SetMethodSignature (line 76) and ZeroCopySetMethodSignature (line 107) specify identical signatures: bmw::Result<MethodReturnTypePtr<SampleDataType>> Set(MethodInArgPtr<SampleDataType>). The zero-copy variant should presumably take an already-allocated buffer (from Allocate()) with move/ownership semantics, yet the signature description is verbatim identical to the non-zero-copy version. Is this intentional, or should the zero-copy variant's description clarify the ownership/move semantics difference?
Context for Agents
`SetMethodSignature` (line 76) and `ZeroCopySetMethodSignature` (line 107) specify identical signatures: `bmw::Result<MethodReturnTypePtr<SampleDataType>> Set(MethodInArgPtr<SampleDataType>)`. The zero-copy variant should presumably take an already-allocated buffer (from `Allocate()`) with move/ownership semantics, yet the signature description is verbatim identical to the non-zero-copy version. Is this intentional, or should the zero-copy variant's description clarify the ownership/move semantics difference?
File: score/mw/com/requirements/component_requirements/component_requirements_ipc_fields.trlc
Line: 107Chain of Thought
- Validation Decision: accepted
- Confidence: 0.80
- Likelihood Score: 3/5
- Likelihood Reason: The signatures are demonstrably identical, which is suspicious for a zero-copy variant. There's also a type mismatch: Allocate() returns impl::MethodInArgPtr while ZeroCopySetMethodSignature takes plain MethodInArgPtr. This could be intentional design (the difference is runtime, not type-level) or a copy-paste oversight.
- Suggested Action: modify
- Tool Calls Used: 5
Reasoning
The comment is factually accurate:
-
Identical signatures confirmed: Line 76 (SetMethodSignature) and line 107 (ZeroCopySetMethodSignature) both specify the exact same signature:
bmw::Result<MethodReturnTypePtr<SampleDataType>> Set(MethodInArgPtr<SampleDataType>). -
Additional supporting evidence: The
AllocateMethodSignature(line 94) returnsbmw::Result<impl::MethodInArgPtr<SampleDataType>>— note theimpl::namespace qualifier onMethodInArgPtr. This is different from the plainMethodInArgPtrused in the ZeroCopySetMethodSignature. This inconsistency further suggests the zero-copy variant's signature description may be incomplete or copied incorrectly. -
This is new code: The entire file is newly added in this PR, so this is entirely introduced by this PR.
-
Actionable: The comment identifies a specific potential issue (identical descriptions where they should probably differ) and asks the right question. The developer can either confirm it's intentional or fix the zero-copy variant's description.
The only reservation is that in some zero-copy IPC patterns, the Set method genuinely has the same type signature — the difference is only in how the argument was obtained (from Allocate vs normal construction). However, if that's the case, the requirement description should clarify this to avoid confusion, and the impl::MethodInArgPtr vs MethodInArgPtr inconsistency with Allocate still warrants attention.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/requirements/feature_requirements/feature_requirements_ipc.trlc
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
score/mw/com/requirements/component_requirements/component_requirements_ipc_fields.trlc
1: /********************************************************************************
2: * Copyright (c) 2026 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
| description="""After the user provided `SetHandler` has returned, the (potentially updated) field value has to: | ||
| - be stored as the latest value. | ||
| - if enabled, notifier for the updated value has to be called to inform subscribers of the field about the updated value. | ||
| - be returned as the result of the `get()` method call to the proxy side.""" |
There was a problem hiding this comment.
[Logic] The description states that the updated field value should be "returned as the result of the get() method call to the proxy side." This seems incorrect, as this requirement describes the behavior following a Set operation initiated from the proxy. The result should be returned to the Set() method call, not a get() call.
Please clarify if this is intended, or correct get() to Set() for clarity and correctness.
Context for Agents
The description states that the updated field value should be "returned as the result of the `get()` method call to the proxy side." This seems incorrect, as this requirement describes the behavior following a `Set` operation initiated from the proxy. The result should be returned to the `Set()` method call, not a `get()` call.
Please clarify if this is intended, or correct `get()` to `Set()` for clarity and correctness.
File: score/mw/com/requirements/component_requirements/component_requirements_ipc_fields.trlc
Line: 160Chain of Thought
- Validation Decision: accepted
- Confidence: 0.82
- Likelihood Score: 4/5
- Likelihood Reason: The requirement describes the skeleton-side behavior after a SetHandler returns (triggered by proxy's Set() call). The proxy-side SetMethodReturnValue requirement (line 82) explicitly states that
Setreturns the updated field value. Usingget()here is almost certainly a typo that would cause confusion in the requirements specification. - Suggested Action: keep
- Tool Calls Used: 3
Reasoning
The comment correctly identifies an inconsistency in the requirements text at line 160.
Context analysis:
- The requirement at lines 156-163 (
SetHandlerTypeDefinition) is in the "Field_member_representation_at_the_Skeleton_side" section - It describes what happens after the
SetHandlerhas returned on the skeleton side - The
SetHandleris invoked when a proxy callsSet()on a field
The flow:
- Proxy calls
Set()→ skeleton receives the request - Skeleton invokes the registered
SetHandlerwith the new value SetHandlerpotentially modifies the value and returns- After return, the three things must happen: store as latest, notify subscribers, and return the value to the proxy side
The issue:
Line 160 says: "be returned as the result of the get() method call to the proxy side"
But looking at the proxy-side requirements:
SetMethodSignature(line 76):bmw::Result<MethodReturnTypePtr<SampleDataType>> Set(MethodInArgPtr<SampleDataType>)— theSet()method returns the updated valueSetMethodReturnValue(line 82): "Setmethod shall return the field value, which has been updated last by the provider in success case"
Since the entire flow describes a Set operation, the value should be returned as the result of the proxy's Set() call, not get(). The get() reference is inconsistent with the operational context.
One could argue get() means "the value that a subsequent get() call would return" but that interpretation would be redundant with bullet 1 ("be stored as the latest value"), and the phrasing "be returned as the result of" implies an active return in the current operation.
The comment is:
- Accurate — correctly identifies the inconsistency
- Actionable — suggests changing
get()toSet() - Introduced by this PR — this is entirely new content
I rate confidence at 0.82 (not higher) because there's a small chance the requirement intentionally describes the return value both for the Set() response an...
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/requirements/component_requirements/component_requirements_ipc_fields.trlc - Lines:
143-168
143: ScoreReq.CompReq SetHandlerTypeDefinition {
144: description="""The `SetHandlerType` shall be a callable of this type: using SetHandlerType = amp::callback<void(SampleDataType&)>;`"""
145: derived_from = [Communication.Field@1]
146: version = 1
147: }
148:
149: ScoreReq.CompReq SetHandlerTypeDefinition {
150: description="""It is mandatory to register a `SetHandler` for a `SkeletonField` instance. If the user has not registered a handler before calling `Offer()` on the enclosing skeleton, the call to `Offer()` shall fail with an error."""
151: derived_from = [Communication.Field@1, Communication.ServiceInstance@1]
152: version = 1
...
2. File Snapshot (HEAD)
- File:
score/mw/com/requirements/feature_requirements/feature_requirements_ipc.trlc
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Snapshot (HEAD)
- File:
score/mw/com/requirements/component_requirements/component_requirements_ipc_fields.trlc - Lines:
69-112
69: ScoreReq.CompReq SetMethodOnlyIfEnabledInInterfaceDefinition {
70: description="""It shall provide a public `Set` method only in the case, that "support for a Set()" has been enabled in the underlying interface definition (see [[FieldGetAndSetInterfaceDefinition]])."""
71: derived_from = [Communication.Field@1]
72: version = 1
73: }
74:
75: ScoreReq.CompReq SetMethodSignature {
76: description=""" The signature of the `Set` method shall be: `bmw::Result<MethodReturnTypePtr<SampleDataType>> Set(MethodInArgPtr<SampleDataType>)`"""
77: derived_from = [Communication.Field@1]
78: version = 1
...
| ScoreReq.CompReq GetMethodOnlyIfEnabledInInterfaceDefinition { | ||
| description="""It shall provide a public `Get` method only in the case, that 'support for a `Get()`' has been enabled in the underlying interface definition (see [[FieldGetAndSetInterfaceDefinition]])""" | ||
| derived_from = [Communication.Field@1] | ||
| version = 1 |
There was a problem hiding this comment.
[Company Standards] Inconsistent safety field: FieldGetAndSetInterfaceDefinition (line 29) and NoSupportForNotificationInSharedMemoryBinding (line 38) specify safety = ScoreReq.Asil.B, but all remaining CompReq entries in this file omit the safety field. If safety is required for ASIL-B-derived requirements (which these are, given derived_from includes Communication.Field@1 with safety = ScoreReq.Asil.B), the missing fields will either cause validation failures or default to a lower ASIL level than intended. Add safety = ScoreReq.Asil.B to all entries derived from safety-relevant requirements.
Context for Agents
Inconsistent `safety` field: `FieldGetAndSetInterfaceDefinition` (line 29) and `NoSupportForNotificationInSharedMemoryBinding` (line 38) specify `safety = ScoreReq.Asil.B`, but all remaining `CompReq` entries in this file omit the `safety` field. If `safety` is required for ASIL-B-derived requirements (which these are, given `derived_from` includes `Communication.Field@1` with `safety = ScoreReq.Asil.B`), the missing fields will either cause validation failures or default to a lower ASIL level than intended. Add `safety = ScoreReq.Asil.B` to all entries derived from safety-relevant requirements.
File: score/mw/com/requirements/component_requirements/component_requirements_ipc_fields.trlc
Line: 50Chain of Thought
- Validation Decision: accepted
- Confidence: 0.92
- Likelihood Score: 4/5
- Likelihood Reason: The existing codebase pattern is 100% consistent - every CompReq in the sibling file has safety = ScoreReq.Asil.B. The new file omits it from ~15/17 entries while including it in 2, strongly suggesting an oversight. Whether TRLC schema enforces this or not, the inconsistency will either cause validation failures or silently default to a lower safety level than intended for safety-critical requirements.
- Suggested Action: keep
- Tool Calls Used: 14
Reasoning
The comment correctly identifies a genuine inconsistency in the newly introduced file. Evidence:
-
Existing convention: In
component_requirements_ipc.trlc(the sibling file, 3588 lines), every singleScoreReq.CompReqentry includessafety = ScoreReq.Asil.B. This is a universal pattern with zero exceptions. -
Parent requirement safety level:
Communication.Field@1(thederived_fromtarget for all new entries) is defined withsafety = ScoreReq.Asil.Bat line 60-65 offeature_requirements_ipc.trlc. -
Within-file inconsistency: Even within the new file itself,
FieldGetAndSetInterfaceDefinition(line 29) andNoSupportForNotificationInSharedMemoryBinding(line 38) havesafety = ScoreReq.Asil.B, but the remaining ~15 entries omit it. This strongly suggests an oversight rather than intentional omission. -
Introduced by this PR: The file is entirely new (didn't exist in BASE), so this issue is fully the PR's responsibility.
-
Actionability: The comment provides a specific, concrete action - add
safety = ScoreReq.Asil.Bto all entries derived from safety-relevant requirements.
The concern about safety classification in an ASIL-rated system is significant. Missing safety annotations on requirements derived from ASIL-B parent requirements could lead to incorrect safety traceability.
Evidence Summary
1. File Lookup
- Filename:
feature_requirements_ipc.trlc
Found 1 file(s) matching 'feature_requirements_ipc.trlc':
- score/mw/com/requirements/feature_requirements/feature_requirements_ipc.trlc
2. File Lookup
- Filename:
.trlc - Max Results:
10
No files found matching '.trlc'
3. File Snapshot (HEAD)
- File:
score/mw/com/requirements/feature_requirements/feature_requirements_ipc.trlc
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4. File Lookup
- Filename:
CompReq - Max Results:
10
No files found matching 'CompReq'
5. File Snapshot (BASE)
- File:
score/mw/com/requirements/feature_requirements/feature_requirements_ipc.trlc
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
6. File Lookup
- Filename:
component_requirements_ipc.trlc - Max Results:
10
Found 1 file(s) matching 'component_requirements_ipc.trlc':
- score/mw/com/requirements/component_requirements/component_requirements_ipc.trlc
7. File Lookup
- Filename:
ScoreReq.rsl - Max Results:
10
No files found matching 'ScoreReq.rsl'
8. File Lookup
- Filename:
score_req.rsl - Max Results:
10
No files found matching 'score_req.rsl'
9. File Lookup
- Filename:
.rsl - Max Results:
10
No files found matching '.rsl'
10. File Snapshot (BASE)
- File:
score/mw/com/requirements/component_requirements/component_requirements_ipc.trlc
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
| } | ||
|
|
||
| ScoreReq.CompReq SetHandlerTypeDefinition { | ||
| description="""The `SetHandlerType` shall be a callable of this type: using SetHandlerType = amp::callback<void(SampleDataType&)>;`""" |
There was a problem hiding this comment.
[Documentation] Unbalanced backtick in the description string on line 144: the text has a closing backtick after the semicolon (;) but is missing the corresponding opening backtick before using. This will render incorrectly in any documentation generator.
Fix: add the missing opening backtick before using:
| description="""The `SetHandlerType` shall be a callable of this type: using SetHandlerType = amp::callback<void(SampleDataType&)>;`""" | |
| description="""The `SetHandlerType` shall be a callable of this type: `using SetHandlerType = amp::callback<void(SampleDataType&)>;`""" |
Context for Agents
Unbalanced backtick in the description string on line 144: the text has a closing backtick after the semicolon (`;`) but is missing the corresponding opening backtick before `using`. This will render incorrectly in any documentation generator.
Fix: add the missing opening backtick before `using`:
```suggestion
description="""The `SetHandlerType` shall be a callable of this type: `using SetHandlerType = amp::callback<void(SampleDataType&)>;`"""
```
File: score/mw/com/requirements/component_requirements/component_requirements_ipc_fields.trlc
Line: 144Chain of Thought
- Validation Decision: accepted
- Confidence: 0.95
- Likelihood Score: 5/5
- Likelihood Reason: The unbalanced backtick is definitively present in the code on line 144. Any documentation generator processing this file will render the backtick incorrectly.
- Suggested Action: modify
- Tool Calls Used: 1
Reasoning
The comment correctly identifies an unbalanced backtick issue on line 144. The actual text is:
The `SetHandlerType` shall be a callable of this type: using SetHandlerType = amp::callback<void(SampleDataType&)>;`
There's a missing opening backtick before "using" — the closing backtick after ";" has no matching opener. The intended markup was clearly to wrap the code portion in backticks: using SetHandlerType = amp::callback<void(SampleDataType&)>;
The comment's description of the problem is slightly imprecise (it says "unmatched opening backtick before using" when really the issue is a missing opening backtick there), but the core finding is accurate and the suggested fix is correct. This is a new file added entirely by this PR, so the issue is introduced by this change.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/requirements/component_requirements/component_requirements_ipc_fields.trlc - Lines:
143-147
143: ScoreReq.CompReq SetHandlerTypeDefinition {
144: description="""The `SetHandlerType` shall be a callable of this type: using SetHandlerType = amp::callback<void(SampleDataType&)>;`"""
145: derived_from = [Communication.Field@1]
146: version = 1
147: }
2 participants
issue:
mw/com: Add Requirements for the Method extension of Fields
This PR introduces comprehensive requirements for the
Fieldcommunication element, detailing itsGet(),Set(), and notification functionalities. It specifies how these methods should behave on both the proxy and skeleton sides, including signatures and return values. Additionally, it defines requirements for zero-copySet()operations and clarifies notification behavior for shared-memory bindings.Architecture Diagram:
flowchart TD subgraph Requirements_Layer FR["Feature_Requirements_IPC (+7 lines)"] CR["Component_Requirements_IPC_Fields (+168 lines)"] end subgraph Communication_Components Field["Field (Concept)"] Proxy["Proxy Side"] Skeleton["Skeleton Side"] end CR -->|defines behavior for| Field FR -->|defines feature| Field CR -->|specifies API for| Proxy CR -->|specifies API for| Skeleton Proxy -->|uses Get/Set/Allocate| Field Skeleton -->|registers handlers for| FieldThis summary was automatically generated by @propel-code-bot