Skip to content

Develop/v1.1 - #12

Merged
VinhTechiee merged 3 commits into
mainfrom
develop/v1.1
Jul 28, 2026
Merged

Develop/v1.1#12
VinhTechiee merged 3 commits into
mainfrom
develop/v1.1

Conversation

@VinhTechiee

Copy link
Copy Markdown
Owner

No description provided.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 164029107a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@@ -0,0 +1,15 @@
============================================================

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Match the waveform test's top module to its filename

Adding this golden file makes test_010 an active regression, but the Vivado path derives the top-level name from CPU_tb_wave.v and invokes xelab with CPU_tb_wave, while the file still declares module CPU_tb. Consequently, running the documented suite with --sim vivado fails to elaborate this test; rename either the module or the file so the runner selects an existing design unit.

Useful? React with 👍 / 👎.

Comment thread README.md
Comment on lines +248 to +251
- 32 addressable locations
- 8-bit data width
- asynchronous read
- synchronous write

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Document the memory read as synchronous

Change this to synchronous read: Memory.v only updates data_out inside always @(posedge clk) when rd && !wr, so consumers do not receive a newly addressed value asynchronously. The current description can lead users extending the datapath or writing timing-sensitive tests to assume behavior the RTL does not provide.

Useful? React with 👍 / 👎.

Comment thread README.md
Comment on lines +327 to +331
| `test_002` | Memory-module verification |
| `test_003` | Instruction-register verification |
| `test_004` | Accumulator verification |
| `test_005` | ALU arithmetic and logical operations |
| `test_006` | Controller FSM verification |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Align the test coverage table with the actual directories

Correct these mappings before directing users to select a test by name: test_002 contains AM_tb.v, test_003 contains ALU_tb.v, test_004 contains Controller_tb.v, and the later entries are similarly shifted (test_005 is IR, test_006 is AC, and test_007 is Memory). Following this table therefore runs the wrong verification target for most module-level tests.

Useful? React with 👍 / 👎.

Comment thread README.md
Comment on lines +615 to +617
## License

This project is released under the **MIT License**. See the `LICENSE` file for details.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add the referenced license file or remove the claim

Do not direct users to a nonexistent file: a repository-wide tree search of this commit contains no LICENSE or COPYING file, so the stated licensing terms cannot be inspected from the project and the README link is broken. Add the intended MIT license text or revise this section and the license badge accordingly.

Useful? React with 👍 / 👎.

@VinhTechiee
VinhTechiee merged commit 2df8195 into main Jul 28, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant