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

Avalon stream update #666

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Avalon stream update #666

wants to merge 6 commits into from

Conversation

slaweksiluk
Copy link
Contributor

also deleted gtkw files

@@ -123,6 +129,29 @@ package body avalon_stream_pkg is
return source.p_data_length;
end;

function calc_empty_length(data_width : positive) return natural is
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall, the PR looks good. However, I'm in doubt with this function. Isn't this equivalent to log2? Otherwise, should it be placed in a different location, to be reused by other parts of VUnit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is equivalent. However as I remember I had unexpected results due to rounding issues when used log2 and ceil/floor from ieee math library. I might applied them in wrong order or made other mistake, so will be grateful for some example.

Placing function for calculating vector width on in vunit lib is good idea overall. Its often used in test benches.

Copy link
Collaborator

@eine eine Jul 8, 2020

Choose a reason for hiding this comment

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

Actually, I faced those issues myself too... I "fixed" them for n-1 or for 0, but I always have issues with some of them. Apart from that, I've seen variants of your function multiple times, but I cannot remember if it exists in VUnit's codebase already.

Let me ask @Paebbels, what is his suggested approach: do you use log2 + ceil/floor or a custom function? is there anything in the latest IEEE libs?

Copy link

@Paebbels Paebbels Jul 8, 2020

Choose a reason for hiding this comment

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

The PoC Library provides:

  • log2ceil - 2s logarithm that rounds up
  • log2ceilnz - 2s logarithm that rounds up and is not zero (nz)

The latter function is sometimes called to_bits.


There is no standard implementation in IEEE libs. This could be implemented in CoreLib.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @Paebbels!

@slaweksiluk, I'm thinking that we might move calc_empty_length to a new package in the core vunit/vunit/vhdl/core/src/utils_pkg.vhd. If your implementation is equivalent to either log2ceil or log2ceilnz from PoC, I think we should go with that (not adding PoC as a ref, but using the same name).

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is also a similar function here. I'm for a "utility package" but I would avoid using that word since that is used to denote the set of optional VUnit packages, i.e. the packages that helps you write testbenches but aren't essential for running VUnit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@LarsAsplund what about vunit/vunit/vhdl/helpers/src/helpers_pkg.vhd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How to tell vunit to compile this new package? 215c64d

Copy link
Collaborator

Choose a reason for hiding this comment

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

@eine
Copy link
Collaborator

eine commented Jul 7, 2020

@LarsAsplund is it ok to merge this before #520?

@slaweksiluk
Copy link
Contributor Author

please review. Mind the commit f14fcae where I modified ready signal stimulation - to be truly random and not dependent on calling VC API functions in user main tb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants