-
Notifications
You must be signed in to change notification settings - Fork 189
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.video.vga_output: convert to nmigen #233
Conversation
@whitequark - if you have a chance to review this, the one query I have is related to the pixel generation (da8f559). I've used this approach again in #246, and can squash if you're happy. |
Originally this applet was subclassed by another one that implemented a sort of a terminal. However I don't think the interface you're exposing here is a good one to use in subclasses, if any arise in the future (not your fault!). I see two options here:
|
62fc6b1
to
8532a4f
Compare
I've gone for this, we can re-visit if necessary. Apologies, on re-reading there was one other query... how to handle this: glasgow/software/glasgow/applet/video/vga_output/__init__.py Lines 69 to 72 in 8532a4f
|
What exactly makes it impossible? I.e. what's the error? |
Ah, sorry. The error is below... which (to my understanding) contradicts the
|
Oh ok so that's an nMigen quirk that has nothing to do with elaborate order. Try this:
|
Perfect, thanks! ... and I've confirmed still working too. |
Addresses
video.vga_output
item from #150.This PR is WIP... there are a couple of things I'd like feedback on how best to implement.
VGAOutputApplet.build()
... this seems to be no longer possible (asVGAOutputSubtarget.elaborate()
has not yet been called). hereVGAOutputApplet.build()
, but again, this is no longer possible. hereI very briefly tried placing them into
VGAOutputApplet.run()
, but reverted to this state to get something that worked.