Coe fix #432
Coe fix #432
Conversation
E2E Integration Test SummaryWorkflow Run: 370 Test ResultsEnd-to-End Tests❌ E2E Tests: FAILED Artifacts
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces "Core fixes" including Vivado synthesis compatibility improvements, behavioral simulation enhancements, and PCIe timing configurations. The main changes address synthesis errors in Vivado, add behavioral register simulation for device types, and improve template flexibility.
- Fix Vivado synthesis issues with ram_init_file attributes and wire declarations
- Add comprehensive behavioral simulation framework for network, storage, and media devices
- Enhance PCIe clock configuration support for 7-series FPGAs
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_top_level_wrapper_template.py | Update test to handle both wire and assign declarations for cfg_mgmt_wr_rw1c_as_rw |
| tests/test_oui_dsn_extraction.py | Add comprehensive tests for OUI extraction and DSN decomposition functionality |
| tests/test_constraints_template_defaults.py | Add PCIe clock parameters for 7-series support in template tests |
| tests/test_cfg_shadow_template.py | Add regression tests for Vivado synthesis compatibility issues |
| tests/test_behavioral.py | Add complete test suite for behavioral simulation framework |
| src/utils/unified_context.py | Add PCIe clock configuration and behavioral context integration |
| src/utils/behavioral_context.py | New module for behavioral simulation context building |
| src/templating/tcl_builder.py | Add board part ID mapping for Xilinx development boards |
| src/templating/sv_context_builder.py | Add vendor OUI extraction and DSN semantic decomposition |
| src/templates/tcl/pcileech_generate_project.j2 | Enhance project setup with comprehensive IP and fileset configuration |
| src/templates/tcl/ip_config.j2 | Add PCIe clock configuration parameters |
| src/templates/tcl/constraints.j2 | Update timing constraints with PCIe clock references |
| src/templates/sv/top_level/internal_signals.sv.j2 | Fix cfg_mgmt_wr_rw1c_as_rw wire declaration and add config handshaking |
| src/templates/sv/top_level/bar_controller_block.sv.j2 | Remove duplicate cfg_mgmt_wr_rw1c_as_rw assignment |
| src/templates/sv/pcie_endpoint_defines.sv.j2 | New template for PCIe endpoint device-specific defines |
| src/templates/sv/cfg_shadow.sv.j2 | Fix Vivado synthesis compatibility by removing ram_init_file attributes |
| src/templates/sv/behavioral/pcileech_bar_behavioral.sv.j2 | New template for behavioral device simulation |
| src/device_clone/fallback_manager.py | Add PCIe clock parameters to fallback configuration |
| src/cli/cli.py | Fix version display to show current version instead of latest |
| src/behavioral/ | New behavioral simulation framework with network, storage, and media device support |
| src/version.py | Version bump to 0.13.39 |
| CHANGELOG.rst | Add entries for versions 0.13.38 and 0.13.39 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/utils/behavioral_context.py
Outdated
| import logging | ||
| from typing import Dict, Any, Optional | ||
|
|
||
| from string_utils import log_info_safe, log_warning_safe, log_debug_safe, safe_format |
There was a problem hiding this comment.
The import path 'string_utils' is incorrect. Based on the other files in the codebase, this should be 'from src.string_utils import ...' to maintain consistency with the module structure.
| from string_utils import log_info_safe, log_warning_safe, log_debug_safe, safe_format | |
| from src.string_utils import log_info_safe, log_warning_safe, log_debug_safe, safe_format |
Copilot uses AI. Check for mistakes.
| import logging | ||
| from typing import Dict, Any, Optional | ||
|
|
||
| from string_utils import log_info_safe, safe_format |
There was a problem hiding this comment.
The import path 'string_utils' is incorrect. Based on the codebase structure, this should be 'from src.string_utils import ...' to maintain consistency.
Copilot uses AI. Check for mistakes.
| import logging | ||
| from typing import Dict, Any, Optional | ||
|
|
||
| from string_utils import log_info_safe, safe_format |
There was a problem hiding this comment.
The import path 'string_utils' is incorrect. Based on the codebase structure, this should be 'from src.string_utils import ...' to maintain consistency.
Copilot uses AI. Check for mistakes.
| import logging | ||
| from typing import Dict, Any, Optional | ||
|
|
||
| from string_utils import log_info_safe, safe_format |
There was a problem hiding this comment.
The import path 'string_utils' is incorrect. Based on the codebase structure, this should be 'from src.string_utils import ...' to maintain consistency.
| from string_utils import log_info_safe, safe_format | |
| from src.string_utils import log_info_safe, safe_format |
Copilot uses AI. Check for mistakes.
| from dataclasses import dataclass | ||
| from enum import Enum | ||
|
|
||
| from string_utils import log_error_safe, log_debug_safe, safe_format |
There was a problem hiding this comment.
The import path 'string_utils' is incorrect. Based on the codebase structure, this should be 'from src.string_utils import ...' to maintain consistency.
| from string_utils import log_error_safe, log_debug_safe, safe_format | |
| from src.string_utils import log_error_safe, log_debug_safe, safe_format |
Copilot uses AI. Check for mistakes.
| import logging | ||
| from typing import Any, Optional | ||
|
|
||
| from string_utils import log_info_safe, log_warning_safe, safe_format |
There was a problem hiding this comment.
The import path 'string_utils' is incorrect. Based on the codebase structure, this should be 'from src.string_utils import ...' to maintain consistency.
| from string_utils import log_info_safe, log_warning_safe, safe_format | |
| from src.string_utils import log_info_safe, log_warning_safe, safe_format |
Copilot uses AI. Check for mistakes.
| def require(condition: bool, message: str, **context) -> None: | ||
| """Validate condition or exit with error.""" | ||
| if not condition: | ||
| log_error_safe(safe_format("Build aborted: {msg} | ctx={ctx}", |
There was a problem hiding this comment.
The log_error_safe function call is missing the logger parameter as the first argument. This should be 'log_error_safe(logger, safe_format(...))' to match the function signature used elsewhere in the codebase.
| log_error_safe(safe_format("Build aborted: {msg} | ctx={ctx}", | |
| log_error_safe(logger, safe_format("Build aborted: {msg} | ctx={ctx}", |
Copilot uses AI. Check for mistakes.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| """Base behavioral register infrastructure for device simulation.""" | ||
|
|
||
| import logging | ||
| from typing import Dict, Any, List, Optional |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the problem, remove the unused import of List from the statement from typing import Dict, Any, List, Optional on line 5 in src/behavioral/base.py. The best way is to edit the import so it only includes the names that are still needed (Dict, Any, and Optional). No other changes are required, and this does not affect any functionality of the code.
| @@ -2,7 +2,7 @@ | ||
| """Base behavioral register infrastructure for device simulation.""" | ||
|
|
||
| import logging | ||
| from typing import Dict, Any, List, Optional | ||
| from typing import Dict, Any, Optional | ||
| from dataclasses import dataclass | ||
| from enum import Enum | ||
|
|
| """Behavioral simulation for media controllers (audio/video).""" | ||
|
|
||
| import logging | ||
| from typing import Dict, Any, Optional |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
The correct fix is to remove the unused Dict from the import statement on line 5. The import of Any and Optional should be retained because both are used in type annotations present in the shown code (device_config: Any, -> Optional[BehavioralSpec]).
Change the import line from:
from typing import Dict, Any, Optionalto:
from typing import Any, OptionalNo further changes are needed.
| @@ -2,7 +2,7 @@ | ||
| """Behavioral simulation for media controllers (audio/video).""" | ||
|
|
||
| import logging | ||
| from typing import Dict, Any, Optional | ||
| from typing import Any, Optional | ||
|
|
||
| from string_utils import log_info_safe, safe_format | ||
| from .base import ( |
| """Behavioral simulation for network controllers.""" | ||
|
|
||
| import logging | ||
| from typing import Dict, Any, Optional |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the problem, remove the import of Dict from the typing module while keeping the imports for Any and Optional, since those are used.
In file src/behavioral/network_behavioral.py, edit line 5 to read:
from typing import Any, OptionalThis ensures that all imported symbols are actually used in the code, improving code clarity and slightly reducing overhead.
| @@ -2,7 +2,7 @@ | ||
| """Behavioral simulation for network controllers.""" | ||
|
|
||
| import logging | ||
| from typing import Dict, Any, Optional | ||
| from typing import Any, Optional | ||
|
|
||
| from string_utils import log_info_safe, safe_format | ||
| from .base import ( |
| """Behavioral simulation for storage controllers.""" | ||
|
|
||
| import logging | ||
| from typing import Dict, Any, Optional |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the unused import, we should remove Dict from the list of imports from the typing module. The best practice is to only import what is actually used (Any, Optional). The change should occur on line 5 of src/behavioral/storage_behavioral.py, updating it from from typing import Dict, Any, Optional to from typing import Any, Optional. This edit removes the unused import, thus cleaning up the codebase with no effect on existing functionality.
| @@ -2,7 +2,7 @@ | ||
| """Behavioral simulation for storage controllers.""" | ||
|
|
||
| import logging | ||
| from typing import Dict, Any, Optional | ||
| from typing import Any, Optional | ||
|
|
||
| from string_utils import log_info_safe, safe_format | ||
| from .base import ( |
| """Tests for behavioral device simulation.""" | ||
|
|
||
| import pytest | ||
| from typing import Dict, Any |
Check notice
Code scanning / CodeQL
Unused import Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix this issue, the unused import statement from typing import Dict, Any on line 5 should be entirely removed from the file tests/test_behavioral.py. This change should be limited only to the import line itself and must not affect any other logic or functionality in the test file, as those two types are not used anywhere in the provided code. No further additions or definitions are necessary. Just delete line 5.
| @@ -2,7 +2,6 @@ | ||
| """Tests for behavioral device simulation.""" | ||
|
|
||
| import pytest | ||
| from typing import Dict, Any | ||
| from unittest.mock import Mock, MagicMock | ||
|
|
||
| from src.behavioral.base import ( |
|
|
||
| import pytest | ||
| from typing import Dict, Any | ||
| from unittest.mock import Mock, MagicMock |
Check notice
Code scanning / CodeQL
Unused import Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the problem, we should remove the unused MagicMock import from the import statement on line 6, while leaving the Mock import intact since it is used elsewhere in the file. Specifically, we should modify line 6 in tests/test_behavioral.py to import only Mock from unittest.mock.
| @@ -3,7 +3,7 @@ | ||
|
|
||
| import pytest | ||
| from typing import Dict, Any | ||
| from unittest.mock import Mock, MagicMock | ||
| from unittest.mock import Mock | ||
|
|
||
| from src.behavioral.base import ( | ||
| BehavioralSpec, |
| result = self.render_template(template_content, minimal_context) | ||
|
|
||
| # Find all synthesis attributes | ||
| attr_pattern = r'\(\*[^)]+\*\)\s*(?:logic|reg|wire)?\s*(?:\[[^\]]+\])?\s*(\w+)' |
Check notice
Code scanning / CodeQL
Unused local variable Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the problem, remove the assignment to the unused variable attr_pattern on line 648 of tests/test_cfg_shadow_template.py. Since the variable is never used, and assigning a string literal to a variable has no side effects, simply deleting the assignment is safe and will not affect the functionality of the code. No additional imports, definitions, or changes elsewhere are necessary.
| @@ -645,7 +645,7 @@ | ||
| result = self.render_template(template_content, minimal_context) | ||
|
|
||
| # Find all synthesis attributes | ||
| attr_pattern = r'\(\*[^)]+\*\)\s*(?:logic|reg|wire)?\s*(?:\[[^\]]+\])?\s*(\w+)' | ||
| # attr_pattern removed: unused variable | ||
|
|
||
| # Check hash table declarations specifically (they were problematic) | ||
| hash_table_attrs = re.findall( |
| # Add src to path for imports | ||
| sys.path.insert(0, str(Path(__file__).parent.parent)) | ||
|
|
||
| from src.string_utils import log_info_safe, safe_format |
Check notice
Code scanning / CodeQL
Unused import Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the problem, we should delete the unused import statement on line 22: from src.string_utils import log_info_safe, safe_format. Since neither log_info_safe nor safe_format are used in this file, their import is unnecessary, and removing this line will improve code clarity and reduce unnecessary dependencies. The rest of the code does not need to be changed.
| @@ -19,7 +19,6 @@ | ||
| # Add src to path for imports | ||
| sys.path.insert(0, str(Path(__file__).parent.parent)) | ||
|
|
||
| from src.string_utils import log_info_safe, safe_format | ||
| from src.templating.sv_context_builder import SVContextBuilder | ||
|
|
||
|
|
E2E Integration Test SummaryWorkflow Run: 372 Test ResultsEnd-to-End Tests❌ E2E Tests: FAILED Artifacts
|
E2E Integration Test SummaryWorkflow Run: 374 Test ResultsEnd-to-End Tests❌ E2E Tests: FAILED Artifacts
|
| from dataclasses import dataclass, field | ||
| from pathlib import Path | ||
| from typing import Any, Callable, Dict, List, Optional, Protocol, Tuple, Union | ||
| from typing import Any, Callable, Dict, List, Optional, Tuple, Union |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To resolve the issue, we need to remove only Callable from the import statement on line 23. This means editing line 23 to remove Callable from the comma-separated list imported from typing. No other changes are required, as the rest of the code will continue to import the actually used type hinting symbols. This change should occur precisely at line 23, maintaining all other existing imports and code structure.
| @@ -20,7 +20,7 @@ | ||
| from concurrent.futures import ThreadPoolExecutor, as_completed | ||
| from dataclasses import dataclass, field | ||
| from pathlib import Path | ||
| from typing import Any, Callable, Dict, List, Optional, Tuple, Union | ||
| from typing import Any, Dict, List, Optional, Tuple, Union | ||
|
|
||
| from src.string_utils import ( | ||
| log_debug_safe, |
| """Behavioral simulation context integration.""" | ||
|
|
||
| import logging | ||
| from typing import Dict, Any, Optional |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the problem, remove Optional from the import statement on line 5, since it is not used anywhere in the file. Only retain the imports from typing that are actually referenced in the code (Dict, Any). This change should be made only to the import statement at the top of src/utils/behavioral_context.py, and no other modifications are required.
| @@ -2,7 +2,7 @@ | ||
| """Behavioral simulation context integration.""" | ||
|
|
||
| import logging | ||
| from typing import Dict, Any, Optional | ||
| from typing import Dict, Any | ||
|
|
||
| from src.string_utils import log_info_safe, log_warning_safe, log_debug_safe, safe_format | ||
| from src.behavioral.analyzer import BehavioralAnalyzerFactory |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
1 participant
No description provided.