Bidirectional feature group connection produces incorrect connection instance

[thari]

Summary

Osate produces connection instances with both source and destination being the same port.

Expected and Current Behavior

Osate instantiation of a system with bi-directional feature group connections between parent and sub components produces the backward connection instance incorrectly. It is supposed to make a connection instance from a leaf level component out port to the top level component's out port. However, Osate connects back to the leaf level components out port.

Steps to Reproduce

1. Instantiate the toplevel.i system from the provided model
2. Check for the connection instances
3. The second connection instance's source and destination are same

package FeatureGroupTest
public
	
	feature group abstract_fg
		
	end abstract_fg;
	
	feature group refined_fg extends abstract_fg
		features 
			test_feature1 : in data port;
			test_feature2 : out data port;
	end refined_fg;
	
	system abstract_toplevel
		features 
			fg_port : feature group abstract_fg;
	end abstract_toplevel;
	
	system toplevel extends abstract_toplevel
		features
			fg_port : refined to feature group refined_fg;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end toplevel;
	
	system implementation toplevel.i
		subcomponents
			sub : process subsystem.i;
		connections
			conn:  feature group fg_port <-> sub.fg_port;
	end toplevel.i;
	
	process abstract_subsystem
		features 
			fg_port : feature group abstract_fg;
	end abstract_subsystem;
	
	process subsystem extends abstract_subsystem
		features 
			fg_port : refined to feature group refined_fg;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end subsystem;
	
	process implementation subsystem.i
		subcomponents
			th : thread subsub;
		connections
			conn2 : feature group fg_port <->th.fg_port;
	end subsystem.i;
	
	thread subsub
		features 
			fg_port : feature group refined_fg;
	end subsub;
	
end FeatureGroupTest;

Other Observation

1. Even when the connection instance of two different ports are made, the connection reference with same source and destination exists

2. Bug exist with and without the feature group refinement

Environment

• OSATE Version: 2.7.0, 2.7.1 and the latest master branch development environment
• Operating System: Mac

[AaronGreenhouse]

Updated the example to see what happens when the connections are declared in the other direction (that is, change the roles of the src and dest in the connection declarations):

package FeatureGroupTest
public
	
	feature group abstract_fg
		
	end abstract_fg;
	
	feature group refined_fg extends abstract_fg
		features 
			test_feature1 : in data port;
			test_feature2 : out data port;
	end refined_fg;
	
	system abstract_toplevel
		features 
			fg_port : feature group abstract_fg;

			fg_port2 : feature group abstract_fg;
	end abstract_toplevel;
	
	system toplevel extends abstract_toplevel
		features
			fg_port : refined to feature group refined_fg;
			
			fg_port2 : refined to feature group refined_fg;			
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end toplevel;
	
	system implementation toplevel.i
		subcomponents
			sub : process subsystem.i;
		connections
			conn:  feature group fg_port <-> sub.fg_port;

			conn_other:  feature group sub.fg_port2 <-> fg_port2;
	end toplevel.i;
	
	process abstract_subsystem
		features 
			fg_port : feature group abstract_fg;

			fg_port2 : feature group abstract_fg;
	end abstract_subsystem;
	
	process subsystem extends abstract_subsystem
		features 
			fg_port : refined to feature group refined_fg;

			fg_port2 : refined to feature group refined_fg;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end subsystem;
	
	process implementation subsystem.i
		subcomponents
			th : thread subsub;
		connections
			conn2 : feature group fg_port <->th.fg_port;

			conn2_other : feature group th.fg_port2 <-> fg_port2;
	end subsystem.i;
	
	thread subsub
		features 
			fg_port : feature group refined_fg;

			fg_port2 : feature group refined_fg;
	end subsub;
	
end FeatureGroupTest;

The results are interesting in that the error doesn't exists in the other direction:

[AaronGreenhouse]

This is true for regular bidirectional port connections as well. That is, this is not a feature group problem.

package BiDirPortTest
public
	
	system abstract_toplevel
		features 
			fg_port : feature ;

			fg_port2 : feature ;
	end abstract_toplevel;
	
	system toplevel extends abstract_toplevel
		features
			fg_port : refined to in out event port;
			
			fg_port2 : refined to in out event port;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end toplevel;
	
	system implementation toplevel.i
		subcomponents
			sub : process subsystem.i;
		connections
			conn:  port fg_port <-> sub.fg_port;

			conn_other:  port sub.fg_port2 <-> fg_port2;
	end toplevel.i;
	
	process abstract_subsystem
		features 
			fg_port : feature;

			fg_port2 :feature;
	end abstract_subsystem;
	
	process subsystem extends abstract_subsystem
		features 
			fg_port : refined to in out event port;

			fg_port2 : refined to in out event port;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end subsystem;
	
	process implementation subsystem.i
		subcomponents
			th : thread subsub;
		connections
			conn2 : port fg_port <->th.fg_port;

			conn2_other : port th.fg_port2 <-> fg_port2;
	end subsystem.i;
	
	thread subsub
		features 
			fg_port : in out event port;

			fg_port2 : in out event port;
	end subsub;
	
