Shared subprogram access yields too many connection instances in instance model

[AaronGreenhouse]

This example is derived from the example in Issue #1941. When the system below is instantiated, the process instance MyP has three connection instances, two of which should not be generated. Furthermore, it should have an additional connection instance that is missing.

package ExtraConnections
public
	subprogram ComputeAverage
	end ComputeAverage;

	thread T1
		features
			shared_sub: provides subprogram access ComputeAverage;
	end T1;

	thread implementation T1.impl
		subcomponents
			CompAvg: subprogram ComputeAverage;
		connections
			ac1: subprogram access CompAvg <-> shared_sub;
	end T1.impl;

	thread T2
		features
			ext_sub: requires subprogram access ComputeAverage;
	end T2;

	thread implementation T2.impl
	end T2.impl;

	process p1
	end p1;

	process implementation p1.impl
		subcomponents
			MyT1: thread T1.impl;
			MyT2: thread T2.impl;
		connections
			ac2: subprogram access MyT1.shared_sub <-> MyT2.ext_sub;
	end p1.impl;

	system SubprogramExample19
	end SubprogramExample19;

	system implementation SubprogramExample19.impl
		subcomponents
			MyP: process p1.impl;
	end SubprogramExample19.impl;
end ExtraConnections;

The instance model has the connection instances (with the given connection references):

Only the second connection instance MyT1.CompAvg -> MyT2.ext_sub should exist. The other two connections are incomplete nonsense.

Furthermore, the connection between CompAvg and ext_sub should be bidirectional, so there should be a connection instances MyT2.ext_sub -> MyT1.CompAvg, but there currently is not one.

[AaronGreenhouse]

Further testing indicates the same problems for shared data connections, but that shared bus connections work as desired:

package WithDataConnections
public
	subprogram ComputeAverage
	end ComputeAverage;

	data D
	end D;
	
	bus B
	end B;

	system s1
		features
			shared_bus: provides bus access B;
	end s1;

	system implementation s1.impl
		subcomponents
			myBus: bus B;
		connections
			bc1: bus access myBus <-> shared_bus;
	end s1.impl;
	
	system s2
		features
			ext_bus: requires bus access B;
	end s2;
	
	system implementation s2.impl
	end s2.impl;
	
	system MySystem
	end MySystem;
	
	system implementation MySystem.impl
		subcomponents
			myS1: system s1.impl;
			myS2: system s2.impl;
		connections
			bc2: bus access myS1.shared_bus <-> myS2.ext_bus;
	end MySystem.impl;
	
	thread T1
		features
			shared_sub: provides subprogram access ComputeAverage;
			shared_data: provides data access D;
	end T1;

	thread implementation T1.impl
		subcomponents
			CompAvg: subprogram ComputeAverage;
			myData: data D;
		connections
			ac1: subprogram access CompAvg <-> shared_sub;
			dc1: data access myData <-> shared_data;
	end T1.impl;

	thread T2
		features
			ext_sub: requires subprogram access ComputeAverage;
			ext_data: requires data access D;
	end T2;

	thread implementation T2.impl
	end T2.impl;

	process p1
	end p1;

	process implementation p1.impl
		subcomponents
			MyT1: thread T1.impl;
			MyT2: thread T2.impl;
		connections
			ac2: subprogram access MyT1.shared_sub <-> MyT2.ext_sub;
			dc2: data access MyT1.shared_data <-> MyT2.ext_data;
	end p1.impl;

	system SubprogramExample19
	end SubprogramExample19;

	system implementation SubprogramExample19.impl
		subcomponents
			MyP: process p1.impl;
			MyS: system MySystem.impl;
	end SubprogramExample19.impl;
end WithDataConnections;

[AaronGreenhouse]

More tests:

• subprogram groups don't work (like subprogram and data)
• virtual buses do work (like bus)

Complete test system:

package TestAccessConnections
public
	subprogram ComputeAverage
	end ComputeAverage;

	subprogram group SubGroup
	end SubGroup;

	data D
	end D;
	
	bus B
	end B;
	
	virtual bus VB
	end VB;

	system s1
		features
			shared_bus: provides bus access B;
			shared_VB: provides virtual bus access VB;
	end s1;

	system implementation s1.impl
		subcomponents
			myBus: bus B;
			myVB: virtual bus VB;
		connections
			bc1: bus access myBus <-> shared_bus;
			vbc1: virtual bus access myVB <-> shared_VB;
	end s1.impl;
	
	system s2
		features
			ext_bus: requires bus access B;
			ext_VB: requires virtual bus access VB;
	end s2;
	
	system implementation s2.impl
	end s2.impl;
	
	system MySystem
	end MySystem;
	
	system implementation MySystem.impl
		subcomponents
			myS1: system s1.impl;
			myS2: system s2.impl;
		connections
			bc2: bus access myS1.shared_bus <-> myS2.ext_bus;
			vbc2: virtual bus access myS1.shared_vb <-> myS2.ext_vb;
	end MySystem.impl;
	
	thread T1
		features
			shared_sub: provides subprogram access ComputeAverage;
			shared_subg: provides subprogram group access SubGroup;
			shared_data: provides data access D;
	end T1;

	thread implementation T1.impl
		subcomponents
			CompAvg: subprogram ComputeAverage;
			SubG: subprogram group SubGroup;
			myData: data D;
		connections
			ac1: subprogram access CompAvg <-> shared_sub;
			sgc1: subprogram group access SubG <-> shared_subg;
			dc1: data access myData <-> shared_data;
	end T1.impl;

	thread T2
		features
			ext_sub: requires subprogram access ComputeAverage;
			ext_subg: requires subprogram group access SubGroup;
			ext_data: requires data access D;
	end T2;

	thread implementation T2.impl
	end T2.impl;

	process p1
	end p1;

	process implementation p1.impl
		subcomponents
			MyT1: thread T1.impl;
			MyT2: thread T2.impl;
		connections
			ac2: subprogram access MyT1.shared_sub <-> MyT2.ext_sub;
			sgc2: subprogram group access MyT1.shared_subg <-> MyT2.ext_subg;
			dc2: data access MyT1.shared_data <-> MyT2.ext_data;
	end p1.impl;

	system SubprogramExample19
	end SubprogramExample19;

	system implementation SubprogramExample19.impl
		subcomponents
			MyP: process p1.impl;
			MyS: system MySystem.impl;
	end SubprogramExample19.impl;
