Add support for optional axis_tstrb #60
Conversation
| if self.tstrb is not None: | ||
| self.tstrb = self.tstrb[:n] + [self.tstrb[-1]]*(n-len(self.tstrb)) | ||
| else: | ||
| self.tstrb = self.tkeep |
There was a problem hiding this comment.
This is deliberate. If tstrb is not used, it should match tkeep. This implies 2 possible states:
- keep byte + byte is data
- may discard byte
It also means the invalid state of tkeep low + tstrb high is avoided.
| @@ -95,6 +100,11 @@ def normalize(self): | |||
| else: | |||
| self.tkeep = [1]*n | |||
|
|
|||
| if self.tstrb is not None: | |||
| self.tstrb = self.tstrb[:n] + [self.tstrb[-1]]*(n-len(self.tstrb)) | |||
There was a problem hiding this comment.
The open question is if tstrb should be checked here, to validate the validity of the tkeep, tstrb combination.
There was a problem hiding this comment.
Definitely should check for protocol violations when possible. The question is what to do: I'm thinking logging a warning or error would be appropriate, but an exception or assert I suppose could also be reasonable. Perhaps the best solution would be to have an option for enabling protocol assertions somewhere, which would generate exceptions instead of log messages.
There was a problem hiding this comment.
But, I suppose what we could do is roll with the log messages for now and then think about some config options for this sort of thing separately.
| @@ -306,6 +323,14 @@ def __init__(self, bus, clock, reset=None, reset_active_level=True, | |||
| self.byte_lanes = len(self.bus.tkeep) | |||
| if byte_size is not None or byte_lanes is not None: | |||
| raise ValueError("Cannot specify byte_size or byte_lanes if tkeep is connected") | |||
| if hasattr(self.bus, "tstrb") and (len(self.bus.tstrb) != self.byte_lanes): | |||
There was a problem hiding this comment.
If both tkeep and tstrb are provided, then they should be the same size.
| if hasattr(self.bus, "tstrb") and (len(self.bus.tstrb) != self.byte_lanes): | ||
| raise ValueError("tstrb must be the same width as tkeep") | ||
|
|
||
| elif hasattr(self.bus, "tstrb"): |
There was a problem hiding this comment.
I tstrb only provided, we can use that to get byte_lanes. Also reflected in the docs
| @@ -691,16 +724,18 @@ async def _run(self): | |||
| if tready_sample and tvalid_sample: | |||
| if not frame: | |||
| if self.byte_size == 8: | |||
| frame = AxiStreamFrame(bytearray(), [], [], [], []) | |||
| frame = AxiStreamFrame(bytearray(), [], [], [], [], []) | |||
There was a problem hiding this comment.
These caught me out, and may catch out users. the API change may justify a major version uplift
There was a problem hiding this comment.
I could mitigate this by adjusting the ordering of tstrb in the constructor arguments?
|
@alexforencich Your thoughts here would be appreciated, especially on the validity checking and API implications |
|
@alexforencich I think this is ready to go |
Support driving
tstrb, if present.Unimplmented, and unknown if desried, but when both
tstrbandtkeepare present, should monitoring be added for the invalid statetvalid & tstrb & !tkeep? If so, where?