schang412 / cocotbext-spi

MIT License
20 stars 10 forks source link

SpiBus Implementation #17

Open Scafir opened 3 months ago

Scafir commented 3 months ago

Hi,

The problem

Following the last update, I discovered that some of my testbenches broke and cannot be easily ported to the new SpiBus implementation.

The first problem is that a naming convention is imposed to the user for the naming of the bus. This is annoying in my project where input and outputs of a module must be suffixed by xDI and xDO, respectively. This problem can be worked around by instantiating a cocotb_bus.Bus directly, bypassing the SpiBus.

The second problem always existed in this project, but it was possible to work around it previously: not all devices have all four sclk, miso, mosi and cs. For example, the AD5542 does not have a miso signal. Previously, I could pass a dummy BinaryValue to spi_signals and get to where I wanted. Now, with cocotb_bus only accepting strings and looking for the corresponding signal on its own, such workaround is not possible anymore.

Of course, it would be possible to simply use a wrapper around my HDL code to present all four signals with proper naming. I however think that it is not a good approach, as (1) writing the wrapper requires work and more importantly (2), it makes my code less scalable, as I can not reuse my code in a module that contains my AD5542 frontend without writing wrappers for them too.

Suggested solution

SpiBus could be rewritten so that it is more of a wrapper around cocotb_bus's Bus. Hence allowing for more freedom in how signals are named. The presence of the "optional" signals would then need to be checked in SpiMaster or SpiMasterSlave, and assigned dummies if not present.

This approach would only break the cs_name compatibility, but the fix would be quite straightforward by using the signals argument. It can be added back with a bit of logic.

I have implemented these changes and will open a pull request, if you would like to check it out. In any case, thank you for your work :)