Repository: workcraft/workcraft
Issue: Connection from output port to input pin

To reproduce
1. Create a digital circuit with a output port and an inverter.
2. Connect output port to the inverter input.
3. Generate the circuit STG.

Observed:
An isolated elementary cycle is crested for inverter input pin.

Expected behaviour:
The STG for inverter input is redundant.

Possible solutions:
Option A: Forbid connections from output ports.
Option B: Improve the STG conversion code. 

Changed files:
CircuitPlugin/src/org/workcraft/plugins/circuit/CircuitUtils.java, StgPlugin/src/org/workcraft/plugins/stg/StgSettings.java, CircuitPlugin/src/org/workcraft/plugins/circuit/stg/CircuitToStgConverter.java, StgPlugin/src/org/workcraft/plugins/stg/generator/StgGenerator.java

diff --git a/CircuitPlugin/src/org/workcraft/plugins/circuit/CircuitUtils.java b/CircuitPlugin/src/org/workcraft/plugins/circuit/CircuitUtils.java
index dad9b787d..29d4d8b30 100644
--- a/CircuitPlugin/src/org/workcraft/plugins/circuit/CircuitUtils.java
+++ b/CircuitPlugin/src/org/workcraft/plugins/circuit/CircuitUtils.java
@@ -78,7 +78,8 @@ public class CircuitUtils {
                     queue.addAll(circuit.getPreset(zeroDelayInput));
                 } else if (contact.isDriver()) {
                     result = contact;
-                } else if (node == curNode) {
+                } else {
+                    // Is it necessary to check that (node == curNode) before adding preset to queue?
                     queue.addAll(circuit.getPreset(contact));
                 }
             } else {
@@ -121,25 +122,27 @@ public class CircuitUtils {
         }
         while (!queue.isEmpty()) {
             Node node = queue.remove();
-            if (!visited.contains(node)) {
-                visited.add(node);
-                if (node instanceof Joint) {
-                    queue.addAll(circuit.getPostset(node));
-                } else if (node instanceof Contact) {
-                    Contact contact = (Contact) node;
-                    // Support for zero-delay buffers and inverters.
-                    Contact zeroDelayOutput = transparentZeroDelayComponents ? findZeroDelayOutput(contact) : null;
-                    if (zeroDelayOutput != null) {
-                        queue.addAll(circuit.getPostset(zeroDelayOutput));
-                    } else if (contact.isDriven()) {
-                        result.add(contact);
-                    } else if (node == curNode) {
-                        queue.addAll(circuit.getPostset(contact));
-                    }
+            if (visited.contains(node)) {
+                continue;
+            }
+            visited.add(node);
+            if (node instanceof Joint) {
+                queue.addAll(circuit.getPostset(node));
+            } else if (node instanceof Contact) {
+                Contact contact = (Contact) node;
+                // Support for zero-delay buffers and inverters.
+                Contact zeroDelayOutput = transparentZeroDelayComponents ? findZeroDelayOutput(contact) : null;
+                if (zeroDelayOutput != null) {
+                    queue.addAll(circuit.getPostset(zeroDelayOutput));
+                } else if (contact.isDriven()) {
+                    result.add(contact);
                 } else {
-                    throw new RuntimeException("Unexpected node '" + circuit.getNodeReference(node)
-                            + "' in the driven trace for node '" + circuit.getNodeReference(curNode) + "'!");
+                    // Is it necessary to check that (node == curNode) before adding postset to queue?
+                    queue.addAll(circuit.getPostset(contact));
                 }
+            } else {
+                throw new RuntimeException("Unexpected node '" + circuit.getNodeReference(node)
+                + "' in the driven trace for node '" + circuit.getNodeReference(curNode) + "'!");
             }
         }
         return result;
diff --git a/CircuitPlugin/src/org/workcraft/plugins/circuit/stg/CircuitToStgConverter.java b/CircuitPlugin/src/org/workcraft/plugins/circuit/stg/CircuitToStgConverter.java
index d96b1a9ce..5bf14a320 100644
--- a/CircuitPlugin/src/org/workcraft/plugins/circuit/stg/CircuitToStgConverter.java
+++ b/CircuitPlugin/src/org/workcraft/plugins/circuit/stg/CircuitToStgConverter.java
@@ -41,12 +41,12 @@ import org.workcraft.plugins.petri.VisualPlace;
 import org.workcraft.plugins.stg.SignalTransition;
 import org.workcraft.plugins.stg.SignalTransition.Direction;
 import org.workcraft.plugins.stg.Stg;
+import org.workcraft.plugins.stg.StgSettings;
 import org.workcraft.plugins.stg.VisualImplicitPlaceArc;
 import org.workcraft.plugins.stg.VisualSignalTransition;
 import org.workcraft.plugins.stg.VisualStg;
 import org.workcraft.plugins.stg.generator.SignalStg;
 import org.workcraft.util.Geometry;
-import org.workcraft.util.Hierarchy;
 import org.workcraft.util.Pair;
 import org.workcraft.util.TwoWayMap;
 
@@ -79,10 +79,11 @@ public class CircuitToStgConverter {
         this.driverToStgMap = convertDriversToStgs(drivers);
         connectDriverStgs(drivers);
         if (CircuitSettings.getSimplifyStg()) {
-            simplifyDriverStgs(drivers); // remove dead transitions
+            // Remove dead transitions
+            simplifyDriverStgs(drivers);
         }
         positionDriverStgs(drivers);
-        //groupDriverStgs(drivers);
+        groupDriverStgs(drivers);
     }
 
     public CircuitToStgConverter(VisualCircuit circuit, VisualStg stg) {
@@ -91,12 +92,14 @@ public class CircuitToStgConverter {
         this.refToPageMap = convertPages();
         HashSet<VisualContact> drivers = identifyDrivers();
         this.nodeToDriverMap = associateNodesToDrivers(drivers);
-        this.driverToStgMap = associateDriversToStgs(drivers);  // STGs already exist, just associate them with the drivers
+        // STGs already exist, just associate them with the drivers
+        this.driverToStgMap = associateDriversToStgs(drivers);
         if (CircuitSettings.getSimplifyStg()) {
-            simplifyDriverStgs(drivers); // remove dead transitions
+            // Remove dead transitions
+            simplifyDriverStgs(drivers);
         }
         positionDriverStgs(drivers);
-        //groupDriverStgs(drivers);
+        groupDriverStgs(drivers);
     }
 
     public VisualStg getStg() {
@@ -152,7 +155,7 @@ public class CircuitToStgConverter {
 
     private HashSet<VisualContact> identifyDrivers() {
         HashSet<VisualContact> result = new HashSet<>();
-        for (VisualContact contact : Hierarchy.getDescendantsOfType(circuit.getRoot(), VisualContact.class)) {
+        for (VisualContact contact : circuit.getVisualFunctionContacts()) {
             VisualContact driver = CircuitUtils.findDriver(circuit, contact);
             if (driver == null) {
                 driver = contact;
@@ -507,27 +510,32 @@ public class CircuitToStgConverter {
     }
 
     private void groupDriverStgs(HashSet<VisualContact> drivers) {
-        for (VisualContact driver: drivers) {
-            SignalStg signalStg = driverToStgMap.getValue(driver);
-            if (signalStg != null) {
-                Collection<Node> nodesToGroup = new LinkedList<>();
-                nodesToGroup.addAll(signalStg.getAllNodes());
-
-                Container currentLevel = null;
-                Container oldLevel = stg.getCurrentLevel();
-                for (Node node: nodesToGroup) {
-                    if (currentLevel == null) {
-                        currentLevel = (Container) node.getParent();
-                    }
-                    if (currentLevel != node.getParent()) {
-                        throw new RuntimeException("Current level is not the same among the processed nodes");
-                    }
+        if (StgSettings.getGroupSignalConversion()) {
+            for (VisualContact driver: drivers) {
+                SignalStg signalStg = driverToStgMap.getValue(driver);
+                groupSignalStg(signalStg);
+            }
+        }
+    }
+
+    private void groupSignalStg(SignalStg signalStg) {
+        if ((signalStg != null) && StgSettings.getGroupSignalConversion()) {
+            Collection<Node> nodesToGroup = new LinkedList<>();
+            nodesToGroup.addAll(signalStg.getAllNodes());
+            Container currentLevel = null;
+            Container oldLevel = stg.getCurrentLevel();
+            for (Node node: nodesToGroup) {
+                if (currentLevel == null) {
+                    currentLevel = (Container) node.getParent();
+                }
+                if (currentLevel != node.getParent()) {
+                    throw new RuntimeException("Current level is not the same among the processed nodes");
                 }
-                stg.setCurrentLevel(currentLevel);
-                stg.select(nodesToGroup);
-                stg.groupSelection();
-                stg.setCurrentLevel(oldLevel);
             }
+            stg.setCurrentLevel(currentLevel);
+            stg.select(nodesToGroup);
+            stg.groupSelection();
+            stg.setCurrentLevel(oldLevel);
         }
     }
 
diff --git a/StgPlugin/src/org/workcraft/plugins/stg/StgSettings.java b/StgPlugin/src/org/workcraft/plugins/stg/StgSettings.java
index 2b4480ce8..3738aede4 100644
--- a/StgPlugin/src/org/workcraft/plugins/stg/StgSettings.java
+++ b/StgPlugin/src/org/workcraft/plugins/stg/StgSettings.java
@@ -39,16 +39,19 @@ public class StgSettings implements Settings {
     private static final String keyDensityMapLevelLimit = prefix + ".densityMapLevelLimit";
     private static final String keyLowLevelSuffix = prefix + ".lowLevelSuffix";
     private static final String keyHighLevelSuffix = prefix + ".highLevelSuffix";
+    private static final String keyGroupSignalConversion = prefix + ".groupSignalConversion";
     private static final String keyConceptsFolderLocation = prefix + ".conceptsFolderLocation";
 
     private static final Integer defaultDensityMapLevelLimit = 5;
     private static final String defaultLowLevelSuffix = "_LOW";
     private static final String defaultHighLevelSuffix = "_HIGH";
+    private static final Boolean defaultGroupSignalConversion = false;
     private static final String defaultConceptsFolderLocation = DesktopApi.getOs().isWindows() ? "tools\\\\concepts\\\\" : "tools/concepts/";
 
     private static Integer densityMapLevelLimit = defaultDensityMapLevelLimit;
     private static String lowLevelSuffix = defaultLowLevelSuffix;
     private static String highLevelSuffix = defaultHighLevelSuffix;
+    private static Boolean groupSignalConversion = defaultGroupSignalConversion;
     private static String conceptsFolderLocation = defaultConceptsFolderLocation;
 
     public StgSettings() {
@@ -96,6 +99,16 @@ public class StgSettings implements Settings {
             }
         });
 
+        properties.add(new PropertyDeclaration<StgSettings, Boolean>(
+                this, "Group signals on conversion", Boolean.class, true, false, false) {
+            protected void setter(StgSettings object, Boolean value) {
+                setGroupSignalConversion(value);
+            }
+            protected Boolean getter(StgSettings object) {
+                return getGroupSignalConversion();
+            }
+        });
+
         properties.add(new PropertyDeclaration<StgSettings, String>(
                 this, "Concepts folder location", String.class, true, false, false) {
             protected void setter(StgSettings object, String value) {
@@ -142,6 +155,7 @@ public class StgSettings implements Settings {
         setDensityMapLevelLimit(config.getInt(keyDensityMapLevelLimit, defaultDensityMapLevelLimit));
         setLowLevelSuffix(config.getString(keyLowLevelSuffix, defaultLowLevelSuffix));
         setHighLevelSuffix(config.getString(keyHighLevelSuffix, defaultHighLevelSuffix));
+        setGroupSignalConversion(config.getBoolean(keyGroupSignalConversion, defaultGroupSignalConversion));
         setConceptsFolderLocation(config.getString(keyConceptsFolderLocation, defaultConceptsFolderLocation));
     }
 
@@ -150,6 +164,7 @@ public class StgSettings implements Settings {
         config.setInt(keyDensityMapLevelLimit, getDensityMapLevelLimit());
         config.set(keyLowLevelSuffix, getLowLevelSuffix());
         config.set(keyHighLevelSuffix, getHighLevelSuffix());
+        config.setBoolean(keyGroupSignalConversion, getGroupSignalConversion());
         config.set(keyConceptsFolderLocation, getConceptsFolderLocation());
     }
 
@@ -191,6 +206,14 @@ public class StgSettings implements Settings {
         }
     }
 
+    public static Boolean getGroupSignalConversion() {
+        return groupSignalConversion;
+    }
+
+    public static void setGroupSignalConversion(Boolean value) {
+        groupSignalConversion = value;
+    }
+
     public static String getConceptsFolderLocation() {
         return conceptsFolderLocation;
     }
diff --git a/StgPlugin/src/org/workcraft/plugins/stg/generator/StgGenerator.java b/StgPlugin/src/org/workcraft/plugins/stg/generator/StgGenerator.java
index 3ba234751..50ccb44af 100644
--- a/StgPlugin/src/org/workcraft/plugins/stg/generator/StgGenerator.java
+++ b/StgPlugin/src/org/workcraft/plugins/stg/generator/StgGenerator.java
@@ -20,6 +20,7 @@ import org.workcraft.exceptions.InvalidConnectionException;
 import org.workcraft.plugins.petri.VisualPlace;
 import org.workcraft.plugins.petri.VisualReplicaPlace;
 import org.workcraft.plugins.stg.Stg;
+import org.workcraft.plugins.stg.StgSettings;
 import org.workcraft.plugins.stg.SignalTransition;
 import org.workcraft.plugins.stg.VisualStg;
 import org.workcraft.plugins.stg.VisualSignalTransition;
@@ -281,8 +282,10 @@ public abstract class StgGenerator {
     }
 
     public void groupComponentStg(NodeStg nodeStg) {
-        stg.select(nodeStg.getAllNodes());
-        stg.groupSelection();
+        if (StgSettings.getGroupSignalConversion()) {
+            stg.select(nodeStg.getAllNodes());
+            stg.groupSelection();
+        }
     }
 
 }