Broader refactoring of manifest-based naming and multi A2L by pmai · Pull Request #81 · modelica/fmi-ls-xcp

pmai · 2024-06-19T16:53:23Z

This constitutes a more complete refactoring of the manifest schema, so that better naming and expandability is achieved.

Fixes #63.

@t-sommer

Regularize naming of attributes (fixes #68, #78, #79). Incorporate formatting changes from #69 by @t-sommer.

@t-sommer

This reflects discussions with @t-sommer for a broader refactoring, also fixes #80.

chrbertsch · 2024-06-25T13:42:21Z

FMI Design Webmeeting:
Torsten: the filename gets appended to the path to the corresponding binary.
Patrick: what if we have an ME and an CS binary?
Pierre: this is a general problem ... If you want to have multiple A2L files
Torsten: We could add another attribute "modelIdentifier" ...
Pierre: also in the old proposal you would need a way to state that you need a different set of a2l files ... We have to fix this in any case ...
Patrick: Before there was no issue in the case of only one a2l file ...
Pierre: we need to express whether a set a2l files is applicable to an ME, CS implementation ...
If we have multiple implementation ... we might need multiple a2l files. This was not expressible in the old proposal.
Pierre: we should state which use cases we want to support. -->#83

Fixes #83.

kahramonj · 2024-07-19T15:02:20Z

schema/fmi3LayeredStandardXcpManifest.xsd

Thank you very much for your changes. I have some questions and small notes:

Is it intention that the enumeration names in "role" don't include the "IP" as in original XCP specifications like TCP/IP or UDP/IP, which represent the combination of transport and network layer?
Especially, if we have "Address" in the name like "XCPServiceTCPListenAddress", it should mean "IP-Address" in the network layer, otherwise it could also be MAC-Address in data link layer.
If we are using Ethernet terms, maybe it is better to use "Listener" (also called TCP/IP-Server) instead "Listen" (or XCPSlave in XCP terms)?

Is it independent on the IP versions or should we mention it somewhere (currently I see that we are using everywhere IPv4 in the examples, but nowhere mentioned IPv6). Not sure, if also multicasts IPs (only UDP/IP) supported.

One more side note: I read somewhere that XCP Slave detection was defined in XCP version 1.3 specifically for XCP on Ethernet, so if we have to mention the minimum supported XCP version somewhere.

There is no XCP on Ethernet, even though the ASAM authors called it that, but they were just mistaken, they defined XCP on TCP and UDP which is independent of any data link layer. Hence there is no potential confusion with MAC addresses or anything of that nature.

One can add the IP in there, but it adds little value, as the IP is always implied (there is no UDP/TCP defined on anything else than IP).

We should clarify that the address can in theory also be IPv6 rather than only IPv4.

The layered standard is independent of the XCP version.

Pierre: I think that the "IP" part is not very important. We should mention IPV6. I will add it.
Kharamonj: IP is network layer, UDP is transport layer ... but not important
Pierre: historically people included "/IP" ... it does not add benefit.
Pierre: mention "minimum supported XCP version": You have to read the A2L file anyway, there it will be mentioned. We are independent on a specific XCP version.

t-sommer

The variables XCPService{TCP|UDP}Enable do not end with a noun which looks kind of odd. Maybe we should call it EnableXCPService{TCP|UDP}or even better XCPStart{TCP|UDP}Service`, because this is what it does according to the description.

docs/index.adoc

PTaeuberDS · 2024-07-30T09:34:49Z

The variables XCPService{TCP|UDP}Enable do not end with a noun which looks kind of odd. Maybe we should call it EnableXCPService{TCP|UDP}or even better XCPStart{TCP|UDP}Service`, because this is what it does according to the description.

I agree that XCPService{TCP|UDP}Enable doesn't look nice. The earlier variant EnableXcpOnTcpIp was based on the A2L block name that this config variable refers to: XCP_ON_TCP_IP. I still prefer that variant.

PTaeuberDS · 2024-07-30T09:09:04Z

docs/index.adoc

 If size is a concern, it may be decided to supply just a single platform and A2L file with an FMU.

-The root name of the A2L file shall be identical to the model identifier and is case sensitive, i.e., a variable description named `<modelIdentifier>.a2l` is associated with an FMU binary named `<modelIdentifier>.{dll|so}`.
+The root name of the A2L file shall be identical to the `identifier` attribute from the `Interface` element of the manifest file and is case sensitive.


to the definition attribute?

Pierre: this might be a left-over, I will look into this

The whole sentence should be removed as it is in the wrong place of the doc, and it is now redundant to the actual mechanism of locating the a2l files from the manifest. Will remove in wrap up.

docs/index.adoc

pmai · 2024-07-30T13:00:45Z

The variables XCPService{TCP|UDP}Enable do not end with a noun which looks kind of odd. Maybe we should call it EnableXCPService{TCP|UDP}or even better XCPStart{TCP|UDP}Service`, because this is what it does according to the description.

The common prefix is more important IMHO, especially since enable flag variables are commonly termed enable and not enable flag or something even more cumbersome. German angst about non-nouns aside...

chrbertsch · 2024-07-30T13:56:18Z

The variables XCPService{TCP|UDP}Enable do not end with a noun which looks kind of odd. Maybe we should call it EnableXCPService{TCP|UDP}or even better XCPStart{TCP|UDP}Service`, because this is what it does according to the description.

I agree that XCPService{TCP|UDP}Enable doesn't look nice. The earlier variant EnableXcpOnTcpIp was based on the A2L block name that this config variable refers to: XCP_ON_TCP_IP. I still prefer that variant.

Pierre: we are not talking about the variable names any more, only the role names.
Pierre: the benefit of the current proposal is the common prefix.
Patrick: for me the current way is fine.

This reflects the review feedback and should not induce any technical changes. Signed-off-by: Pierre R. Mai <[email protected]>

pmai

This hopefully finalizes the changes discussed in the last design meeting.

pmai · 2024-07-30T13:06:40Z

docs/index.adoc

 | Relative URI
 | This attribute must be a relative reference to a URI (formerly called a relative URI) to the A2L variable description file for this interface.
-It may not contain any dot-segments (i.e. complete path segments of `.` or `..`).
+It may not contain any dot-segments (i.e., complete path segments of `.` or `..`).


rather than adding the commas I think we should work more to remove the use of i.e. in its entirety by having a separate definition of dot segments, or otherwise re-phrasing. This goes for all occurrences of i.e.

pmai · 2024-07-30T13:11:38Z

schema/fmi3LayeredStandardXcpManifest.xsd

There is no XCP on Ethernet, even though the ASAM authors called it that, but they were just mistaken, they defined XCP on TCP and UDP which is independent of any data link layer. Hence there is no potential confusion with MAC addresses or anything of that nature.

One can add the IP in there, but it adds little value, as the IP is always implied (there is no UDP/TCP defined on anything else than IP).

We should clarify that the address can in theory also be IPv6 rather than only IPv4.

The layered standard is independent of the XCP version.

pmai · 2024-08-08T13:23:54Z

docs/index.adoc

 If size is a concern, it may be decided to supply just a single platform and A2L file with an FMU.

-The root name of the A2L file shall be identical to the model identifier and is case sensitive, i.e., a variable description named `<modelIdentifier>.a2l` is associated with an FMU binary named `<modelIdentifier>.{dll|so}`.
+The root name of the A2L file shall be identical to the `identifier` attribute from the `Interface` element of the manifest file and is case sensitive.


The whole sentence should be removed as it is in the wrong place of the doc, and it is now redundant to the actual mechanism of locating the a2l files from the manifest. Will remove in wrap up.

pmai added 3 commits June 19, 2024 18:51

Initial sketch of manifest-based naming and multi A2L

96f1d60

Fixes #63.

Add documentation for manifest-based variable naming

4e4cd3e

Regularize naming of attributes (fixes #68, #78, #79). Incorporate formatting changes from #69 by @t-sommer.

Broader refactoring of manifest mechanism and naming

51f1aec

This reflects discussions with @t-sommer for a broader refactoring, also fixes #80.

pmai added this to the v1.0 milestone Jun 19, 2024

pmai self-assigned this Jun 19, 2024

pmai mentioned this pull request Jun 19, 2024

Initial sketch of manifest-based naming and multi A2L #76

Closed

chrbertsch requested review from MartinBenedikt, PTaeuberDS and bmenne-dspace and removed request for MartinBenedikt June 25, 2024 13:57

Add ability to restrict Interface to certain CS/ME/SE

e3c2ac2

Fixes #83.

chrbertsch requested review from kahramonj and t-sommer July 16, 2024 13:37

kahramonj reviewed Jul 19, 2024

View reviewed changes

t-sommer added 9 commits July 22, 2024 11:09

Validate examples in CI

be12946

List directory

d459773

Checkout repository in CI

9232c41

Install dependencies

5260822

Clean up

87660da

Fix broken xrefs

05310bf

Use curly braces for protocol alternatives

f0e0453

Extend description of "types" attribute

f027a33

Fix role attribute for XCP.vECU1.UDP.* variables

cce9da0

t-sommer requested changes Jul 22, 2024

View reviewed changes

chrbertsch reviewed Jul 27, 2024

View reviewed changes

docs/index.adoc Show resolved Hide resolved

docs/index.adoc Show resolved Hide resolved

docs/index.adoc Show resolved Hide resolved

Fix list

7d0af89

Some small formal changes

a71e7ad

PTaeuberDS force-pushed the feature/multi-a2l-support-struct branch from 72209f3 to a71e7ad Compare July 30, 2024 10:20

PTaeuberDS reviewed Jul 30, 2024

View reviewed changes

bmenne-dspace requested changes Jul 30, 2024

View reviewed changes

docs/index.adoc Show resolved Hide resolved

docs/index.adoc Show resolved Hide resolved

bmenne-dspace approved these changes Jul 30, 2024

View reviewed changes

kahramonj self-requested a review July 30, 2024 14:13

kahramonj approved these changes Jul 30, 2024

View reviewed changes

PTaeuberDS approved these changes Jul 30, 2024

View reviewed changes

chrbertsch mentioned this pull request Aug 2, 2024

Create a release candidate #86

Closed

Adjust wording and add IPv6 addresses

8eaac38

This reflects the review feedback and should not induce any technical changes. Signed-off-by: Pierre R. Mai <[email protected]>

pmai commented Aug 8, 2024

View reviewed changes

t-sommer approved these changes Aug 9, 2024

View reviewed changes

PTaeuberDS approved these changes Aug 9, 2024

View reviewed changes

pmai merged commit 86b55db into main Aug 13, 2024

pmai deleted the feature/multi-a2l-support-struct branch August 13, 2024 11:41

Conversation

pmai commented Jun 19, 2024

Uh oh!

chrbertsch commented Jun 25, 2024

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!

t-sommer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PTaeuberDS commented Jul 30, 2024

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!

pmai commented Jul 30, 2024

Uh oh!

chrbertsch commented Jul 30, 2024

Uh oh!

pmai left a comment

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!

6 participants