search

Origen-SDK / origen_sim

Plugin to enable Origen patterns to be run in a dynamic Verilog simulation
MIT License
1 stars 4 forks source link

Add pin monitors & correct a few bugs #25

Closed pderouen closed 5 years ago

pderouen commented 5 years ago

I'm not a verilog expert. I don't have high confidence in the pin contention monitor. But, in my limited testing it seems to work. I'm 50/50 on whether or not to leave it in. Thoughts?

coreyeng commented 5 years ago

Hi @pderouen, a few quick questions:

  1. What are the drive wave bugs this fixes? The only drive issues I've noticed with driving is trying to 'dont_care' a clock pin causes ncsim to actually crash. Other than that, I'm curious as to what you've found.

  2. Where are you defining pin.drive and pin.value? Chris and I have a need for that change as well, but we pass in the initial state from the command line and override the parameter. Are you adding onto the DUT during the sim build? (this line https://github.com/Origen-SDK/origen_sim/pull/25/commits/1744315c93a4f9470f443114b75d5056e1c36e79#diff-abe2b8ecb7f5f9767114755af6e97458R109)

Thanks!

pderouen commented 5 years ago

@coreyeng, the drive wave bug has to do with the disable drive wave function in c. There were 2 issues. The disable function was setting *wave to &compare_waves instead of drive_waves which led to segmentation faults for me. The second has to do with a pin switching directions from drive to compare. There needed to be a check for "is_driving_whole_cycle" before calling the disable function.

I didn't add any definition for pin.driving. That was already in the rtl.erb. I don't know where it gets set.

ginty commented 5 years ago

Thanks for this nice update @pderouen, really hope you can find a path to being an ongoing OrigenSim user!

The only thing I can really see 'wrong' with the contention check is that if you have a contention situation which extends for 10 cycles, then you will only get 1 error. This is in contrast to a 10 cycle compare error where you would get 10 errors.

To align the contention check, it would require a bigger change to introduce a cycle pulse into the test bench, so that you could say:

always @(posedge cycle or posedge contention) begin
  if (contention)...

Introducing visibility of the cycle like that would definitely be worth doing, but I don't think it has to be coupled to this change.

I am OK to approve in its current state and improve it later once we can trigger on a cycle if you are.

ginty commented 5 years ago

@coreyeng, I think these bugs were exposed when changing the same pin from drive to compare (and/or vice versa) back-back, i.e. without going via a don't care cycle in between.

pderouen commented 5 years ago

@ginty, yeah I agree. I played around with adding a falling edge message to let you know when the contention ended as a kludge. But, that didn't seem to work well.

coreyeng commented 5 years ago

Okay. Thanks! I think the back-to-back cycle is part of the issue I'm seeing with the clock (setting it to 'dont_care' then immediately trying to update it). Just curious - thanks for the details!

I think at the time of compiling the ERB in sim build, the pin drive and values will always be nil (since they haven't done anything yet). Its fine to leave it in for now for but I don't think it will have any effect (unless I'm misunderstanding something) - only bringing this up because this conflicts with something Chris and I need to add.

pderouen commented 5 years ago

ok, feel free to change the pin drive as needed. Like I said, I didn't add it. I added the pin name parameter for fail logging. I left pin drive unchanged.

ginty commented 5 years ago

@pderouen, I merged this but in final testing I noticed that the pin contention check causes the OrigenSim test pattern to fail. I don't know if it really should or not, but I didn't have time to find out and needed to push a release today, so I've disabled it for now by commenting it out. We can re-visit later to enable it by default, I guess for now you can manually uncomment it in the compiled origen.v if you want to use it. Thanks.

pderouen commented 5 years ago

I was on the fence about keeping that feature anyway. In the end that isn't the implementation we want.