Update stream pkg #513

bradleyharden · 2019-06-10T22:16:52Z

See the first commit message for the major changes.

I haven't added any tests yet. But to be fair, the stream package never had any tests to begin with.

bradleyharden · 2019-06-10T22:38:42Z

As I mentioned on Gitter, combining the stream_master and stream_slave packages broke the documentation. Personally, I think it makes more sense for the entire VCI to be defined in one package, but I can split it up again, if requested

bradleyharden · 2019-06-10T22:54:16Z

I think the Python 2.7 AppVeyor GHDL version is out of date. I had to get some GHDL bugs fixed for this package to work, so GHDL needs to be up to date.

kraigher · 2019-06-11T17:10:31Z

I do not mind merging the features into one packages as long as the documentation is updated accordingly. It would be easier to review your changes if the merge into a single package was first done without any changes and then the changes could be reviewed separately on top of the single packege. Now it just look like a new file was added and the old ones removed.

Separate pkg and body; Add overloaded functions

bradleyharden · 2019-06-11T18:59:42Z

I rewrote the history to better show the changes. Commit 2afaa7a is the most significant one. That commit introduces all major changes.

kraigher · 2019-06-12T17:48:15Z

I am ok with the changes, I like that there is encapsulation of push and pop and that a record is used to protect from protocol mismatch in VC when new fields are added in the future.

It just needs to pass Travis CI and the documentation needs to be updated.

bradleyharden · 2019-06-12T19:48:28Z

Yes, I plan to add tests. I just wanted to get some feedback before continuing. I'll mark it as a draft to make that clear.

bradleyharden · 2019-06-12T19:51:12Z

Well, never mind. It looks like you can't go from ready back to draft. At least not yet. I'll work on finishing this soon.

LarsAsplund · 2019-06-16T07:46:05Z

vunit/vhdl/verification_components/src/stream_pkg.vhd

+    constant expected_last : in    boolean := false;
+    constant msg           : in    string := "");
+
+  -- Overload sync functions


I think you should provide as_sync functions to support that design pattern as well.

What is the purpose of sync_handle_t? Right now it's just an alias for actor_t. Is it supposed to be a hook for future modifications? Right now, an actor function would be sufficient for sync_pkg as well, but I guess it's not quite as future proof. Is that the motivation?

Yes, that is the motivation. We try to make our public interface somewhat future-proof to avoid breaking backwards compatibility.

LarsAsplund · 2019-06-16T07:47:53Z

vunit/vhdl/verification_components/src/stream_pkg.vhd

+    constant stream : in    stream_slave_t;
+    constant delay  : in    delay_length);
+
+  -- Overload com funtions


I think you should provide helper function to get the actor form the handles such that all com functionality becomes available while not accessing private fields of the handles

Yes, I agree. After our conversation in Gitter, I realized that it's a much more general solution than overloading, even if it's not quite as clean when writing code.

LarsAsplund · 2019-06-16T07:58:56Z

vunit/vhdl/verification_components/src/stream_pkg.vhd

+  end record;
+
+  -- Create a new stream master object
+  impure function new_stream_master


I've started to think about best practices for VC/VCIs and I think they will include:

Allow the user to specify actor. This allows for greater control when integrating the VC

Allow the user to specify a logger and create a local checker based on that. Again it's about greater control for the integrator

Have a look at AXI stream for examples

Good idea about a logger. I'll add access to both in this interface.

LarsAsplund · 2019-06-16T08:00:07Z

vunit/vhdl/verification_components/src/stream_pkg-body.vhd

+      if msg_type = stream_pop_msg then
+        exit;
+      else
+        unexpected_msg_type(msg_type);


Use the local logger as the second parameter, see comment below

bradleyharden · 2019-07-09T14:01:29Z

I haven't forgotten about this. There are two issues.

