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

Allowing to disable the sending of end of burst packet #34

Closed
wants to merge 3 commits into from
Closed

Allowing to disable the sending of end of burst packet #34

wants to merge 3 commits into from

Conversation

rijulg
Copy link
Contributor

@rijulg rijulg commented Mar 7, 2019

Motivation

For certain use cases of RFNoC blocks the data expected by the block is only in terms of streams. This is the case by default while using AXI Wrappers' simple mode when the user does not intend to interact with the headers of the packets and is concerned only with the payload of the packets.

In such applications when the user is not intending to use the headers an additional packet that is sent as the end of burst produces unexpected results. The situation is worsened upon implementation of state machines that depend on receiving certain number of packets.

The problem can be resolved in hardware block by consuming/ignoring the end of burst packet, but that still results in performing operations that are not relevant to intended designs.

Proposed solution

  • Instead of always sending the end of burst packet, a parameter is used to determine whether or not the end of burst packet should be sent or not.
  • This parameter has been assigned a default value that performs the existing functionality of sending the burst packet, while it can be now configured to not send that packet if the user so wishes.

Testing

Copy link
Contributor

@mbr0wn mbr0wn left a comment

Choose a reason for hiding this comment

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

I think this is a sensible changeset. Thanks for putting it together! What do you think about renaming the argument?

@@ -74,7 +74,8 @@ namespace gr {
const device3::sptr &dev,
const std::string &block_id,
const ::uhd::stream_args_t &tx_stream_args,
const ::uhd::stream_args_t &rx_stream_args
const ::uhd::stream_args_t &rx_stream_args,
const bool end_of_burst_enabled = true
Copy link
Contributor

@mbr0wn mbr0wn Mar 7, 2019

Choose a reason for hiding this comment

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

I think this name is misleading. How about enable_eob_on_stop?

Reason being, this made me take another look at this code, and we should be honoring EOBs in the TX functions, but we don't. Let's assume that we did, then this argument would imply that all EOBs are disabled, but we only want to disable EOBs on stop().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @mbr0wn ,
That makes sense, I honestly did not think of things from that perspective. I have updated the name accordingly.

@mbr0wn
Copy link
Contributor

mbr0wn commented Mar 8, 2019

Thank you so much! Manually merged.

@mbr0wn mbr0wn closed this Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants