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

Expose force/release to cocotb #1403

Merged
merged 1 commit into from
Feb 21, 2020
Merged

Expose force/release to cocotb #1403

merged 1 commit into from
Feb 21, 2020

Conversation

thasti
Copy link
Contributor

@thasti thasti commented Feb 17, 2020

I finally got around to looking at #657. I have mostly re-applied the patch prepared by @lukedarnell to current master, fixed small issues with it and gave it a test.

As it stands, a testbench running on a VPI simulator can do:

from cocotb.handle import Force, Release, Deposit
...
dut.q <= 0 # default action: deposit
dut.q <= Deposit(0) # does the same as above
dut.q <= Force(1) # forces the net to 1
dut.q <= Release() # releases the net

What's still to be done:

  • add documentation
  • implement the mechanisms also for VHPI/FLI
  • add a proper unit test/example

I'd already like to hear whether there are some thoughts on how it was implemented to figure out if any significant changes would be required in your opinion.

@imphil
Copy link
Member

imphil commented Feb 17, 2020

This PR has a large number of conflicts. Can you try to rebase on top of the current master?

@thasti
Copy link
Contributor Author

thasti commented Feb 17, 2020

Indeed, I was a bit behind current master. Should be better now :)

Copy link
Member

@imphil imphil left a comment

Choose a reason for hiding this comment

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

Thanks for the rebase, much easier to read now. I'm fine with the general approach and the implementation. Things do to before this can be merged:

  • Please add documentation: this needs doc comments in the SetAction classes, and a couple paragraphs of text in the general user guide.
  • Please also add a newsfragment, please see https://github.com/cocotb/cocotb/blob/master/documentation/source/newsfragments/README.rst for details.
  • Please combine the commits into one and add a descriptive commit message. You can reset the author to be you, but please mention Julius Baxter and Luke Darnell as co-authors.
  • Ensure that you have a Fixes #657 in the commit message as well.

cocotb/handle.py Outdated Show resolved Hide resolved
cocotb/share/lib/vpi/VpiCbHdl.cpp Show resolved Hide resolved
cocotb/handle.py Outdated Show resolved Hide resolved
@eric-wieser eric-wieser self-requested a review February 17, 2020 12:18
@thasti thasti force-pushed the force_release branch 7 times, most recently from 8fb99e1 to 890f4d0 Compare February 17, 2020 20:29
cocotb/share/lib/vpi/VpiCbHdl.cpp Outdated Show resolved Hide resolved
tests/test_cases/test_force_release/Makefile Outdated Show resolved Hide resolved
cocotb/handle.py Outdated Show resolved Hide resolved
cocotb/handle.py Outdated Show resolved Hide resolved
documentation/source/library_reference.rst Outdated Show resolved Hide resolved
cocotb/handle.py Outdated Show resolved Hide resolved
cocotb/handle.py Outdated Show resolved Hide resolved
@thasti thasti force-pushed the force_release branch 2 times, most recently from 7d479ba to a8f08c7 Compare February 17, 2020 21:50
cocotb/handle.py Outdated Show resolved Hide resolved
cocotb/handle.py Outdated Show resolved Hide resolved
@thasti thasti force-pushed the force_release branch 2 times, most recently from 2146c7c to b546a46 Compare February 17, 2020 22:06
@thasti
Copy link
Contributor Author

thasti commented Feb 17, 2020

Any chance someone can test the VHPI implementation? I do not have access to a simulator implementing VHPI at the moment.

@eric-wieser
Copy link
Member

@themperek can probably test that

@eric-wieser eric-wieser added the type:feature new or enhanced functionality label Feb 17, 2020
@thasti thasti force-pushed the force_release branch 3 times, most recently from e16d9d3 to 387a5c8 Compare February 18, 2020 16:20
@thasti
Copy link
Contributor Author

thasti commented Feb 18, 2020

Many thanks everyone for the constructive review yesterday. Today I was able to check the patch also compiles in a Python 3.6.8 environment and the testcase correctly works for Cadence Xcelium and Incisive. On my other machine (Python 3.8) I can test against iverilog (works fine) and ghdl (runs, but force/release is not implemented).

Let me know in case there's something else you'd like to see happening for this PR.

Edit: I realized that I can test VHPI by using TOPLEVEL_LANG=vhdl in conjunction with incisive and xcelium. Both worked fine.

This commit adds the notion of SetActions to simulator handles. The
default action type is 'Deposit', and actions for 'Force', 'Release' and
'Freeze' are added initially. These actions allow placing procedural continuous
assignments, which can be useful in the context of fault injection,
testbench qualification or mutation coverage.

The implementation is based on work done by Jules Baxter <julius.baxter@broadcom.com>
and Luke Darnell <luke.darnell@broadcom.com>.

Fixes cocotb#657
@ktbarrett
Copy link
Member

"Force or release action not supported for FLI."

This isn't true, mti_ForceSignal has the ability to force, deposit, freeze, (and drive!) a signal, and mti_ReleaseSignal can release a signal. They are just not what is currently used to write to signal handles. The drive method is particularly interesting and is probably worth another SetValueAction, even if only Questa will be able to do it right now.

@thasti
Copy link
Contributor Author

thasti commented Feb 19, 2020

I'm aware of that. Perhaps 'not implemented' is the wording I should have used instead. In case I can establish access to Questa before this is merged I may try also adding an FLI implementation.

@ktbarrett
Copy link
Member

Ah, if you don't have access to Questa, and have no way to test it, it's probably best to leave it out of this PR. I might take a look at adding a more thorough FLI implementation after this PR is in, it's definitely something we want. Thank you for the work.

@thasti thasti changed the title WIP: Expose force/release to cocotb Expose force/release to cocotb Feb 20, 2020
@thasti thasti requested a review from imphil February 21, 2020 08:19
Copy link
Member

@imphil imphil left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for all reviews everyone, and thanks @thasti for following up on those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature new or enhanced functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants