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

feature: add new optional reset signal to StreamWidthAdapter and StreamFragmentWidthAdapter #1008

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

jonnykl
Copy link
Contributor

@jonnykl jonnykl commented Jan 8, 2023

Context, Motivation & Description

I've added a new optional reset signal to the StreamWidthAdapter (and also StreamFragmentWidthAdapter). This signal resets the internal counters to 0. Output valid/ready signals are combinatorially set to False when the reset input is asserted. Therefore the reset signal effectivly discards the buffered input. It is allowed to pass null for the reset signal. In that case the adapters behave exactly as before.

Impact on code generation

  • additional reset signal for the counter
  • additional signal in the output valid/ready signals

Checklist

  • Unit tests were added
  • API changes are or will be documented:

@Readon
Copy link
Collaborator

Readon commented Jan 8, 2023

Have you investigated the reset area described here? I think this can help with the reset related logic.

@jonnykl
Copy link
Contributor Author

jonnykl commented Jan 8, 2023

The reset area isn't useful for this purpose. The reset area would also add the reset signal to the internal buffer register which is not needed and therefore makes the design unnecessarily more complex. More important, the reset area does not affect the combinatorial valid and ready signals.

@kleinai
Copy link
Collaborator

kleinai commented Jan 8, 2023

I see how the reset area wouldn't be necessary here. Also looks like your apply method changes failed to compile.

@Readon
Copy link
Collaborator

Readon commented Jan 8, 2023

The reset area isn't useful for this purpose. The reset area would also add the reset signal to the internal buffer register which is not needed and therefore makes the design unnecessarily more complex. More important, the reset area does not affect the combinatorial valid and ready signals.

I think the combination logic you want to apply to those Streams is more like a call to haltWhen() or a reduced version.
On the other hand, I think the reset itself is a definition in Sequential logic, not a combinational one.

The design principle of the components in the library is to adopt the combinational logic in some methods or areas. While user using them, they can easily composite them through facilities with or without registers to control the timing.
So I personally prefer to use a method standalone like "haltWhen" to do combinational work.

As a reference, there are no components (to my knowledge) mixed reset logic into the functional logic.

@jonnykl
Copy link
Contributor Author

jonnykl commented Jan 8, 2023

Also looks like your apply method changes failed to compile.

Yes, I already fixed that.

I think the combination logic you want to apply to those Streams is more like a call to haltWhen() or a reduced version.
On the other hand, I think the reset itself is a definition in Sequential logic, not a combinational one.

As of my unterstanding, haltWhen() implies that the stream is just interrupted without loosing data. This is not the case for my proposed feature and that's why I think it is useful.

The design principle of the components in the library is to adopt the combinational logic in some methods or areas. While user using them, they can easily composite them through facilities with or without registers to control the timing. So I personally prefer to use a method standalone like "haltWhen" to do combinational work.

As a reference, there are no components (to my knowledge) mixed reset logic into the functional logic.

How do you think such a "reset" signal could be implemented in a better way? Putting the adapter into a reset area and adding haltWhen(reset) to the input and output streams should achieve the same effect (altough this has the downside that the reset area also affects the internal buffer register which is not needed (as already mentioned)). Do you think the user should do that if such a behavior is required?

@Dolu1990
Copy link
Member

Dolu1990 commented Jan 9, 2023

instead of "reset", maybe it should be more like "flush" ?

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.

4 participants