end BiDirPortTest;

[AaronGreenhouse]

Simplified the broken port connection example:

package BiDirPortSimple
public
	system toplevel
		features
			fg_port : in out event port;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end toplevel;
	
	system implementation toplevel.i
		subcomponents
			sub : process subsystem.i;
		connections
			conn:  port fg_port <-> sub.fg_port;
	end toplevel.i;
	
	process subsystem 
		features 
			fg_port : in out event port;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end subsystem;
	
	process implementation subsystem.i
		subcomponents
			th : thread subsub;
		connections
			conn2 : port fg_port <->th.fg_port;
	end subsystem.i;
	
	thread subsub
		features 
			fg_port : in out event port;
	end subsub;	
end BiDirPortSimple;

[AaronGreenhouse]

EDIT: This went away after I restarted OSATE. Strange. Going to keep this comment for now in case it comes back.

More weirdness. I replaced the bidirectional connection in the above example with two directional connections:

package BiDirPortSimple2
public
	system toplevel
		features
			fg_port : in out event port;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end toplevel;
	
	system implementation toplevel.i
		subcomponents
			sub : process subsystem.i;
		connections
			--conn:  port sub.fg_port -> fg_port;
			conn_other:  port fg_port -> sub.fg_port;
	end toplevel.i;
	
	process subsystem 
		features 
			fg_port : in out event port;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end subsystem;
	
	process implementation subsystem.i
		subcomponents
			th : thread subsub;
		connections
			--conn2 : port th.fg_port -> fg_port;
			conn2_other : port fg_port -> th.fg_port;
	end subsystem.i;
	
	thread subsub
		features 
			fg_port : in out event port;
	end subsub;	
end BiDirPortSimple2;

This semantic connection from inside to the outside (sub component to the border of the toplevel) is fine. But the semantic connection from the outside to the inside (from the edge of toplevel to the subcomponent) is missing links to the declared connections.

However, if I comment out the declaration of conn and conn2 then the connection from outside to the inside is fine:

[thari]

osate2/core/org.osate.aadl2.instantiation/src/org/osate/aadl2/instantiation/CreateConnectionsSwitch.java

Line 585 in cf05234

boolean opposite = false;

Wondering if this initialization is correct. I am not sure in which situation where one segment of the connection instance is towards the destination and other is towards the source. I believe the recursive call should follow the same direction therefore the goOpposite argument must be equal to opposite parameter.

[thari]

The connection instantiation process starts from toplevel system instantiation for building connection instance fg_port -> sub.th.fg_port .
Similarly, it starts from th to build the connection instance sub.th.fg_port->fg_port.

After adding the conn2 to the connection instance,

osate2/core/org.osate.aadl2.instantiation/src/org/osate/aadl2/instantiation/CreateConnectionsSwitch.java

Line 592 in cf05234

FeatureInstance nextDstFi = nextCi.findFeatureInstance(nextDstFeature);

The above line searches for a feature instance for the feature sub.fg_port in the component toplevel. This works when the goOpposite is false.
In our case, when the goOpposite is true, it is suppose to search in “sub” component to match the rhs of conn.

Changing nextCi to (goOpposite ? ci : nextCi) fixes the issue.

Bigger issue:

osate2/core/org.osate.aadl2/src/org/osate/aadl2/instance/impl/ComponentInstanceImpl.java

Line 891 in cf05234

