FPGA-Research-Manchester / FABulous

Fabric generator and CAD tools
https://fabulous.readthedocs.io/en/latest/
Apache License 2.0
141 stars 30 forks source link

Merge Verilog and VHDL project templates #155

Open mole99 opened 4 months ago

mole99 commented 4 months ago

To ease maintainability, it would make sense to merge FABulous_project_template_verilog and FABulous_project_template_vhdl into a single FABulous_project_template.

Many files in both directories are duplicated and some files/filenames already diverge.

My idea is that after merging #154, the logic for reading the tile configuration can be changed a bit:

Instead of specifying file extensions for HDL sources in BEL entries inside tile configs, omit them:

BEL,./InPass4_frame_config_mux,RAM2FAB_D0_

Depending on whether a Verilog/VHDL writer is used, append the correct file ending (.v, .vhdl). Since both Verilog and VHDL files are in the tile directory, the correct one can be picked up.

What do you think? Am I missing something?

KelvinChung2000 commented 4 months ago

The main problem would be that the single template would contain both the VHDL and Verilog files, which might confuse the users. We can solve this by copying only the VHDL or Verilog files according to which writer is used.

Another potential problem is that the VHDL side Test folder is slightly different. But we can resolve it by selectively copying the file.

Ultimately, we need to find a better way to maintain and handle both Verilog and VHDL backend to avoid problems like #153

mole99 commented 4 months ago

Speaking as a user, I wouldn't find this so confusing as I know which backend I am using (Verilog/VHDL) and I could easily switch between the two. But yeah, selectively copying the files would be an option to avoid that.

What way would you think of to improve maintainability?

I think it would be worthwhile to setup the CI with a test suite for both Verilog and VHDL, covering various fabrics and features such as supertiles. Every time a new feature PR is opened, the CI runs all the tests to make sure no regressions have occurred. Simple regressions like in #153 should then not be possible.

But I guess that's for another issue ;)

KelvinChung2000 commented 4 months ago

Yes, this should improve maintainability, less duplication is always better.

mole99 commented 4 months ago

I'll open an issue regarding the CI then, if that's okay.

I'll also open some other issues that improve maintainablity of FABulous or make it easier for new users to get started.