Feat/tlp emoulation #523
Feat/tlp emoulation #523
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| # Mock write_text to capture the content without actual file write | ||
| with patch("pathlib.Path.write_text") as mock_write: | ||
| # Create the build script | ||
| script_path = integration.create_unified_build_script("artix7") |
|
|
||
| # Mock write_text to capture the content | ||
| with patch("pathlib.Path.write_text") as mock_write: | ||
| script_path = integration.create_unified_build_script("artix7") |
|
|
||
| # Mock write_text to capture the content | ||
| with patch("pathlib.Path.write_text") as mock_write: | ||
| script_path = integration.create_unified_build_script("artix7") |
|
|
||
| # Mock write_text to capture the content | ||
| with patch("pathlib.Path.write_text") as mock_write: | ||
| script_path = integration.create_unified_build_script("pcileech_100t484_x1") |
|
|
||
| # Mock write_text to capture the content | ||
| with patch("pathlib.Path.write_text") as mock_write: | ||
| script_path = integration.create_unified_build_script(board_name) |
E2E Integration Test SummaryWorkflow Run: 759 Test ResultsEnd-to-End Tests❌ E2E Tests: FAILED Artifacts
|
There was a problem hiding this comment.
Pull request overview
This pull request adds TLP (Transaction Layer Packet) latency emulation functionality to enhance PCIe device cloning realism. The main changes include adding a new TLP latency emulator module in SystemVerilog, integrating it into the BAR controller, extending timing configuration parameters, supporting additional PCILeech board variants, and improving code quality through formatting and error handling improvements.
Key Changes:
- Adds a new TLP latency emulator SystemVerilog module that injects realistic response delays with optional jitter based on LFSR pseudo-random generation
- Extends TimingParameters with optional TLP latency fields (min/max/avg read latency, jitter enable flag, LFSR seed) and logic to calculate these from behavior profile timing patterns
- Adds support for 6 new PCILeech board configurations (100T x4, GBOX, NeTV2 variants, ScreamerM2, AC701)
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_pcileech_build_integration.py | Adds comprehensive tests for relative path resolution in Vivado build scripts |
| tests/test_build.py | Updates error message assertion to be more flexible |
| src/vivado_handling/vivado_utils.py | Removes unused sys import |
| src/vivado_handling/pcileech_build_integration.py | Updates build script generation to use relative paths instead of absolute paths for better portability |
| src/tui/models/configuration.py | Adds 6 new board types to the configuration options |
| src/tui/main.py | Fixes BuildConfiguration conversion between dict and object when opening dialog |
| src/tui/core/ui_coordinator.py | Refactors device table update to use VirtualDeviceTable's set_data method |
| src/templates/sv/tlp_latency_emulator.sv.j2 | New SystemVerilog module implementing TLP response latency emulation with LFSR-based jitter |
| src/templates/sv/pcileech_tlps128_bar_controller.sv.j2 | Integrates TLP latency emulator into BAR controller when timing config enables it |
| src/file_management/repo_manager.py | Adds repository path mappings for 6 new board types |
| src/file_management/donor_dump_manager.py | Improves formatting and shortens warning messages |
| src/file_management/board_discovery.py | Adds board configurations and clock constraints for 6 new boards, removes unused import |
| src/device_clone/pcileech_context.py | Extends TimingParameters with TLP latency fields and adds calculation logic from behavior profiles |
| src/device_clone/constants.py | Adds FPGA part definitions for 6 new board types |
| src/build_helpers.py | Adds build strategies for Artix-7 100T and 200T FPGAs |
| pcileech.py | Extensive formatting improvements, consolidated error messages, enhanced exception handling with specific exception types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Set defaults for optional TLP latency parameters if not provided | ||
| if self.min_read_latency is None: | ||
| self.min_read_latency = max(1, int(self.read_latency * 0.8)) | ||
| if self.max_read_latency is None: | ||
| self.max_read_latency = int(self.read_latency * 1.2) | ||
| if self.avg_read_latency is None: | ||
| self.avg_read_latency = self.read_latency |
There was a problem hiding this comment.
Dataclass field mutation in post_init may not work correctly with slots=True in Python 3.9 and earlier. The TimingParameters dataclass uses slots=True (line 350), and the post_init method modifies optional fields. While this works in Python 3.10+, it may cause issues in earlier versions. Consider initializing these fields with field(default_factory=...) instead of setting them in post_init.
| # TLP latency emulation parameters (optional, for enhanced realism) | ||
| min_read_latency: Optional[int] = None | ||
| max_read_latency: Optional[int] = None | ||
| avg_read_latency: Optional[int] = None | ||
| enable_latency_jitter: bool = False | ||
| latency_lfsr_seed: int = 0xACE1 |
There was a problem hiding this comment.
The new TLP latency emulation parameters added to TimingParameters lack test coverage. The existing test_timing_parameters_validation test doesn't verify the new optional fields (min_read_latency, max_read_latency, avg_read_latency, enable_latency_jitter, latency_lfsr_seed) or their default initialization in post_init. Consider adding test cases to verify these fields are properly initialized and validated.
| ) i_tlp_latency_emulator ( | ||
| .rst ( rst ), | ||
| .clk ( clk ), | ||
| .req_ctx ( bar_rsp_ctx[bar_idx] ), | ||
| .req_data ( bar_rsp_data[bar_idx] ), | ||
| .req_valid ( bar_rsp_valid[bar_idx] ), | ||
| .rsp_ctx ( bar_latency_rsp_ctx[bar_idx] ), | ||
| .rsp_data ( bar_latency_rsp_data[bar_idx] ), | ||
| .rsp_valid ( bar_latency_rsp_valid[bar_idx] ) | ||
| ); |
There was a problem hiding this comment.
Missing req_ready output connection in the TLP latency emulator instantiation. The module defines req_ready as an output, but it's not connected in the instantiation. This will cause a synthesis error as the module interface requires all ports to be connected.
| .req_valid ( bar_rsp_valid[bar_idx] ), | ||
| .rsp_ctx ( bar_latency_rsp_ctx[bar_idx] ), | ||
| .rsp_data ( bar_latency_rsp_data[bar_idx] ), | ||
| .rsp_valid ( bar_latency_rsp_valid[bar_idx] ) |
There was a problem hiding this comment.
Missing rsp_ready input connection in the TLP latency emulator instantiation. The module defines rsp_ready as an input for backpressure handling, but it's not connected. This will cause a synthesis error and prevent proper flow control of responses.
| .rsp_valid ( bar_latency_rsp_valid[bar_idx] ) | |
| .rsp_valid ( bar_latency_rsp_valid[bar_idx] ), | |
| .rsp_ready ( 1'b1 ) |
| min_latency = min(min_latency, latency_cycles - dev_cycles) | ||
| max_latency = max(max_latency, latency_cycles + dev_cycles) |
There was a problem hiding this comment.
Potential negative latency calculation. When calculating min_latency, subtracting dev_cycles from latency_cycles could result in a negative value. While there's a max(1, int(min_latency)) call later at line 2386, this happens after all patterns are processed. If the first pattern has a large deviation, min_latency could become negative. Consider adding max(1, latency_cycles - dev_cycles) at the calculation point.
| # Mock write_text to capture the content without actual file write | ||
| with patch("pathlib.Path.write_text") as mock_write: | ||
| # Create the build script | ||
| script_path = integration.create_unified_build_script("artix7") |
There was a problem hiding this comment.
Variable script_path is not used.
|
|
||
| # Mock write_text to capture the content | ||
| with patch("pathlib.Path.write_text") as mock_write: | ||
| script_path = integration.create_unified_build_script("artix7") |
There was a problem hiding this comment.
Variable script_path is not used.
|
|
||
| # Mock write_text to capture the content | ||
| with patch("pathlib.Path.write_text") as mock_write: | ||
| script_path = integration.create_unified_build_script("artix7") |
There was a problem hiding this comment.
Variable script_path is not used.
|
|
||
| # Mock write_text to capture the content | ||
| with patch("pathlib.Path.write_text") as mock_write: | ||
| script_path = integration.create_unified_build_script("pcileech_100t484_x1") |
There was a problem hiding this comment.
Variable script_path is not used.
| script_path = integration.create_unified_build_script("pcileech_100t484_x1") | |
| integration.create_unified_build_script("pcileech_100t484_x1") |
|
|
||
| # Mock write_text to capture the content | ||
| with patch("pathlib.Path.write_text") as mock_write: | ||
| script_path = integration.create_unified_build_script(board_name) |
There was a problem hiding this comment.
Variable script_path is not used.
| script_path = integration.create_unified_build_script(board_name) | |
| integration.create_unified_build_script(board_name) |
No description provided.