Add support for optional axis_tstrb#60
Add support for optional axis_tstrb#60ollie-etl wants to merge 1 commit intoalexforencich:masterfrom
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.
| 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.
| 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
| 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 |
2 participants
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?