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

add possibility to configure random stalls for axi_stream #557

Merged
merged 25 commits into from
Feb 10, 2020
Merged

add possibility to configure random stalls for axi_stream #557

merged 25 commits into from
Feb 10, 2020

Conversation

dstadelm
Copy link
Contributor

@dstadelm dstadelm commented Oct 4, 2019

At creation of axi_stream_master/slave one can provide a configuration which tells
how often a stall should occur, and if it occurs what the
minimal/maximal stall length of such a stall shall be.

@kraigher
Copy link
Collaborator

I am ok with this. But there are merge conflicts.

@LarsAsplund @1138-4eb are you ok with this?

Copy link
Collaborator

@eine eine left a comment

Choose a reason for hiding this comment

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

@dstadelm, can you please resolve the conflicts?

Overall, LGTM. I added a few comments. Some are minor suggestions, some are thoughts which don't need to be addressed.

@dstadelm
Copy link
Contributor Author

I don't understand why the tests are failing, they run without failing on my local machine...

@dstadelm
Copy link
Contributor Author

Ok, somehow the last changes got stuck along the way...

@eine
Copy link
Collaborator

eine commented Nov 18, 2019

I don't understand why the tests are failing, they run without failing on my local machine...

Which simulator are you using? Tests run with GHDL in a docker container. You can run them locally with:

docker run --rm \
  -v /$(pwd)://src \
  -w //src \
  vunit/dev:llvm \
  tox -e py38-vcomponents-ghdl

constant slave_stream : stream_slave_t := as_stream(slave_axi_stream);
constant slave_sync : sync_handle_t := as_sync(slave_axi_stream);

constant monitor : axi_stream_monitor_t := new_axi_stream_monitor(
data_length => 8, id_length => g_id_length, dest_length => g_dest_length, user_length => g_user_length,
logger => get_logger("monitor"), actor => new_actor("monitor"),
data_length => 8, id_length => g_id_length, dest_length => g_dest_length, user_length => g_user_length,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The format here is different from slave_axi_stream or master_axi_stream above. I'd personally put each parameter in a line. However, since a different style is already being used, I think that it should be preserved. Specially, the additional spaces in lines 39, 48, 56, 62, 124, 164, etc. and in lines 53, 54, 354, etc. Nevertheless, I think that the alignment fixes in 126-129, 427-434, 457-464, etc. are good.

Alternatively, if you did this beautification automatically with some tool, we'd be glad to know which did you use. We are currently using black for Python, but we don't know of any open source tool that allows to do the same with VHDL.

Copy link
Contributor Author

@dstadelm dstadelm Nov 19, 2019

Choose a reason for hiding this comment

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

I'm not quite sure how to interpret your comment, should I revert beautification?

This is the emacs beautification. I'm actually using neovim but the emacs beautification can be called in batch mode.

IMHO emacs has the only 'correct' indentation for vhdl code.
emacs --no-site-file -batch <FILE> -f vhdl-beautify-buffer -f save-buffer

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not quite sure how to interpret your comment, should I revert beautification?

Yes. It's ok to fix misaligned groups of parameters and other minor issues around your changes; but some of the style changes introduce inconsistencies that should be handled in a separate PR. For example, this specific line feels awkward.

Thanks for pointing to emacs. Although it seems that no new features are to be added (https://guest.iis.ee.ethz.ch/~zimmi/emacs/vhdl-mode.html#maintenance), VHDL 2008 is said to be supported. (Un)fortunately, the style can be customized: https://www.gnu.org/software/emacs/manual/html_mono/vhdl-mode.html#Customizing-Indentation. On the one hand, did you use any specific style configuration? On the other hand, this discussion might deserve a separate issue before a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I did not use any specific style configuration.

Copy link
Collaborator

@eine eine left a comment

Choose a reason for hiding this comment

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

Some minor fixes, but LGTM. @kraigher, @LarsAsplund, can you have a look at the two points above?

vunit/vhdl/verification_components/src/axi_stream_pkg.vhd Outdated Show resolved Hide resolved
vunit/vhdl/verification_components/test/tb_axi_stream.vhd Outdated Show resolved Hide resolved
@eine
Copy link
Collaborator

eine commented Dec 13, 2019

@dstadelm, I rebased on top of master and I force-pushed the changes to your branch/fork, so that the conflicts are gone and tests are properly executed.

@dstadelm
Copy link
Contributor Author

Please review, thx

@eine
Copy link
Collaborator

eine commented Dec 19, 2019

@dstadelm, it seems that, instead of making your latest changes on top of the rebased branch commented in #557 (comment) (i.e. 6f769f9), you changed your local branch and force-pushed on top. Please, either rebase it yourself or apply latest fixes on top of 6f769f9 .

@dstadelm
Copy link
Contributor Author

I had to merge, rebase f***ed up good time. I've never experienced this before, but I just couldn't figure out what went wrong.

@eine
Copy link
Collaborator

eine commented Jan 3, 2020

@dstadelm, what's the status of this PR? I saw that you reacted to some of Lars' comments, but it seems that you didn't push any commit after that. Did you forget to push it or have you been busy?

Just asking to know when to help you with rebasing.

@dstadelm
Copy link
Contributor Author

@eine We had Christmas vacation (family time:) and I had a release for our customer. I will work on it now...

@dstadelm
Copy link
Contributor Author

@eine
Do you have the capacity/resource to perform the review?

Copy link
Collaborator

@eine eine left a comment

Choose a reason for hiding this comment

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

Just a typo and a few style issues. Overall, LGTM.

@eine
Copy link
Collaborator

eine commented Feb 7, 2020

The CI failures seem to be due to some uppercasing I pushed some days ago. Easy to fix. At the same time, docs are failing due to an unrelated issue. I'll try to take care of it before merging this.

@dstadelm
Copy link
Contributor Author

@kraigher and @eine could you please help me? The build fails due to some weird error in docs. I don't understand what to do...

@eine
Copy link
Collaborator

eine commented Feb 10, 2020

@kraigher and @eine could you please help me? The build fails due to some weird error in docs. I don't understand what to do...

Ignore them. It is unrelated to your changes, and I'm taking care of it: https://github.com/eine/vunit/commits/rm-ablog

You should only worry about vcomponents tests. tb_axi_stream in vunit/vhdl/verification_components/run.py needs to be uppercased: https://github.com/VUnit/vunit/blob/master/vunit/vhdl/verification_components/run.py#L97

@eine
Copy link
Collaborator

eine commented Feb 10, 2020

@dstadelm, you can now rebase, and the docs should pass.

David Stadelmann added 25 commits February 10, 2020 16:50
At creation of master/slave one can provide a configuration which tells
how often a stall should occure, and if it occures what the
minimal/maximal stall length of such a stall shall be.
based on pull request comment of 1138-4EB
this makes it more general and percentage numbers can be varied through
the generics without having to manual change the vhdl code.
@eine eine changed the title Added possibility to configure random stalls for axi_stream add possibility to configure random stalls for axi_stream Feb 10, 2020
@eine eine merged commit a7f2bbe into VUnit:master Feb 10, 2020
@eine
Copy link
Collaborator

eine commented Feb 10, 2020

Thanks @dstadelm! Nice work! 🎉

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