Fix --unroll CLI argument to properly unroll arrays instead of repeating them (#5)

* Initial plan

* Fix --unroll CLI argument to properly unroll arrays

Updated check_is_array() method and all CPUIF implementations to correctly detect and handle unrolled array elements. When unroll=True, each array element is now treated as an individual instance instead of being repeated as an array.

Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com>

* Add comprehensive tests for --unroll functionality

Created tests to verify that array elements are correctly unrolled into individual instances when --unroll flag is used. Also fixed conftest.py fixture to keep temp files for later reference.

Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com>

src/peakrdl_busdecoder/cpuif/apb3/apb3_cpuif.py

@@ -12,11 +12,16 @@ class APB3Cpuif(BaseCpuif):
 
     def _port_declaration(self, child: AddressableNode) -> str:
         base = f"apb3_intf.master m_apb_{child.inst_name}"
-        if child.array_dimensions is None:
-            return base
+        
+        # When unrolled, current_idx is set - append it to the name
         if child.current_idx is not None:
-            return f"{base}_{'_'.join(map(str, child.current_idx))} {''.join(f'[{dim}]' for dim in child.array_dimensions)}"
-        return f"{base} {''.join(f'[{dim}]' for dim in child.array_dimensions)}"
+            base = f"{base}_{'_'.join(map(str, child.current_idx))}"
+        
+        # Only add array dimensions if this should be treated as an array
+        if self.check_is_array(child):
+            return f"{base} {''.join(f'[{dim}]' for dim in child.array_dimensions)}"
+        
+        return base
 
     @property
     def port_declaration(self) -> str:

src/peakrdl_busdecoder/cpuif/apb3/apb3_cpuif_flat.py

@@ -54,11 +54,13 @@ class APB3CpuifFlat(BaseCpuif):
 
         # Master signal
         base = f"m_apb_{node.inst_name}"
-        if not node.is_array:
+        if not self.check_is_array(node):
+            # Not an array or an unrolled element
+            if node.current_idx is not None:
+                # This is a specific instance of an unrolled array
+                return f"{base}_{signal}_{'_'.join(map(str, node.current_idx))}"
             return f"{base}_{signal}"
-        if node.current_idx is not None:
-            # This is a specific instance of an array
-            return f"{base}_{signal}_{'_'.join(map(str, node.current_idx))}"
+        # Is an array
         if idx is not None:
             return f"{base}_{signal}[{idx}]"
         return f"{base}_{signal}[N_{node.inst_name.upper()}S]"

src/peakrdl_busdecoder/cpuif/apb4/apb4_cpuif.py

@@ -12,11 +12,16 @@ class APB4Cpuif(BaseCpuif):
 
     def _port_declaration(self, child: AddressableNode) -> str:
         base = f"apb4_intf.master m_apb_{child.inst_name}"
-        if child.array_dimensions is None:
-            return base
+        
+        # When unrolled, current_idx is set - append it to the name
         if child.current_idx is not None:
-            return f"{base}_{'_'.join(map(str, child.current_idx))} {''.join(f'[{dim}]' for dim in child.array_dimensions)}"
-        return f"{base} {''.join(f'[{dim}]' for dim in child.array_dimensions)}"
+            base = f"{base}_{'_'.join(map(str, child.current_idx))}"
+        
+        # Only add array dimensions if this should be treated as an array
+        if self.check_is_array(child):
+            return f"{base} {''.join(f'[{dim}]' for dim in child.array_dimensions)}"
+        
+        return base
 
     @property
     def port_declaration(self) -> str:

src/peakrdl_busdecoder/cpuif/apb4/apb4_cpuif_flat.py

@@ -58,11 +58,13 @@ class APB4CpuifFlat(BaseCpuif):
 
         # Master signal
         base = f"m_apb_{node.inst_name}"
-        if not node.is_array:
+        if not self.check_is_array(node):
+            # Not an array or an unrolled element
+            if node.current_idx is not None:
+                # This is a specific instance of an unrolled array
+                return f"{base}_{signal}_{'_'.join(map(str, node.current_idx))}"
             return f"{base}_{signal}"
-        if node.current_idx is not None:
-            # This is a specific instance of an array
-            return f"{base}_{signal}_{'_'.join(map(str, node.current_idx))}"
+        # Is an array
         if idx is not None:
             return f"{base}_{signal}[{idx}]"
         return f"{base}_{signal}[N_{node.inst_name.upper()}S]"

src/peakrdl_busdecoder/cpuif/axi4lite/axi4_lite_cpuif.py

@@ -12,11 +12,16 @@ class AXI4LiteCpuif(BaseCpuif):
 
     def _port_declaration(self, child: AddressableNode) -> str:
         base = f"axi4lite_intf.master m_axil_{child.inst_name}"
-        if child.array_dimensions is None:
-            return base
+        
+        # When unrolled, current_idx is set - append it to the name
         if child.current_idx is not None:
-            return f"{base}_{'_'.join(map(str, child.current_idx))} {''.join(f'[{dim}]' for dim in child.array_dimensions)}"
-        return f"{base} {''.join(f'[{dim}]' for dim in child.array_dimensions)}"
+            base = f"{base}_{'_'.join(map(str, child.current_idx))}"
+        
+        # Only add array dimensions if this should be treated as an array
+        if self.check_is_array(child):
+            return f"{base} {''.join(f'[{dim}]' for dim in child.array_dimensions)}"
+        
+        return base
 
     @property
     def port_declaration(self) -> str:

src/peakrdl_busdecoder/cpuif/base_cpuif.py

@@ -71,7 +71,11 @@ class BaseCpuif:
         raise RuntimeError
 
     def check_is_array(self, node: AddressableNode) -> bool:
-        return node.is_array and not self.unroll
+        # When unrolling is enabled, children(unroll=True) returns individual
+        # array elements with current_idx set. These should NOT be treated as arrays.
+        if self.unroll and hasattr(node, 'current_idx') and node.current_idx is not None:
+            return False
+        return node.is_array
 
     def get_implementation(self) -> str:
         class_dir = self._get_template_path_class_dir()

tests/unit/conftest.py

@@ -35,7 +35,8 @@ def compile_rdl(tmp_path: Path):
         for include_path in include_paths or ():
             compiler.add_include_path(str(include_path))
 
-        with NamedTemporaryFile("w", suffix=".rdl", dir=tmp_path) as tmp_file:
+        # Use delete=False to keep the file around after closing
+        with NamedTemporaryFile("w", suffix=".rdl", dir=tmp_path, delete=False) as tmp_file:
             tmp_file.write(source)
             tmp_file.flush()
 

tests/unit/test_unroll.py

@@ -0,0 +1,163 @@
+"""Test the --unroll CLI argument functionality."""
+
+from pathlib import Path
+from tempfile import TemporaryDirectory
+
+import pytest
+
+from peakrdl_busdecoder import BusDecoderExporter
+from peakrdl_busdecoder.cpuif.apb3 import APB3Cpuif
+from peakrdl_busdecoder.cpuif.apb4 import APB4Cpuif
+
+
+@pytest.fixture
+def sample_rdl(compile_rdl):
+    """Create a simple RDL design with an array."""
+    rdl_source = """
+    addrmap top {
+        reg my_reg {
+            field {
+                sw=rw;
+                hw=r;
+            } data[31:0];
+        };
+        
+        my_reg regs[4] @ 0x0 += 0x4;
+    };
+    """
+    return compile_rdl(rdl_source)
+
+
+def test_unroll_disabled_creates_array_interface(sample_rdl):
+    """Test that with unroll=False, array nodes are kept as arrays."""
+    with TemporaryDirectory() as tmpdir:
+        exporter = BusDecoderExporter()
+        exporter.export(
+            sample_rdl.top,
+            tmpdir,
+            cpuif_cls=APB4Cpuif,
+            cpuif_unroll=False,
+        )
+        
+        # Read the generated module
+        module_file = Path(tmpdir) / "top.sv"
+        content = module_file.read_text()
+        
+        # Should have a single array interface with [4] dimension
+        assert "m_apb_regs [4]" in content
+        
+        # Should have a parameter for array size
+        assert "N_REGSS = 4" in content
+        
+        # Should NOT have individual indexed interfaces
+        assert "m_apb_regs_0" not in content
+        assert "m_apb_regs_1" not in content
+        assert "m_apb_regs_2" not in content
+        assert "m_apb_regs_3" not in content
+
+
+def test_unroll_enabled_creates_individual_interfaces(sample_rdl):
+    """Test that with unroll=True, array elements are unrolled into separate instances."""
+    with TemporaryDirectory() as tmpdir:
+        exporter = BusDecoderExporter()
+        exporter.export(
+            sample_rdl.top,
+            tmpdir,
+            cpuif_cls=APB4Cpuif,
+            cpuif_unroll=True,
+        )
+        
+        # Read the generated module
+        module_file = Path(tmpdir) / "top.sv"
+        content = module_file.read_text()
+        
+        # Should have individual interfaces without array dimensions
+        assert "m_apb_regs_0," in content or "m_apb_regs_0\n" in content
+        assert "m_apb_regs_1," in content or "m_apb_regs_1\n" in content
+        assert "m_apb_regs_2," in content or "m_apb_regs_2\n" in content
+        assert "m_apb_regs_3" in content
+        
+        # Should NOT have array interface
+        assert "m_apb_regs [4]" not in content
+        
+        # Should NOT have individual interfaces with array dimensions (the bug we're fixing)
+        assert "m_apb_regs_0 [4]" not in content
+        assert "m_apb_regs_1 [4]" not in content
+        assert "m_apb_regs_2 [4]" not in content
+        assert "m_apb_regs_3 [4]" not in content
+        
+        # Should NOT have array size parameter when unrolled
+        assert "N_REGSS" not in content
+
+
+def test_unroll_with_apb3(sample_rdl):
+    """Test that unroll works correctly with APB3 interface."""
+    with TemporaryDirectory() as tmpdir:
+        exporter = BusDecoderExporter()
+        exporter.export(
+            sample_rdl.top,
+            tmpdir,
+            cpuif_cls=APB3Cpuif,
+            cpuif_unroll=True,
+        )
+        
+        # Read the generated module
+        module_file = Path(tmpdir) / "top.sv"
+        content = module_file.read_text()
+        
+        # Should have individual APB3 interfaces
+        assert "m_apb_regs_0," in content or "m_apb_regs_0\n" in content
+        assert "m_apb_regs_1," in content or "m_apb_regs_1\n" in content
+        assert "m_apb_regs_2," in content or "m_apb_regs_2\n" in content
+        assert "m_apb_regs_3" in content
+        
+        # Should NOT have array dimensions on unrolled interfaces
+        assert "m_apb_regs_0 [4]" not in content
+
+
+@pytest.fixture
+def multidim_array_rdl(compile_rdl):
+    """Create an RDL design with a multi-dimensional array."""
+    rdl_source = """
+    addrmap top {
+        reg my_reg {
+            field {
+                sw=rw;
+                hw=r;
+            } data[31:0];
+        };
+        
+        my_reg matrix[2][3] @ 0x0 += 0x4;
+    };
+    """
+    return compile_rdl(rdl_source)
+
+
+def test_unroll_multidimensional_array(multidim_array_rdl):
+    """Test that unroll works correctly with multi-dimensional arrays."""
+    with TemporaryDirectory() as tmpdir:
+        exporter = BusDecoderExporter()
+        exporter.export(
+            multidim_array_rdl.top,
+            tmpdir,
+            cpuif_cls=APB4Cpuif,
+            cpuif_unroll=True,
+        )
+        
+        # Read the generated module
+        module_file = Path(tmpdir) / "top.sv"
+        content = module_file.read_text()
+        
+        # Should have individual interfaces for each element in the 2x3 array
+        # Format should be m_apb_matrix_0_0, m_apb_matrix_0_1, ..., m_apb_matrix_1_2
+        assert "m_apb_matrix_0_0" in content
+        assert "m_apb_matrix_0_1" in content
+        assert "m_apb_matrix_0_2" in content
+        assert "m_apb_matrix_1_0" in content
+        assert "m_apb_matrix_1_1" in content
+        assert "m_apb_matrix_1_2" in content
+        
+        # Should NOT have array dimensions on any of the unrolled interfaces
+        for i in range(2):
+            for j in range(3):
+                assert f"m_apb_matrix_{i}_{j} [" not in content