ZeligsoftDev / CX4CBDDS

CX4CBDDS component modeling and generation tool
Apache License 2.0
8 stars 5 forks source link

Issue when a component has both a DDS_write and DDS_listen port to same dataspace with content filter #463

Closed emammoser closed 1 year ago

emammoser commented 1 year ago

Issue and tracking information

Developer's time Estimated effort to fix (hours):

Developer's Actual time spent on fix (hours)

Issue reporter to provide a detailed description of the issue in the space below

If a component has both a DDS_write and DDS_listen port to same dataspace and that dataspace has a content filter, a situation can occur where the DDS Listen port does not have a content filter set in the deployment plan. As far as I can tell, if a component has 2 ports pointed to the same dataspace, only one instance is created (instead of two) and the one that is created is capable of serving both ports. This makes sense from a middleware perspective. However, if the DDS_Write instance is created (instead of the DDS Listen), then no content filter settings are generated (as we recently removed content filter settings that wouldn't apply to DDS_Write ports).

I can hand modify the deployment plan in the .uml file with a text editor to move around certain ID's in a list to make it generate the DDS_Listen instance over the DDS_Write port, which does solve this problem (i.e. the instance has all of the content filter settings).

I am adding a slightly modified BPS example that shows the issue in the generated CDP files. All I did was add a DDS_Listen port to the Publisher component, connect it to the dataspace, and add a content filter settings to the data space in the deployment plan

eposse commented 1 year ago

Hi. I'm not sure I understand what exactly is missing in the generated CDP. I see in the instance BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn-to-basicSubscriber_comp_testDataSub-at-node2.BasicSubscriber_proc the config property push_consumer_filter with the filter expression. Is this not it? If not, what exactly is expected?

Are you saying perhaps that an instance BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn-to-basicPublisher_comp_testDataSub-at-node1.BasicPublisher_proc (for the testDataSub port in the publisher) is missing and that this instance should have the same as the one in the subscriber?

emammoser commented 1 year ago

Are you saying perhaps that an instance BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn-to-basicPublisher_comp_testDataSub-at-node1.BasicPublisher_proc (for the testDataSub port in the publisher) is missing and that this instance should have the same as the one in the subscriber?

This, exactly. Last year (I think), we had your team remove the generation of content filter settings to DDS Write Port entities. This was the correct move to make. However, now we are seeing a scenario where a component has both a DDS Write port and DDS listen port of the exact same type connected to the same dataspace. In this scenario the CDP will only generate a single instance for that component that encompasses both of those ports (which all checks out). However, this can cause the settings for the content filter to not be added to the event connector instance if the DDS listen port was second to be added to the component.

eposse commented 1 year ago

So, to be certain, we should generate the following instance and connections for this example, while making sure that we do not generate push_consumer_filter_config configProperty for the DDS Write port (testDataPub), is this right?

  <instance xmi:id="BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn-to-basicPublisher_comp_testDataSub-at-node1.BasicPublisher_proc">
    <name>BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn_to_basicPublisher_comp@node1.BasicPublisher_proc</name>
    <node>node1</node>
    <source/>
    <implementation xmi:idref="SNA_Examples.TestData_conn.DDS_Event"/>
    <configProperty>
      <name>topic_name</name>
      <value>
        <type>
          <kind>tk_string</kind>
        </type>
        <value>
          <string>SNA_Examples::TestData</string>
        </value>
      </value>
    </configProperty>
    <configProperty>
      <name>domain_id</name>
      <value>
        <type>
          <kind>tk_long</kind>
        </type>
        <value>
          <long>25</long>
        </value>
      </value>
    </configProperty>
    <configProperty>
      <name>qos_profile</name>
      <value>
        <type>
          <kind>tk_string</kind>
        </type>
        <value>
          <string>SNA_Examples#BasicPubSub</string>
        </value>
      </value>
    </configProperty>
  </instance>

  <connection>
    <name>BasicPubSub_asm.basicPublisher_comp.testDataSub::BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn_to_basicPublisher_comp@node1.BasicPublisher_proc.push_consumer_data_listener</name>
    <internalEndpoint>
      <portName>testDataSub_data_listener</portName>
      <provider>true</provider>
      <kind>Facet</kind>
      <instance xmi:idref="BasicPubSub.assemblies.Deployment.descriptors.BasicPubSub.basicPublisher_comp"/>
    </internalEndpoint>
    <internalEndpoint>
      <portName>push_consumer_data_listener</portName>
      <provider>false</provider>
      <kind>SimplexReceptacle</kind>
      <instance xmi:idref="BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn-to-basicPublisher_comp_testDataSub-at-node1.BasicPublisher_proc"/>
    </internalEndpoint>
  </connection>

  <connection>
    <name>BasicPubSub_asm.basicPublisher_comp.testDataSub::BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn_to_basicPublisher_comp@node1.BasicPublisher_proc.push_consumer_status</name>
    <internalEndpoint>
      <portName>testDataSub_status</portName>
      <provider>true</provider>
      <kind>Facet</kind>
      <instance xmi:idref="BasicPubSub.assemblies.Deployment.descriptors.BasicPubSub.basicPublisher_comp"/>
    </internalEndpoint>
    <internalEndpoint>
      <portName>push_consumer_status</portName>
      <provider>false</provider>
      <kind>SimplexReceptacle</kind>
      <instance xmi:idref="BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn-to-basicPublisher_comp_testDataSub-at-node1.BasicPublisher_proc"/>
    </internalEndpoint>
  </connection>

  <connection>
    <name>BasicPubSub_asm.basicPublisher_comp.testDataSub::BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn_to_basicPublisher_comp@node1.BasicPublisher_proc.push_consumer_data</name>
    <internalEndpoint>
      <portName>testDataSub_data</portName>
      <provider>false</provider>
      <kind>SimplexReceptacle</kind>
      <instance xmi:idref="BasicPubSub.assemblies.Deployment.descriptors.BasicPubSub.basicPublisher_comp"/>
    </internalEndpoint>
    <internalEndpoint>
      <portName>push_consumer_data</portName>
      <provider>true</provider>
      <kind>Facet</kind>
      <instance xmi:idref="BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn-to-basicPublisher_comp_testDataSub-at-node1.BasicPublisher_proc"/>
    </internalEndpoint>
  </connection>

  <connection>
    <name>BasicPubSub_asm.basicPublisher_comp.testDataSub::BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn_to_basicPublisher_comp@node1.BasicPublisher_proc.push_consumer_data_control</name>
    <internalEndpoint>
      <portName>testDataSub_data_control</portName>
      <provider>false</provider>
      <kind>SimplexReceptacle</kind>
      <instance xmi:idref="BasicPubSub.assemblies.Deployment.descriptors.BasicPubSub.basicPublisher_comp"/>
    </internalEndpoint>
    <internalEndpoint>
      <portName>push_consumer_data_control</portName>
      <provider>true</provider>
      <kind>Facet</kind>
      <instance xmi:idref="BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn-to-basicPublisher_comp_testDataSub-at-node1.BasicPublisher_proc"/>
    </internalEndpoint>
  </connection>

  <connection>
    <name>BasicPubSub_asm.basicPublisher_comp.testDataSub::BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn_to_basicPublisher_comp@node1.BasicPublisher_proc.push_consumer_dds_entity</name>
    <internalEndpoint>
      <portName>testDataSub_dds_entity</portName>
      <provider>false</provider>
      <kind>SimplexReceptacle</kind>
      <instance xmi:idref="BasicPubSub.assemblies.Deployment.descriptors.BasicPubSub.basicPublisher_comp"/>
    </internalEndpoint>
    <internalEndpoint>
      <portName>push_consumer_dds_entity</portName>
      <provider>true</provider>
      <kind>Facet</kind>
      <instance xmi:idref="BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn-to-basicPublisher_comp_testDataSub-at-node1.BasicPublisher_proc"/>
    </internalEndpoint>
  </connection>

  <connection>
    <name>BasicPubSub_asm.basicPublisher_comp.testDataSub::BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn_to_basicPublisher_comp@node1.BasicPublisher_proc.push_consumer_filter_config</name>
    <internalEndpoint>
      <portName>testDataSub_filter_config</portName>
      <provider>false</provider>
      <kind>SimplexReceptacle</kind>
      <instance xmi:idref="BasicPubSub.assemblies.Deployment.descriptors.BasicPubSub.basicPublisher_comp"/>
    </internalEndpoint>
    <internalEndpoint>
      <portName>push_consumer_filter_config</portName>
      <provider>true</provider>
      <kind>Facet</kind>
      <instance xmi:idref="BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn-to-basicPublisher_comp_testDataSub-at-node1.BasicPublisher_proc"/>
    </internalEndpoint>
  </connection>
emammoser commented 1 year ago

I don't know that we are on the same page and I am not sure I agree with the above CDP snippet. I will see if I can clarify and if not, I can draw a picture.

In a model, when a Component has a port, that port usually has to connect to a dataspace. In the generated CDP, an instance is created for the component, and it appears that an instance is created for a port x Dataspace combination. However, if a component has two different ports that connect to the same dataspace, it appears that only a single instance is created for both of those components, and that single instance services all of the ports on the component that are connected to that single dataspace. As far as I can tell, this works. I cannot speak to whether or not this is better or worse than creating two separate instances, one for each port, although I am open to using two or more instances to service each individual port on a component connected to the same dataspace.

Currently, if we have a DDS Dataspace with a push consumer filter configured, all instances for DDS Write ports do not have the push consumer filter config values in the CDP file (correct) and all instances for DDS Listen ports do have them (also correct).

What we are seeing is that if a component has both a DDS Write and a DDS Listen port connected to the same dataspace, they will only have a single instance for both ports (see above on why). This is okay, however, depending on a hidden ordering of the ports in the model, the instance may, or may not, have the push consumer filter config options. This is not correct as we would need those filter settings for the DDS Listen entities of that instance.

eposse commented 1 year ago

Hi. First, to ensure we are on the same page, here's a simplified list of what is currently generated in the CDP:

An instance for each "container" process:

1) BasicPublisher_proc 2) BasicSubscriber_proc

An instance for each assembly part:

3) basicPublisher_comp 4) basicSubscriber_comp

An instance for each (CCM) connector (*) between a dataspace and a port (in some component):

5) TestData_conn-to-basicPublisher_comp_testDataPub-at-node1.BasicPublisher_proc 6) TestData_conn-to-basicSubscriber_comp_testDataSub-at-node2.BasicSubscriber_proc

(*) Small note about terminology here: "connector" may be a bit ambiguous, as in AXCIOMA, if my understanding is right, and correct me if I'm wrong, a "connector" would be a "DataSpace", which is visualized in assembly diagrams as a black circle shape and has a type such as DDS_Event. Here I'm using "connector" in the CCM/UML sense, as the line that connects to "connectable" elements, such as a port and a dataspace. So, for example, a connection from publisher to subscriber consists of ports testDataPub, the data space, testDataSub, and two (CCM) connectors, one between testDataPub and the data space, and a second one between the data space and testDataSub. In the remainder, when I use "connector" I mean a (CCM) connector, rather than a dataspace.

Each instance contains configuration properties. These are defined in the Properties View/CX Tab, and for assembly parts, they come from the type of the part, which is a Component Definition (e.g. BasicPublisher), while for connectors, they come from the Connector Definition (e.g. DDS_Event).

