Peek functionality

[dalex78]

Add a peek functionality to the queue_pkg: returns the element at the front the queue. It does not delete the element in the queue (contrary to the already existing pop command).
The peek functionality has alos been added to the com package and some of the verification components.

Added a small description of the peek functionality in the documentation of the com library when using messages.

My first Pull Request (ever), don't hesitate to give feedback.

[dalex78]

Corresponding issue: #812

[LarsAsplund]

I took a first quick look at the user interfaces for peek and have a question about the offset parameter. Is offset the distance in the encoded data, i.e. number of bytes, or is it the distance in elements (0 is the first element, 1 the second and so on)?

[dalex78]

It is the same idea than the index parameter in the codec package (therefore, it is the distance in the encoded data, i.e. number of bytes). I only learned about the index parameter today. offset should be renamed as index.

[dalex78]

@LarsAsplund, I am currently working on #818. If the issue is accepted, I plan to do the same thing with the queue_pkg: better code, cleaner with more documentation.
I would suggest, you give your feedback, what you like and don't like with the implementation of the peek functionality and with what is being done at #818. You refuse this PR and then in the following weeks, I can present something better (I have some time for the next two weeks).

[LarsAsplund]

I don't think you need the high-level peek procedures with offset/index since the user will never peek a number of bytes into the queue. A high-level peek function needs lower-level peek functions for a fix or a variable length string starting at the head of the queue. These low-level functions need to peek characters with offset/index but only at this level there is a need to handle offset/index. Once you have peeked the string of correct length the value can be calculated using the decode function.

However, we can extend the scope and provide peek functions that allow you to peek the n:th element in the queue. I think this is a feature that can be done in a second iteration

I also see a use case where you don't know the type at the head of the queue so you start peeking the type and then based on the response you pop or peek an element of that type.

[dalex78]

I agree that the user will never use the offset/index functionality provided by the procedure. It's not intended for them but for other VUnit developer (or advanced users). Each predefined type has a function and a procedure to peek. For example, an integer can be peek using:

• impure function peek(queue : queue_t) return integer => What the user should use (high level function)
• procedure peek(queue : queue_t; variable value : out integer; variable offset : inout natural) => What the VUnit developer could use if needed (low level function)

The procedure is needed to build peek functions for more complex types. For example, it is possible to peek a message (msg_t) from a queue using: impure function peek(queue : queue_t) return msg_t which is build from its sibling procedure:

  procedure peek(
    queue : queue_t;
    variable ret_val : out msg_t;
    variable offset : inout natural
  ) is
    variable value : integer;
  begin
    peek(queue, ret_val.id, offset);
    peek(queue, value, offset);
    ret_val.msg_type := (p_code => value);
    peek(queue, value, offset);
    ret_val.status := com_status_t'val(value);
    peek(queue, ret_val.sender.id, offset);
    peek(queue, ret_val.receiver.id, offset);
    peek(queue, ret_val.request_id, offset);
    peek_queue_ref(queue, ret_val.data, offset);
  end procedure;

As you can see, the peek procedures are needed to update the value of offset automatically and provide abstraction.
The queue_pkg packages are build on the same idea with the procedures decode and the index variables.

However, we can extend the scope and provide peek functions that allow you to peek the n:th element in the queue. I think this is a feature that can be done in a second iteration.

A user can peek several element (not byte) as shown in the procedure above. But, I think that this is a feature more likely to be used by VUnit developers.
If you have a simpler idea for a casual user, I can try to implement it.

I also see a use case where you don't know the type at the head of the queue so you start peeking the type and then based on the response you pop or peek an element of that type.

I can work on that. (Note that there is a function peek_type but I am not sure it enables what you want, I need to double check that)

[dalex78]

If you find that my message did not answer your questions or remarks, then I probably did not understood your point. Do you find the changes I did are too important and/or too complex for the functionality it provides ?

[LarsAsplund]

I agree that the user will never use the offset/index functionality provided by the procedure. It's not intended for them but for other VUnit developer (or advanced users).

That's a valid use case. The only thing I would ask for is to move these to a separate package. I think the same distinction is done for codecs where the codec package is separate from the codec builder package.

A user can peek several element (not byte) as shown in the procedure above. But, I think that this is a feature more likely to be used by VUnit developers.
If you have a simpler idea for a casual user, I can try to implement it.

I was thinking a peek that allow the user to specify what element index to peek. However, leave that for now to avoid making huge commits/PRs. I think such a feature may drive the need for another internal representation of queues so that's why I think we better release the other stuff first.

I can work on that. (Note that there is a function peek_type but I am not sure it enables what you want, I need to double check that)

That is what I was looking for. I missed it. Just make it public. Now it is an unused function in the package body.

In general I think it would also be good if we can break up this PR in smaller pieces. The normal user queue features feels complete and could be released first. I need to look further into the codec updates.

[dalex78]

That's a valid use case. The only thing I would ask for is to move these to a separate package. I think the same distinction is done for codecs where the codec package is separate from the codec builder package.

Indeed, it is done this way in the codec package. However, (I intend to raise an issue about that), I do not think it is a good thing to do.

From the user point of view, when looking for information directly into the code, he has to understand why the decode_integer function and decode_integer procedure declaration are specified in different packages (their are no comment or documentation about that at the moment).

From the VUnit developer (or advanced user) point of view, it is not desirable either because the implementation of the encode_integer and decode_integer are far from one another: harder to enter the code and to maintain.

When I personally wanted to understand how the codec package and queue package worked (a couple of month ago), I find it really hard to navigate between the files. I would prefer to have one file having all the functions/procedures with comments indicating which are intended for the casual user and for the VUnit developer.
As an example, see the rework of the declaration package of the codec functionality: Re-work of the codec declaration package

What do you think ? (for the queue package I mean, I just took the codec package as an illustration to show how it could be done but I am also interested in any remarks you have about the codec package).

In general I think it would also be good if we can break up this PR in smaller pieces. The normal user queue features feels complete and could be released first. I need to look further into the codec updates.

Sure, no problem, when we are done discussing, I'll cancel this PR and do that. So, do you want a PR by package ?

1. For the queue package
2. Then for the com package

Or something even smaller ?

[dalex78]

Hum, after sometime, I think you are right. I will move the peek procedures to a different package the same way it is done for the codec package.

[LarsAsplund]

Yes, documentation is lacking here and it would be great if you could improve. However, if the documentation of a subprogram just restates the obvious I rather don't have it at all. I think standard pop, push, and peek fall into that category but the peek procedure does not.

I'm thinking a first separate PR for the queue package is a good start. That is the most obvious extension and a good first iteration to get released. Try to redefine this PR or reference it from the new PR such that the discussion history isn't lost.

[dalex78]

I agree.

Thank you for the various feedback. Here is what I will do:

1. Make a PR for the issue Lifting of the codec packages #818 (I am almost done),
2. Make an issue similar to Lifting of the codec packages #818 for queue_pkg (no more hard coded values, use the changes from issue Lifting of the codec packages #818, improve code comment, etc),
3. Make a new PR for the peek functionality for queue package taking account all that have been discussed previously
4. Make a PR for the the peek functionality for com package taking account all that have been discussed previously.