if (fi.getName().equalsIgnoreCase(feature.getName())) {

findFeatureInstance should check if “this”(ComponentInstance) is an instance of the component containing the feature.

[AaronGreenhouse]

I'm still evaluating this, and I'm not saying that this does or doesn't fix the problem, but I'm not sure this is correct diagnosis.

I changed all the port names to be unique and the problem goes away.

package BiDirPortSimple_samenames
public
	system toplevel
		features
			p : in out event port;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end toplevel;
	
	system implementation toplevel.i
		subcomponents
			sub : process subsystem.i;
		connections
			conn:  port p <-> sub.p;
	end toplevel.i;
	
	process subsystem 
		features 
			p : in out event port;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end subsystem;
	
	process implementation subsystem.i
		subcomponents
			th : thread th;
		connections
			conn2 : port p <->th.p;
	end subsystem.i;
	
	thread th
		features 
			p : in out event port;
	end th;
end BiDirPortSimple_samenames;

package BiDirPortSimple_uniquenames
public
	system toplevel
		features
			tl_p : in out event port;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end toplevel;
	
	system implementation toplevel.i
		subcomponents
			sub : process subsystem.i;
		connections
			conn:  port tl_p <-> sub.ss_p;
	end toplevel.i;
	
	process subsystem 
		features 
			ss_p : in out event port;
		properties
			Classifier_Substitution_Rule => Type_Extension; --HT
	end subsystem;
	
	process implementation subsystem.i
		subcomponents
			th : thread th;
		connections
			conn2 : port ss_p <->th.th_p;
	end subsystem.i;
	
	thread th
		features 
			th_p : in out event port;
	end th;
end BiDirPortSimple_uniquenames;

Here we see on the left that the version where all the ports have the same name has a bad reverse connection instance. On the right we see that the version with unique names has a correct reverse connection instance.

[thari]

Yes that is what I mentioned in the "Bigger Issue" section. There are two problems:

1. The direction of traversal through the hierarchy(top-down/bottom-up) flips in mid way
2. Feature and FeatureInstance are matched using feature name without checking if the ComponentInstance is built from Component containing the Feature

Assume I have components A and B with feature named p1. Additionally, Ainst and Binst are the component instance built from component A and B. Now, if I call Ainst. findFeatureInstance(B.p1) it will return Ainst.p1 because the name p1 matches.

Fixing any one will fix this problem, but there will be other issues if both are not fixed.
My fix address only the first bullet.

[AaronGreenhouse]

Been studying this most of the afternoon. I am almost certain that there is nothing wrong with line 592. It belongs to a block of code that expects failure and is trying to determine which way the connection is going. I think the real and only problem is with the name-based feature lookup.

[AaronGreenhouse]

For sanity checking I created test cases that verify that the logic for computing opposite is correct. I am convinced that it is.

Thus the sole problem here is that ComponentInstanceImpl.findFeatureInstance() relies only on name matching. Looking at the method we see that name matching was added to deal with feature refinement. Makes sense, and works just fine in cases where you know the feature is supposed to be part of the component. But the way the method is being used here in CreateConnectionsSwitch.appendSegment() it is being used to test if the feature is part of the component. So a name-based test is not sufficient.

[AaronGreenhouse]

The problematic call to findFeatureInstance() occurs here, where we are testing tosee if the the feature appears in nextCi:

									if (nextConnDest instanceof Feature) {
										final Feature nextConnDstFeature = (Feature) nextConnDest;
										FeatureInstance nextConnDstFi = nextCi.findFeatureInstance(nextConnDstFeature);

										/*
										 * If we find the connection destination in the containing component instance, then
										 * the connection is a normal (not reversed) traversal of the connection. The
										 * value of `opposite` will stay `false`.
										 */
										if (nextConnDstFi == null) {
											/*
											 * Didn't find the next destination in the containing component, so the question
											 * still is, is the destination in a sibling subcomponent or is it a reversed
											 * traversal from the containing component, or even a reversed traversal from
											 * a sibling subcomponent?
											 */
											// next goes across, maybe?
											Context nextConnDstCtx = nextConn.getAllDestinationContext();

											if (nextConnDstCtx instanceof Subcomponent) {
												final ComponentInstance nextConnDstSubi = nextCi
														.findSubcomponentInstance((Subcomponent) nextConnDstCtx);
												nextConnDstFi = nextConnDstSubi.findFeatureInstance(nextConnDstFeature);
											}

											if (nextConnDstFi != null) {
												/*
												 * Opposite is true if the dest of the next connection the same feature instance as the
												 * dest of the current connection.
												 */
												opposite = ci.findFeatureInstance(toFeature) == nextConnDstFi;
											}
										}
									}

I think this test is actually unnecessary, and we could just have a test based on the looking up the feature in the destination context as done inside the conditional branch.

But after a discussion with @lwrage, we have decided the best thing to do is to fix findFeatureInstance as it is definitely wrong and could be messing up things in other places.

[AaronGreenhouse]

Looked at the rest of the findXXXInstance() methods in ComponentInstanceImpl:

• findSubcomponentInstance() uses names
• findConnectionInstance() looks okay: does use refinement
• findEndToEndFlowInstance() does not use refinement but should
• findFlowSpecInstance() uses names
• findModeInstance() looks okay since modes cannot be refined
• findModeTransitionInstance() looks okay since mode transitions cannot be refined

So I need to fix

• findFeatureInstance()
• findSubcomponentInstance()
• findEndToEndFlowInstance()
• findFlowSpecInstance()

Add test for

• findConnectionInstance

[AaronGreenhouse]

Fixed

• findFeatureInstance()
• findSubcomponentInstance()
• findEndToEndFlowInstance()
• findFlowSpecInstance()

and added JUnit tests for them.

Added a JUnit test for

• findConnectionInstance

[AaronGreenhouse]

Added unit tests for the original test case (FeatureGroupTest) and the simplified port tests.