end TestAccessConnections;

[AaronGreenhouse]

There are two main places in CreateConnectionsSwitch that need to be update, and the correspond with the two different directions of the connection. I found these places pretty quickly by just searching for places where there are tests for BUS.

1. In method instantiateConnections() there is the block of code

					if (hasOutgoingFeatureSubcomponents
							&& ((cat != THREAD && cat != PROCESSOR && cat != DEVICE && cat != VIRTUAL_PROCESSOR)
									// in case of a provides bus access we want to
									// start from the bus.
									|| ((cat == PROCESSOR || cat == DEVICE || cat == MEMORY)
											&& feature instanceof BusAccess
											&& ((BusAccess) feature).getKind() == AccessType.PROVIDES)) {
						connectedInside = isConnectionEnd(insideSubConns, feature);
						destinationFromInside = isDestination(insideSubConns, feature);
					}

This clearly captures all the possibilities of provides bus access features. (This is the case that currently works correctly.) So this needs to be expanded to capture the other provides acces features.

2. In the method appendSegment() there is the block of code

							if (((toImpl instanceof ProcessorImplementation || toImpl instanceof DeviceImplementation
									|| toImpl instanceof MemoryImplementation)
									&& !(toEnd instanceof BusAccess
											&& ((BusAccess) toEnd).getKind() == AccessType.PROVIDES))) {
								final ConnectionInfo clone = connInfo.cloneInfo();
								clone.complete = true;
								finalizeConnectionInstance(ci, clone, toFi);
							}

Again, this filters out the case of provides bus access features. This also needs to be updated to handle the other provides access features.

I started by trying to fix data access connections. It is definitely true that these two locations must be updated, and I have done so as

					if (hasOutgoingFeatureSubcomponents
							&& ((cat != THREAD && cat != PROCESSOR && cat != DEVICE && cat != VIRTUAL_PROCESSOR)
									// in case of a provides bus access we want to
									// start from the bus.
									|| ((cat == PROCESSOR || cat == DEVICE || cat == MEMORY)
											&& feature instanceof BusAccess
											&& ((BusAccess) feature).getKind() == AccessType.PROVIDES)
									// in case of a provides data access we want to
									// start from the data.
									|| ((cat == DATA || cat == THREAD || cat == THREAD_GROUP || cat == PROCESS)
											&& feature instanceof DataAccess
											&& ((DataAccess) feature).getKind() == AccessType.PROVIDES))) {
						connectedInside = isConnectionEnd(insideSubConns, feature);
						destinationFromInside = isDestination(insideSubConns, feature);
					}

and

							if (((toImpl instanceof ProcessorImplementation || toImpl instanceof DeviceImplementation
									|| toImpl instanceof MemoryImplementation)
									&& !(toEnd instanceof BusAccess
											&& ((BusAccess) toEnd).getKind() == AccessType.PROVIDES))
									|| ((toImpl instanceof DataImplementation || toImpl instanceof ThreadImplementation
											|| toImpl instanceof ThreadGroup || toImpl instanceof ProcessImplementation)
											&& !(toEnd instanceof DataAccess
													&& ((DataAccess) toEnd).getKind() == AccessType.PROVIDES))) {
								final ConnectionInfo clone = connInfo.cloneInfo();
								clone.complete = true;
								finalizeConnectionInstance(ci, clone, toFi);
							}

but this is not sufficient because the second block of code is guarded by an extremely complicated chain of conditions. Towards the start of this chain, there is test of the local variable finalComponent which gets its value from isConnectionEndingComponent(toCtx), which is true if the test component is a thread or device subcomponent. This has the effect of preventing the connection processing from going back "in" a thread component to find the nested data (or other subcomponent) being provided, event though this connection is subcomponent is connected and allowed to go "out" of the thread when the connection instance is generated in the other direction.

There is a test early on in this conditional chain

				if (toEnd instanceof Parameter
						|| finalComponent && !(toEnd instanceof FeatureGroup)) {

If I change it to

				if (toEnd instanceof Parameter
						|| finalComponent && !(toEnd instanceof FeatureGroup || toEnd instanceof Access)) {

Then data access connections work as required. As I have used the super type Access this test should be sufficient for subprogram and subprogram group accesses as well. However, I am concerned this change could cause unexpected problems in other cases because I don't at all understand everything happening in this method because it is so extremely complicated. I need to keep an eye on the unit tests.

[AaronGreenhouse]

The logic for checking the end of the connection instance in appendSegment got really tricky. I had to change it to

							boolean finalizeConnectionInstance = true;
							if (toEnd instanceof BusAccess && ((BusAccess) toEnd).getKind() == AccessType.PROVIDES) {
								if (toImpl instanceof ProcessorImplementation || toImpl instanceof MemoryImplementation
										|| toImpl instanceof BusImplementation || toImpl instanceof DeviceImplementation
										|| toImpl instanceof SystemImplementation) {
									finalizeConnectionInstance = false;
								}
							} else if (toEnd instanceof DataAccess
									&& ((DataAccess) toEnd).getKind() == AccessType.PROVIDES) {
								if (toImpl instanceof DataImplementation || toImpl instanceof ThreadImplementation
										|| toImpl instanceof ThreadGroupImplementation
										|| toImpl instanceof ProcessImplementation
										|| toImpl instanceof SystemImplementation) {
									finalizeConnectionInstance = false;
								}
							} else if (toEnd instanceof SubprogramAccess
									&& ((SubprogramAccess) toEnd).getKind() == AccessType.PROVIDES) {
								if (toImpl instanceof DataImplementation
										|| toImpl instanceof SubprogramGroupImplementation
										|| toImpl instanceof ThreadImplementation
										|| toImpl instanceof ThreadGroupImplementation
										|| toImpl instanceof ProcessImplementation
										|| toImpl instanceof ProcessorImplementation
										|| toImpl instanceof VirtualProcessorImplementation
										|| toImpl instanceof DeviceImplementation
										|| toImpl instanceof SystemImplementation) {
									finalizeConnectionInstance = false;
								}
							} else if (toEnd instanceof SubprogramGroupAccess
									&& ((SubprogramGroupAccess) toEnd).getKind() == AccessType.PROVIDES) {
								if (toImpl instanceof DataImplementation
										|| toImpl instanceof SubprogramGroupImplementation
										|| toImpl instanceof ThreadImplementation
										|| toImpl instanceof ThreadGroupImplementation
										|| toImpl instanceof ProcessImplementation
										|| toImpl instanceof ProcessorImplementation
										|| toImpl instanceof VirtualProcessorImplementation
										|| toImpl instanceof DeviceImplementation
										|| toImpl instanceof SystemImplementation) {
									finalizeConnectionInstance = false;
								}
							}
							if (finalizeConnectionInstance) {
								final ConnectionInfo clone = connInfo.cloneInfo();
								clone.complete = true;
								finalizeConnectionInstance(ci, clone, toFi);
							}

This can probably be simplified, but it works this way and the intent seems much clearer like this.

[AaronGreenhouse]

Added unit tests.

[AaronGreenhouse]

Ran all the unit tests. Things are not okay. First test that fails is for Issue 667. The logic for the changes to appendSegment is still incorrect.

[AaronGreenhouse]

I have to revisit the intention of the guard statements identified above in instantiateConnections and appendSegments. Looking at the code as it was before I attempted to fix it, it occurred to me there part of the test, (cat != THREAD && cat != PROCESSOR && cat != DEVICE && cat != VIRTUAL_PROCESSOR), seems to be an optimization to avoid going inside of components that aren't going to have useful internal connections. Whether this check was ever completely true, I'm not sure, but for the current AADL standard it makes no sense. Threads, processors, and devices can all contain subcomponents that have regular ports (never mind the provides access problem), and these ports can be connected to outgoing ports of the thread, processors, or device. The conditional clause above can disrupt this connection.

I can use a similar example as the ones above to demonstrate this. Here we have a thread with an abstract subcomponent, and the connection instances are messed up:

package JustAbstract
public
	abstract A
		features
			p: in out event port;
	end A;

	thread T1
		features
			f1: in out event port;
	end T1;

	thread implementation T1.impl
		subcomponents
			sub: abstract A;
		connections
			c1: port sub.p <-> f1;
	end T1.impl;

	thread T2
		features
			f2: in out event port;
	end T2;

	thread implementation T2.impl
	end T2.impl;

	process p1
	end p1;

	process implementation p1.impl
		subcomponents
			MyT1: thread T1.impl;
			MyT2: thread T2.impl;
		connections
			c2: port MyT1.f1 <-> MyT2.f2;
	end p1.impl;

	system Root
	end Root;

	system implementation Root.impl
		subcomponents
			MyP: process p1.impl;
	end Root.impl;
end JustAbstract;

The connection instances generated are

But, if I take the model above and change the threads and processes to system components,

package JustAbstract_System
public
	abstract A
		features
			p: in out event port;
	end A;

	system T1
		features
			f1: in out event port;
	end T1;

	system implementation T1.impl
		subcomponents
			sub: abstract A;
		connections
			c1: port sub.p <-> f1;
	end T1.impl;

	system T2
		features
			f2: in out event port;
	end T2;

	system implementation T2.impl
	end T2.impl;

	system p1
	end p1;

	system implementation p1.impl
		subcomponents
			MyT1: system T1.impl;
			MyT2: system T2.impl;
		connections
			c2: port MyT1.f1 <-> MyT2.f2;
	end p1.impl;

	system Root
	end Root;

	system implementation Root.impl
		subcomponents
			MyP: system p1.impl;
	end Root.impl;
end JustAbstract_System;

the generated instance model is correct, which two connection instances corresponding to the bidirectionally of the original connections.

[AaronGreenhouse]

I discussed this with Lutz. The guard against threads, processors, and devices is meant to prevent connections ending at subprogram parameters. This is because of timing considerations. Lutz needs to get more clarification from Peter about this.

For now

• Do not go into threads, processors, or devices if the internal connection is to a subprogram parameter, unless that parameter is a requires subprogram or subprogram group access. I think the test here is
• Go into everything else all the time.

Do not go into threads, processors, virtual processors or devices if the internal connection is a parameter connection.

Lots more things to create tests for:

• Obviously test parameter connections inside of threads, processors, virtual processors, and devices.
• Test access connections inside of the above, and inside other items
• test subprograms inside of subprograms, with parameter connections
• Who can have provided subprogram subcomponents: data, subprogram group, thread, thread group, process, processor (but it cannot have subprogram subcomponents??), virtual processor (but it cannot have subprogram subcomponents??), device (but it cannot have subprogram subcomponents??). Check that parameter connections are not allowed in these cases.

[AaronGreenhouse]

Not sure what I need to do on the other end of the connection yet in appendSegment. It's going to have to be a variation of the same test I put in instantiateConnections, because it's just the same issue on the other end of the connection path.

[AaronGreenhouse]

May have gotten carried away in yesterday's discussion. The only things that can have subprogram calls and parameter connections are threads and other subprograms. So I don't see why we need to worry about memory, device, processor, or virtual processor.

Parser/verifier doesn't even allow parameter connections outside of thread or subprogram implementation classifiers.

Also, remember: parameter connections aren't dealt with at all in the instance model.

[AaronGreenhouse]

I tried attacking this problem in a more principle fashion as described above. The test we really care about is the connections inside the component are ParameterConnections.

Again, I have changed things in 3 main locations, plus fiddled with some of the helper methods this time.

1. In instantiateConnections() I replaced

                    if (hasOutgoingFeatureSubcomponents
                            && ((cat != THREAD && cat != PROCESSOR && cat != DEVICE && cat != VIRTUAL_PROCESSOR)
                                    // in case of a provides bus access we want to
                                    // start from the bus.
                                    || ((cat == PROCESSOR || cat == DEVICE || cat == MEMORY)
                                            && feature instanceof BusAccess
                                            && ((BusAccess) feature).getKind() == AccessType.PROVIDES)) {
                        connectedInside = isConnectionEnd(insideSubConns, feature);
                        destinationFromInside = isDestination(insideSubConns, feature);
                    }

with just

					if (hasOutgoingFeatureSubcomponents
					) {
						connectedInside = isConnectionEnd(insideSubConns, feature);
						destinationFromInside = isDestination(insideSubConns, feature);
					}

but, I also changed isConnectionEnd and isDestination to ignore parameter connections:

	public boolean isConnectionEnd(List<Connection> conns, Feature feature) {
		List<Feature> features = feature.getAllFeatureRefinements();

		for (Connection conn : conns) {
			// ignore parameter connections
			if (!(conn instanceof ParameterConnection)) {
				if (features.contains(conn.getAllDestination()) || features.contains(conn.getAllSource())) {
					return true;
				}
				if ((features.contains(conn.getAllDestinationContext())
						|| features.contains(conn.getAllSourceContext()))) {
					return true;
				}
			}
		}
		return false;
	}

	public boolean isDestination(List<Connection> conns, Feature feature) {
		List<Feature> features = feature.getAllFeatureRefinements();

		for (Connection conn : conns) {
			// ignore parameter connections
			if (!(conn instanceof ParameterConnection)) {
				if (features.contains(conn.getAllDestination())
						|| conn.isAllBidirectional() && features.contains(conn.getAllSource())) {
					return true;
				}
				if ((features.contains(conn.getAllDestinationContext())
						|| conn.isAllBidirectional() && features.contains(conn.getAllSourceContext()))) {
					return true;
				}
			}
		}
		return false;
	}

In appendSegment() I changed

                if (toEnd instanceof Parameter
                        || finalComponent && !(toEnd instanceof FeatureGroup)) {

to just

                if (toEnd instanceof Parameter) {

2. I wanted to get rid of the finalComponent flag altogether, but it is used in the following else if having to do with feature group features:

				} else if (finalComponent && toEnd instanceof FeatureGroup) { 

I'm not sure what to do with this yet. I have a feeling this is also wrong and that it will prevent access features inside of feature groups from working correctly. I need a test case for this.

3. I replaced

                            if (((toImpl instanceof ProcessorImplementation || toImpl instanceof DeviceImplementation
                                    || toImpl instanceof MemoryImplementation)
                                    && !(toEnd instanceof BusAccess
                                            && ((BusAccess) toEnd).getKind() == AccessType.PROVIDES))) {
                                final ConnectionInfo clone = connInfo.cloneInfo();
                                clone.complete = true;
                                finalizeConnectionInstance(ci, clone, toFi);
                            }

with

							/*
							 * Issue 2032: If we get here then destination component has internal connections,
							 * not all of which are parameter connections. If there is at least one
							 * parameter connection, as indicated by the hasParameterConnection flag,
							 * we finalize the current connection, but also keep going with processing
							 * the internal connections along the current path.
							 */
							if (hasParameterConnection.get()) {
								final ConnectionInfo clone = connInfo.cloneInfo();
								clone.complete = true;
								finalizeConnectionInstance(ci, clone, toFi);
							}

where, earlier in the method I changed how the list of ingoing connections is computed. I replaced

						List<Connection> conns = AadlUtil.getIngoingConnections(toImpl, toFeature);

with code that gets the connections, but filters out any parameter connections, but also makes note if there were any parameter connections.

						final AtomicBoolean hasParameterConnection = new AtomicBoolean(false);
						List<Connection> conns = AadlUtil.getIngoingConnections(toImpl, toFeature,
								c -> {
									if (c instanceof ParameterConnection) {
										hasParameterConnection.set(true);
										return false;
									} else {
										return true;
									}
								});

where I also changed AadlUtil.getIngoingConnections() to take a predicate, (but also left a compatible original version in place):

	public static EList<Connection> getIngoingConnections(ComponentImplementation cimpl, Feature feature) {
		return getIngoingConnections(cimpl, feature, c -> true);
	}

	/**
	 * Get ingoing connections to subcomponents from a specified feature of the
	 * component impl
	 *
	 * @param feature component impl feature that is the source of a connection
	 * @param context the outer feature (feature group) or null
	 * @param useConnection Predicate that indicates if the connection should be added to the result.  Each ingoing
	 * connection is tested before being added to the result.  This predicate should return <code>true</code> if the
	 * connection should be added to the result.
	 * @return EList connections with feature as source
	 */
	public static EList<Connection> getIngoingConnections(ComponentImplementation cimpl, Feature feature,
			Predicate<Connection> useConnection) {
		EList<Connection> result = new BasicEList<Connection>();
		// The local feature could be a refinement of the feature through which the connection enters the component
		Feature local = (Feature) cimpl.findNamedElement(feature.getName());
		List<Feature> features = local.getAllFeatureRefinements();
		EList<Connection> cimplconns = cimpl.getAllConnections();
		for (Connection conn : cimplconns) {
			if (features.contains(conn.getAllSource()) && !(conn.getAllSourceContext() instanceof Subcomponent)
					|| (conn.isAllBidirectional() && features.contains(conn.getAllDestination())
							&& !(conn.getAllDestinationContext() instanceof Subcomponent))) {
				if (useConnection.test(conn)) {
					result.add(conn);
				}
			}
			if ((features.contains(conn.getAllSourceContext())
					|| (conn.isAllBidirectional() && features.contains(conn.getAllDestinationContext())))) {
				if (useConnection.test(conn)) {
					result.add(conn);
				}
			}
		}
		return result;
	}

My earlier unit tests for this issue pass with these changes. I need to run all the tests now and see what I may have broken. I also want to add additional test of my own:

• Obviously test parameter connections inside of threads, processors, virtual processors, and devices.
• Test access connections inside of the above, and inside other items
• test subprograms inside of subprograms, with parameter connections
• Test access features inside of feature groups (see above)
• test nested abstract components (see the problem above)
• test case where feature has a parameter connection and a connection to an abstract component. ("stop and go" case)

[AaronGreenhouse]

Good news, all the existing unit tests pass under this scenario!

I'm going to add some more tests specific to this issue (see above comment), and hopefully all will be fine.

Need to remember to get rid of any commented out code.

[AaronGreenhouse]

Added tests for

• Virtual bus access connections in system
• Bus access connections in device (instead of system)
• Bus access connections in processor (instead of system)
• Virtual bus access connections in processor (instead of system)
• Virtual bus access connections in virtual processor (instead of system)

The following don't work right now:

• Virtual bus access connections in system
• Virtual bus access connections in processor (instead of system)
• Virtual bus access connections in virtual processor (instead of system)

They all fail the same way, with three connection instances:

[AaronGreenhouse]

Fixed the above problem. Had to update AadlUtil.hasOutgoingFeatureSubcomponents() to consider virtual bus access.

	public static boolean hasOutgoingFeatureSubcomponents(EList<? extends ComponentInstance> subcompinstances) {
		for (ComponentInstance o : subcompinstances) {
			EList<FeatureInstance> filist = o.getFeatureInstances();
			for (FeatureInstance fi : filist) {
				Feature f = fi.getFeature();
				if (isOutgoingFeature(f)) {
					return true;
				}
			}
			// subcomponent can be access source
			ComponentCategory cat = o.getCategory();
			if (cat == DATA || cat == BUS || cat == VIRTUAL_BUS || cat == SUBPROGRAM || cat == SUBPROGRAM_GROUP) {
				return true;
			}
		}
		return false;
	}

[AaronGreenhouse]

Added a ton of more tests, trying to capture all the cases where an access connection can be applied. Also added tests where we use access connections inside of feature groups. As predicted, not all of the feature group cases work. I believe this to be due to the use of the finalComponent flag in appendSegment as identified above.

[AaronGreenhouse]

Fixed feature group problems by removing the special section in appendSegment that handles feature groups on final components:

				} else if (finalComponent && toEnd instanceof FeatureGroup) {
					// connection ends at a feature that is contained in a feature
					// group
					// of a thread, device, or (virtual) processor
					FeatureInstance dstFi = toCi.findFeatureInstance(toFeature);
					if (dstFi == null) {
						error(toCi,
								"Destination feature " + toFeature.getName() + " not found. No connection created.");
					} else {
						connInfo.complete = true;
						finalizeConnectionInstance(ci, connInfo, dstFi);
					}

Was able to remove the whole finalComonent variable

All OSATE Unit tests pass with this change.

[AaronGreenhouse]

Added test case for the abstract component issue that was identified above. Previous corrections to the code have fixed this issue.

[AaronGreenhouse]

Created test for the case where a thread has port feature that is internal connected via both parameter connection and a port connection. This is admittedly a convoluted case because it requires the use of an abstract subcomponent in a way that it is probably not intended. In any case, we want to see two connection instances in this case:

• one that stops at the edge of the thread (due to the parameter connection)
• one that continues into the thread and connects to the abstract subcomponent.

This works as expected.

package StopAndGo
public
	data D
	end D;
	
	subprogram Sub
		features
			p: in parameter D;
	end sub;
	
	abstract A
		features
			input: in data port D;
	end A;
	
	thread T1
		features
			output: out data port D;
	end T1;
	
	thread implementation T1.i
	end T1.i;
	
	thread T2
		features
			input: in data port D;
	end T2;
	
	thread implementation T2.i
		subcomponents
			s: subprogram Sub;
			abs: abstract A;
		calls
			normal: {
				call1: subprogram s;
			};
		connections
			aa: port input -> abs.input;
			bb: parameter input -> call1.p;
	end T2.i;
	
	process p
	end p;
	
	process implementation p.i
		subcomponents
			t1: thread t1.i;
			t2: thread t2.i;
		connections
			cc: port t1.output -> t2.input;
	end p.i;
	
	system Root
	end Root;
	
	system implementation Root.impl
		subcomponents
			p: process p.i;
	end Root.impl;
end StopAndGo;

[AaronGreenhouse]

Lutz pointed out that the above "stop and go" behavior is not desirable. I tested this on the master branch, and there only a connection instance corresponding to cc is created. This is the desired behavior. I have to go back to the working branch and disable whatever I did that allows this to happen with the current changes.

[AaronGreenhouse]

So the whole "stop and go" thing is enabled by getting rid of the finalComponent flag in appendSegment. But making it go away is not as simple as undoing that change because then the access connections in feature groups won't work. We need to add something somewhere that checks what is going on in the feature group?

I made a new version of the test that includes a subprogram access connection:

package StopAndGo
public
    data D
    end D;
        
    subprogram ShareMe
    end SHareMe;
    
    subprogram Sub
        features
            p: in parameter D;
    end sub;
    
    abstract A
        features
            input: in data port D;
    end A;
    
    thread T1
        features
            output: out data port D;
    		shared_sub: provides subprogram access ShareMe;
    end T1;
    
    thread implementation T1.i
    	subcomponents
    		s: subprogram ShareMe;
    	connections
    		xxx: subprogram access s <-> shared_sub;
    end T1.i;
    
    thread T2
        features
            input: in data port D;
    		provided_sub: requires subprogram access ShareMe;
    end T2;
    
    thread implementation T2.i
        subcomponents
            s: subprogram Sub;
            abs: abstract A;
        calls
            normal: {
                call1: subprogram s;
            };
        connections
            aa: port input -> abs.input;
            bb: parameter input -> call1.p;
    end T2.i;
    
    process p
    end p;
    
    process implementation p.i
        subcomponents
            t1: thread t1.i;
            t2: thread t2.i;
        connections
            cc: port t1.output -> t2.input;
            dd: subprogram access t1.shared_sub <-> t2.provided_sub;
    end p.i;
    
    system Root
    end Root;
    
    system implementation Root.impl
        subcomponents
            p: process p.i;
    end Root.impl;
end StopAndGo;

Currently this has 4 connection instances:

• 2 for the shared subprogram one in each direction, that does into the thread that provides the subprogram
• 1 for the parameter connection that stops at the edgesof the threads
• 1 for the port connection to the abstract subcomponent that does into the thread to reach the abstract subcomponent. [this is the connection that we do not want to have]

Now to make sure that we don't break feature groups I made a version that uses a feature group with two features instead of the two features we have above:

package StopAndGo_fg
public
    data D
    end D;
    
    subprogram ShareMe
    end SHareMe;
    
    subprogram Sub
        features
            p: in parameter D;
    end sub;
    
    abstract A
        features
            input: in data port D;
    end A;
    
    feature group FG1
    	features
    		output: out data port D;
    		shared_sub: provides subprogram access ShareMe;
    end FG1;
    
    feature group FG2
    	features
    		input: in data port D;
    		provided_sub: requires subprogram access ShareMe;
    	inverse of FG1
    end FG2;
    
    thread T1
    	features
    		fg1: feature group fg1;
    end T1;
    
    thread implementation T1.i
    	subcomponents
    		s: subprogram ShareMe;
    	connections
    		xxx: subprogram access s <-> fg1.shared_sub;
    end T1.i;
    
    thread T2
        features
            fg2: feature group fg2;
    end T2;
    
    thread implementation T2.i
        subcomponents
            s: subprogram Sub;
            abs: abstract A;
        calls
            normal: {
                call1: subprogram s;
            };
        connections
            aa: port fg2.input -> abs.input;
            bb: parameter fg2.input -> call1.p;
    end T2.i;
    
    process p
    end p;
    
    process implementation p.i
        subcomponents
            t1: thread t1.i;
            t2: thread t2.i;
        connections
            cc: feature group t1.fg1 <-> t2.fg2;
    end p.i;
    
    system Root
    end Root;
    
    system implementation Root.impl
        subcomponents
            p: process p.i;
    end Root.impl;
end StopAndGo;

I expected this to also have 4 connection instances. It does not. It has only 1, the subprogram access connection from t1 to t2.

• If I remove the connection aa to the abstract subcomponent, then I get back the subprogram access connection in the other direction.
• If I remove the connection xxx to the shared subcomponent in t1.i then I have 3 connection instances: one to connection each feature of the feature group across t1 and t2, and then one to go into t2 to connect to the abstract subcomponents (the one we don't want).

I think there is something wrong in instantiateConnection. I don't want to fix the finalComponent issue in appendSegment until I figure out what is going on with this situation.

[AaronGreenhouse]

There are several things that cause the above behavior with StopAndGo_fg.

• The fact that I removed checking for thread/device/processor in the inner loop of instantiateConnections() causes problems because it won't start an opposite connection from a destination feature on a thread if that thread has internal connections to abstract items (the only kinds of non-access, non-parameter connections that would be allowed inside the thread in this case). This is why removing the aa connection brings back the inverse access connection above.
• Feature groups require special handling (that I'm not sure was ever present?): Even if the component has a subcomponent with outgoing feature (access) connections so that we need to start a connection from the subcomponent, we may still need to start a connection from the feature group itself if the feature group has other outgoing features.

Follow up: Actually both these problems come from fact that I got rid of the "final component" checking in instantiateConnections(). Urgh.

[AaronGreenhouse]

Okay, gotta rollback some of my earlier changes.

Changed the check in instantiateConnections() to

					if (hasOutgoingFeatureSubcomponents && (!isConnectionEndingCategory(cat)
							|| (feature instanceof Access && ((Access) feature).getKind() == AccessType.PROVIDES))) {
						connectedInside = isConnectionEnd(insideSubConns, feature);
						destinationFromInside = isDestination(insideSubConns, feature);
					}

where isConnectionEndingCategory() is

	private boolean isConnectionEndingCategory(final ComponentCategory cat) {
		return cat == THREAD || cat == DEVICE || cat == PROCESSOR || cat == VIRTUAL_PROCESSOR;
	}

This fixes some things. But now I have a connection instance between the two subprogram access features in the feature group: t1.fg1.shared_sub -> t2.fg2.provided_sub. Not entirely sure what is causing this, but we don't want it. Should note though that this behavior now matches the behavior along the master branch.

[AaronGreenhouse]

Can fix some more problems by putting back the final component checks in appendSegment() (~line 509):

				if (toEnd instanceof Parameter || finalComponent && !(toEnd instanceof Access)) {
					// connection ends at a parameter or at a simple feature of a
					// thread, device, or (virtual) processor
					FeatureInstance dstFi = toCi.findFeatureInstance(toFeature);
					if (dstFi == null) {
						error(toCi,
								"Destination feature " + toFeature.getName() + " not found. No connection created.");
					} else {
						connInfo.complete = true;
						finalizeConnectionInstance(ci, connInfo, dstFi);
					}
				} else if (dstEmpty) {

(Don't need a separate check for feature groups like there used to be be because the executed code was identical.)

However, we do need to add some specialness for feature groups because we need to treat feature groups that have access features the same as we treat access features.

[AaronGreenhouse]

Further tweaked appendSegment(). Changed the part around line 509 to

				Feature toFeature = (Feature) toEnd;
				final boolean endHasAccessFeatures = endHasAccessFeatures(toCi, toEnd);

				if (toEnd instanceof Parameter || finalComponent && !endHasAccessFeatures) {
					// connection ends at a parameter or at a simple feature of a
					// thread, device, or (virtual) processor
					FeatureInstance dstFi = toCi.findFeatureInstance(toFeature);
					if (dstFi == null) {
						error(toCi,
								"Destination feature " + toFeature.getName() + " not found. No connection created.");
					} else {
						connInfo.complete = true;
						finalizeConnectionInstance(ci, connInfo, dstFi);
					}
				} else if (dstEmpty) {

where endHasAccessFeatures() is

	private boolean endHasAccessFeatures(final ComponentInstance ci, final ConnectionEnd end) {
		if (end instanceof Access) {
			return true;
		} else if (end instanceof FeatureGroup) {
			final FeatureGroup fg = (FeatureGroup) end;
			final FeatureInstance fgInstance = ci.findFeatureInstance(fg);
			for (final FeatureInstance fi : fgInstance.getFeatureInstances()) {
				if (fi.getFeature() instanceof Access) {
					return true;
				}
			}
			return false;
		} else {
			return false;
		}
	}

Then I changed the part that gets the internal "ingoing connections" to filter the connections differently. Namely, if the connection end is part of a final component and but has access features, we only want to return access connections. This is necessary to keep from connecting to the port features inside of feature groups on final components.

						/*
						 * Issue 2032: Get the connections internal to the destination component that connect
						 * to the feature. Two cases here. (1) If the component is final (thread/device/processor) but
						 * the end we are starting from has access features, then we are only interested in access connections
						 * internal to the component. (2) otherwise we want all the internal connections except for the parameter
						 * connections. Either way, we need to know if there are any parameter connections.
						 */
						final AtomicBoolean hasParameterConnection = new AtomicBoolean(false);
						List<Connection> conns = AadlUtil.getIngoingConnections(toImpl, toFeature,
								(finalComponent && endHasAccessFeatures) ? c -> {
									if (c instanceof ParameterConnection) {
										hasParameterConnection.set(true);
										return false;
									} else if (c instanceof AccessConnection) {
										return true;
									} else {
										return false;
									}
								} :
								c -> {
									if (c instanceof ParameterConnection) {
										hasParameterConnection.set(true);
										return false;
									} else {
										return true;
									}
								});

[AaronGreenhouse]

Now I still have one extra connection instance in the feature group case, the connection instance between the provides and requires access features. This shouldn't be there, and doesn't appear in the case where I don't use feature groups. I'm not sure where it is coming from. Going to set a breakpoint on finalizeConnectionInstance() to figure this one out.

[AaronGreenhouse]

Okay the unwanted connection between the access features of the feature groups is generated by finalizeConnectionInstance() as it matches the two feature groups together. This is not trivial to deal with. We want to have it not generate the connection for the features but we may not know yet that we want this. We only know the connection becomes redundant/undesirable after a connection instance is made all the two the shared subprogram.

This behavior was not introduced by trying to fix this bug. It exists on the master branch as well. Probably should introduce a separate issue for it.

[AaronGreenhouse]

Started going through my unit tests for this issue and the feature groups were being weird again. Problem is that I forgot I need to go into the feature group at the start in instantiateConnections just like to do at the end in appendSegment to check if the feature group has access connections.

Changed the block in instantiateConnections to

					if (hasOutgoingFeatureSubcomponents
							&& (!isConnectionEndingCategory(cat) || endHasAccessFeatures(ci, feature))) {
						connectedInside = isConnectionEnd(insideSubConns, feature);
						destinationFromInside = isDestination(insideSubConns, feature);
					}

and I also updated endHasAccessFeatures() to check for provides access only (as was in the original code on the master branch):

	private boolean endHasAccessFeatures(final ComponentInstance ci, final ConnectionEnd end) {
		if (end instanceof Access) {
			return ((Access) end).getKind() == AccessType.PROVIDES;
		} else if (end instanceof FeatureGroup) {
			final FeatureGroup fg = (FeatureGroup) end;
			final FeatureInstance fgInstance = ci.findFeatureInstance(fg);
			for (final FeatureInstance fi : fgInstance.getFeatureInstances()) {
				if (fi.getFeature() instanceof Access) {
					return ((Access) fi.getFeature()).getKind() == AccessType.PROVIDES;
				}
			}
			return false;
		} else {
			return false;
		}
	}

This fixed the feature group problems in my unit tests.

[AaronGreenhouse]

After the above changes the StopAndGo_fg example only has two connection instances, those for the shared subpgrogram. These seems wrong....

[AaronGreenhouse]

The new problems with StopAndGo_fg are caused by these lines in instantiateConnections:

					if (hasOutgoingFeatureSubcomponents
							&& (!isConnectionEndingCategory(cat) || endHasAccessFeatures(ci, feature))) {
						connectedInside = isConnectionEnd(insideSubConns, feature);
						destinationFromInside = isDestination(insideSubConns, feature);
					}

and the interaction with the conditional in the following for-loop:

						if (!(destinationFromInside || conn.isAllBidirectional() && connectedInside)) {

specifically, the test endHasAccessFeatures. This is fine for singular connections, but for feature group it is no good because a feature group can have additional ports that are not access connections, and thus we still want the above conditional to be true even though it would be false in the purely access feature case.

We need to separately keep track of what kind of features are present on the end point and explicitly test for the one that we care about at each point.

There is probably an analogous place in appendSegment() that checks the feature at the other end of the connection.

[AaronGreenhouse]

According to Lutz, we should ignore port connections inside of thread:

So I talked to Peter and he agrees that it is acceptable to ignore connections to/from abstract components inside threads, etc. In the future we may add validations that make sure that such an abstract component can have only features that are valid for concrete components in threads, so, for example, ports on abstract components inside threads would no longer be allowed.

Since we are considering thread, device, processor and virtual processor as connection ending components, I think we should treat them the same here and only go inside them for access connections.

[AaronGreenhouse]

Things get uglier and uglier. To prevent the creation of "upward" connections from the output ports of abstract subcomponents to the ports of their containing "connection ending component", we need to add checks to addSegment(). This cannot be handled in createConnectionIntances(). The analogous "downward" check will be later on inside of addSegment().

[AaronGreenhouse]

Added

		} else {
			toFi = ci.findSubcomponentInstance((Subcomponent) toEnd);
		}

		/*
		 * Issue 2032: We do not want connections that go from abstract subcomponent to the ports of
		 * their containing components if the containing component is final. We specifically are
		 * checking that the connection starts at a port feature and ends at a feature that is a feature
		 * of the containing component and the containing component is a connection ending component. We don't
		 * have to check that the end feature is a port because AADL semantics guarantee that it will be.
		 */
		if (fromFi instanceof FeatureInstance && ((FeatureInstance) fromFi).getFeature() instanceof Port
				&& toFi.eContainer().equals(ci) && isConnectionEndingCategory(ci.getCategory())) {
			return;
		}

		try {
			boolean[] keep = { false };
			boolean valid = connInfo.addSegment(newSegment, fromFi, toFi, ci, goOpposite, keep);
			if (!keep[0]) {
				return;
			}

into appendSegment() to drop upping abstract component connections. Seems to work.

Can now focus on making changes to the end of appendSegment to mirror the recent changes to instantiateConnections() and the above change to appendSegment.

[AaronGreenhouse]

fixed appendSegment() as above. Unit tests for this issue pass. I updated the stop and go tests to deal with the abstract connections:

• There should be connection instances between the two abstract subcomponents.
• There should not be connection instances up from or down an abstract subcomponent.

[AaronGreenhouse]

Unit tests (all) pass. Going to create pull request.