One, I realized that I frequently (mis?)use stream_pkg as a way to push std_ulogic_vectors around without having to write or copy/paste a bunch of procedures. That actually covers a lot of use cases for me. However, the stream VCI ought to represent a generalized stream interface, not just be a collection of convenience procedures. I think my changes were more aimed at my own use cases. Maybe I can think of a new VC package that would cover my needs in that regard.

Two, I want to see the outcome of #520 before I update further.

umarcor · 2019-07-09T16:39:42Z

One, I realized that I frequently (mis?)use stream_pkg as a way to push std_ulogic_vectors around without having to write or copy/paste a bunch of procedures.

Just because of this reason, I've been thinking about FIFO VCs. I think this is a quite common use case and I'd love to have some kind of quick reference describing the alternatives provided by VUnit. E.g.:

integer_vector along with to_unsigned/to_signed and to_integer. If a FIFO/LIFO buffer is desired, the user needs to handle the indices explicitly,
queue along with to_unsigned/to_signed and to_integer. It behaves as a FIFO per se. I don't know if it can be used as a LIFO.
(hypothetical) FIFO VC. Conversions to/from std_logic_vector and integer are handled, so the user does not need to do it. It could include some 'metadata', such as a variable containing the used capacity, or the percentage of clock cycles that it has been full/empty. This can be useful to do statistical analysis of the required FIFO sizes in a given design, provided that the application/context do not allow to infer then analytically.
AXI Stream VC. It is quite similar to FIFO VC, plus multiple (optional) additional signals, a monitor and protocol checker.

Note that:

Both FIFO VC and AXI Stream VC are expected to be built on top of Stream VCI.
Stream VCI is built on top of queue.
queue is built on top of integer_vector.

@bradleyharden, @LarsAsplund, am I forgetting/misunderstanding anything here? Does the FIFO VC make sense to you as a sweet spot between users (mis?)using stream_pkg and 'being forced' to use AXI Stream?

That actually covers a lot of use cases for me. However, the stream VCI ought to represent a generalized stream interface, not just be a collection of convenience procedures. I think my changes were more aimed at my own use cases. Maybe I can think of a new VC package that would cover my needs in that regard.

If it sounds good to you, I'm all in with creating a FIFO VC.

Two, I want to see the outcome of #520 before I update further.

Agree.

eine mentioned this pull request Jun 11, 2019

Split verification components #480

Open

bradleyharden added 10 commits June 11, 2019 14:03

Merge stream VCI files

b60d076

Modify stream_pkg to better show history

0795b2b

Temp commit to transfer files. Bug in GHDL prevents debugging.

2afaa7a

Separate pkg and body; Add overloaded functions

Make minor changes to functions

d95eaa8

Fix issue with avalon_sink

ca7801a

Fix normalization problem with uart_master

a1cdbf2

Remove duplicate stream_pkg use statements

f015f89

Temporarily fix empty tb_stream_pkg

3cbfdbe

Update licenses

a3e5fcc

Update stream_pkg style

d0b87a3

bradleyharden force-pushed the update_stream_pkg branch from 71232ce to d0b87a3 Compare June 11, 2019 18:56

LarsAsplund reviewed Jun 16, 2019

View reviewed changes

umarcor mentioned this pull request Jul 9, 2019

New queue implementation #522

Open

Update stream pkg #513

Are you sure you want to change the base?

Update stream pkg #513

Uh oh!

Conversation

bradleyharden commented Jun 10, 2019

Uh oh!

bradleyharden commented Jun 10, 2019

Uh oh!

bradleyharden commented Jun 10, 2019

Uh oh!

kraigher commented Jun 11, 2019

Uh oh!

bradleyharden commented Jun 11, 2019

Uh oh!

kraigher commented Jun 12, 2019

Uh oh!

bradleyharden commented Jun 12, 2019

Uh oh!

bradleyharden commented Jun 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradleyharden commented Jul 9, 2019

Uh oh!

umarcor commented Jul 9, 2019

Uh oh!

Uh oh!