For each par (part, port, config_property) and (dataspace-to-port-conn, push_consumer, config_property), a connection is made, e.g. a connection between

    (basicPublisher_comp, testDataSub, data_listener) 

and

    (TestData_conn-to-basicSubscriber_comp_testDataSub-at-node2.BasicSubscriber_proc, push_consumer, data_listener)

I agree that in the scenario you describe, where the component has two ports connected to the dataspace, only one instance of the connector is generated.

I understand that if one of the ports is a DDS_Write, the corresponding instance (between the port and the dataspace) doesn't have the push consumer filter config properties, while one for a DDS_Listen port has them.

I assume that you still want this behaviour for instances of DDS_Write ports (not having the filter properties) and instances of DDS_Listen ports (have the config properties). Is this correct?

Then, the question is what exactly should we generate.

If my assumption is correct, then I see two possible solutions:

A) Continue to generate only one instance of the port-to-dataspace connector such that if at least one of the ports in the part is of type DDS_Listen, then generate the filter config properties, but if it is of type DDS_Write, do not generate it.

or

B) Generate separate instances for each port-to-dataspace connector, so that the DDS_Write instance doesn't contain the filter config property, but the DDS_Listen instance does.

Personally I think the most appropriate solution is B), as the first one seems very error-prone, without a clear match between what is in the model, and what is generated. Furthermore, I don't know what the CCM middleware will do if we don't explicitly have instances for each port. I'm guessing it will depend on whichever implementation you have behind the scenes.

The CPD excerpts that I sent before where my attempt to show B), adding a separate instance for

    TestData_conn-to-basicPublisher_comp_testDataSub-at-node1.BasicPublisher_proc

i.e. an instance corresponding to the testDataSub (DDS_Listen) port in the publisher, and the rest where the connections between this instance and the part/port/config_property.

emammoser commented 1 year ago

I agree that option B appears to be the best solution. I will say that Option A does seem to work and the CCM middleware appears to be completely happy with a single instance for multiple ports (which sort of makes sense from a code point of view), however, I am not sure that it is worth having a more complicated and error prone design for such an outlier of a use case.

If we go with B), which is a separate instance for each port-to-dataspace connector, I can't see any issues that would occur in the future other than one more "middleware class connector" instantiation in our runtime, which is not a real concern to us.

The CPD excerpts that I sent before where my attempt to show B), adding a separate instance for TestData_conn-to-basicPublisher_comp_testDataSub-at-node1.BasicPublisher_proc i.e. an instance corresponding to the testDataSub (DDS_Listen) port in the publisher, and the rest where the connections between this instance and the part/port/config_property.

Your CDP excerpt did not have the filter config values, which I would expect for a DDS Listen port connected to a dataspace that did have push consumer configuration values. Maybe this is where my confusion came from.

eposse commented 1 year ago

Your CDP excerpt did not have the filter config values, which I would expect for a DDS Listen port connected to a dataspace that did have push consumer configuration values. Maybe this is where my confusion came from.

Ah! You are right. I meant to put them there. My (copy-and-paste) mistake.

Ok, we'll go for option B then.

emammoser commented 1 year ago

Great, thank you. I personally haven't tested option B, but like I said, I don't see any reason it wouldn't work.

eposse commented 1 year ago

I have a question about the instances names and ids. I remember that some time ago we replaced the ids which were UUIDs with symbolic, meaningful names, but we seem to have left the actual names unchanged, and there is a bit of a mismatch between them. For example, this one:

  <instance xmi:id="BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn-to-basicPublisher_comp_testDataPub-at-node1.BasicPublisher_proc">
    <name>BasicPubSub.assemblies.BasicPubSub_asm.BasicPubSub_asm.TestData_conn_to_basicPublisher_comp@node1.BasicPublisher_proc</name>

The main difference is that the name doesn't include the port name. Now, with the solution I'm working on, the port name becomes relevant. The id will have it but currently the name doesn't, so we would get two different instances with different ids but the same name. I imagine this is not ideal, and I don't know if it has any consequences for you, but shouldn't we make the name the same as the id to avoid ambiguities?

emammoser commented 1 year ago

Yes, I would agree that the name should be the same as the ID. I have done that by hand with other elements without issues.

eposse commented 1 year ago

Just to give you an update, the latest commit seems to work, but I'm still testing, in particular cases with nested components.

eposse commented 1 year ago

It looks like it works. You can give it a try when the build is done. You can get the build here.

I'm attaching a new model that includes the scenario that we have been discussing, as well as nested parts. I included CDPs generated before and after the changes. It now generates separate instances for each port, with the relevant config properties, and all the relevant connections.

Nevertheless, I realized that if you set config properties in delegation ports (the "outer_*" ports in the model below), nothing about them is generated. Is this correct?

AB.zip

emammoser commented 1 year ago

I tested this locally by running the CDP generated in the original reproducer model. The example was able to run as expected.

Nevertheless, I realized that if you set config properties in delegation ports (the "outer_*" ports in the model below), nothing about them is generated. Is this correct?

Yes, I believe any configuration properties set on delegate ports are ignored/not generated.

We would like this fix put on the 2.4 and 2.3 maintenance streams. Thank you.

eposse commented 1 year ago

Ok, I'll add the fix to the maintenance branches.

eposse commented 1 year ago

Ok, it has been merged to the papyrus branch as well as the 2.3 and 2.4 maintenance branches.

Is it ok to close it now?

emammoser commented 1 year ago

Yes, we can close this issue