Skip to content

Add StreamUtils #5

Open
BIGWJZ wants to merge 4 commits intomainfrom
streamUtils
Open

Add StreamUtils #5
BIGWJZ wants to merge 4 commits intomainfrom
streamUtils

Conversation

@BIGWJZ
Copy link
Collaborator

@BIGWJZ BIGWJZ commented Jul 15, 2024

StreamUtils includes 2 modules: mkStreamConcat and mkStreamSplit

isStreamAEnd <= False;
end
// the last StreamA + the first StreamB
else if (streamA.isLast && prepareFifoB.notEmpty) begin
Copy link
Collaborator

Choose a reason for hiding this comment

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

infact, the condition in this else if statement is not necessary. the bsv has implicit conditions, so if the prepareFifoB is empty, this else if branch will not be executed.

but you write the condition here is good for code readability, you can keep the condition here, but this lead to another problem: this if statement has other possiblity that is not covered. you should add a else branch, and add some code there, maybe only some comment like "in all other condition, wait for stream B to be ready"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the condition here is not necessary but good for understanding. I have added a else branch where just waiting for streamB in this clock.

hasLastRemainReg <= streamB.isLast;
remainStreamReg <= remainStream;
remainBytePtrReg <= remainBytePtr;
isStreamAEnd <= !streamB.isLast;
Copy link
Collaborator

Choose a reason for hiding this comment

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

isStreamAEnd is misleading. isStreamAEnd looks like streamA.isLast, but infact it's not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isStreamAEnd means streamA.isLast, but it need to be reset when the whole concat ends, i.e. streamB.isLast asserts. The codes here are misleading and I have modified for better understanding.

test StreamShift
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.

2 participants