Documentation, formatting, and linting for several modules: seq_gen, fast_spi_rx, gpio, spi, pulse_gen

[kcaisley]

I'm opening a pull here, to merge in some of the incremental changes I've been making. The changes are broadly:

• Update to the fast_spi_rx modules, so that it supports variable output data size
• Changed main readme RST file to markdown
• Cleaned up docstrings across touched python hardware drivers, to flesh out documenation and make peace with ruff lint
• Add include guards across a majority of basil firmware modules, to allow verible/verilator linting. Previously the linting was too noisy to even begin to imagine one could use it.
• Add RST documenation for fast_spi_rx module
• Fix some typos and formatting in other RST files (nothing major)
• Change to behaviral description in blk_mem_gen_8_to_1_2k, instead of hardcoded RAMB16_S1_S9
• Update/add behavioral modules for several FPGA primitives, like IOBUF, IDDR, IBUFDS, etc
• Add an icon for the RTD page (e.g. a favicon)

[cbespin]

I have one remaining question: why are the include guards labelled as BASIL_GPAC_ADC_RX_GPAC_ADC_CORE_V? To reflect the include path gpac_adc_rx/gpac_adc_rx_core.v? I am not sure if I like that too much, honestly. It makes it a bit hard to read/understand and could probably be solved with GPAC_ADC_RX_V and GPAC_ADC_RX_V without losing information?
Am I wrong or is there a common standard how it is done? Just curios and genuinely asking.

[kcaisley]

Good point @cbespin . I previously ran something like this to add the guards:

from pathlib import Path

root = Path("basil/firmware/modules")

for path in root.rglob("*.v"):
    text = path.read_text()

    # Construct guard from the path below basil/firmware/modules.
    # Example:
    #   gpac_adc_rx/gpac_adc_rx_core.v
    # becomes:
    #   BASIL_GPAC_ADC_RX_GPAC_ADC_RX_CORE_V
    guard = (
        "BASIL_"
        + str(path.relative_to(root).with_suffix(""))
        .upper()
        .replace("/", "_")
        + "_V"
    )

    # Insert the include guard after the initial copyright/header block.
    text = text.replace(
        "*/\n",
        f"*/\n`ifndef {guard}\n`define {guard}\n\n",
        1,
    )

    # Add closing `endif` at the end of the file.
    text = text.rstrip() + "\n\n`endif\n"

    path.write_text(text)

which produced the scheme BASIL_ + PATH + MODULE _V naming to avoid collisions, but it can just be simplified to the modules name.

I'll push a change with the simplified naming in a bit.

[kcaisley]

My reason for adding include guards, is the following: Our tests and build systems currently have a mixture of in-file includes plus added file paths in the tool execution paths. I'd like to introduce linting for the verilog files in the repo, but verilog and verible lint are quite picky about double or missing includes. So to quiet the noise, these guards insure each file is included only once.

[awalsemann]

Is there a reason to remove the __FILE__ and __LINE__ command from assertions?
These should be valid compiler directives in verilog and might provides useful insights into why an assertion failed.

[cbespin]

The comment also applies to all other simulation-replacement modules, of course 😀

[kcaisley]

Is there a reason to remove the __FILE__ and __LINE__ command from assertions? These should be valid compiler directives in verilog and might provides useful insights into why an assertion failed.

For posterity, I had removed this because the files were linted without SystemVerilog 2009 mode being enabled. This had given the warning preprocessing error at "__FILE__" Error expanding macro identifier, might not be defined before. .

However, as long as verible-verilog-syntax --lang sv is run, I guess this warning shouldn't appear.

[kcaisley]

Okay, I think this is ready on my end. In my opinion, in the future, we should look to remove the include guards entirely, in favor of explicit targets in the build system (whether that be a synthesis run with vivado or a simulation with cocotb plus verilator or iverilog).

To avoid having to reproduce the same config across these different targets, I think a library like siliconcompiler would be ideal.

[cbespin]

Nice changes, looks good to me now!

[kcaisley]

Oops! I merged in the in the changes from #273 quickly, using the Github web editor, and didn't notice a a missing ] in the pyproject.toml.

Hot fixing this in #276 !