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
applet.interface.ps2_host: convert to nmigen #232
Conversation
2fb701c
to
81acf3e
Compare
self.in_fifo.w_en.eq(1), | ||
self.in_fifo.w_data.eq(error), | ||
] | ||
with m.If(self.in_fifo.w_rdy): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this check on w_rdy
before advancing state... it wasn't in before, but I figured we'd stand a chance of missing the error counter if it was left out.
If that's wrong, I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the very least it should be in a separate commit--there's nothing worse than a "no functional change" commit turning out to introduce a functional change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very true!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I went through the logic again and I think you aren't really fixing it--none of the other FIFO writes check readiness either. IIRC this choice is deliberate on my part (but is definitely sketchy); if you wanted to fix it, you could add a new FSM state (reached when the FIFO is full) that hangs the FSM while inhibiting any bus activity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none of the other FIFO writes check readiness either
You're quite right... whoops.
I've had a look, and I'm not sure I know enough about the protocol to cleanly implement something safer (e.g: does inhibiting the bus kill off the current request / response, or will it continue after releasing the bus).
I'll drop this patch for the moment, and it can be re-visited if it causes an issue later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than the comment.
Thanks! |
Addresses
interface.ps2_host
item from #150.