Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SpiBus Implementation #17

Open
Scafir opened this issue Jun 20, 2024 · 1 comment · May be fixed by #18
Open

SpiBus Implementation #17

Scafir opened this issue Jun 20, 2024 · 1 comment · May be fixed by #18

Comments

@Scafir
Copy link
Contributor

Scafir commented Jun 20, 2024

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 :)

@Scafir Scafir linked a pull request Jun 20, 2024 that will close this issue
@schang412
Copy link
Owner

Hi, this is a good implementation. But it has some breaking change. I took another similar pull request instead. If there are any needs for other breaking change in the future, then we can use your suggested changes.

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 a pull request may close this issue.

2 participants