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.

Sign up for GitHub

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

Jump to bottom

applet.video.vga_output: convert to nmigen #233

Merged
merged 1 commit into from Dec 29, 2020
Merged

applet.video.vga_output: convert to nmigen #233

merged 1 commit into from Dec 29, 2020

Conversation

attie
Copy link
Member

@attie attie commented Nov 12, 2020 •
edited

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.

  1. Previously the clock had a constraint asserted on it in VGAOutputApplet.build()... this seems to be no longer possible (as VGAOutputSubtarget.elaborate() has not yet been called). here
  2. Previously the pixel data was generated in VGAOutputApplet.build(), but again, this is no longer possible. here

I very briefly tried placing them into VGAOutputApplet.run(), but reverted to this state to get something that worked.

@attie
Copy link
Member Author

attie commented Dec 29, 2020

@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.

@attie attie mentioned this pull request Dec 29, 2020
Closed
30 tasks
@whitequark
Copy link
Member

the one query I have is related to the pixel generation

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:

  1. Leave the test pattern completely hardcoded, and avoid having a public interface in first place.
  2. Have a public interface that would be better than a bunch of counters, and implement the test pattern in a subclass.

@attie attie force-pushed the vga-nmigen branch 2 times, most recently from 62fc6b1 to 8532a4f Compare December 29, 2020 10:36
@attie
Copy link
Member Author

attie commented Dec 29, 2020

Leave the test pattern completely hardcoded, and avoid having a public interface in first place.

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

# TODO: re-assert the clock constraint?
m.domains.pix = ClockDomain(reset_less=True)
m.submodules += PLL(f_in=platform.default_clk_frequency, f_out=self.pix_clk_freq, odomain="pix")
#platform.add_clock_constraint(m.domains.pix.clk, self.pix_clk_freq)

@whitequark
Copy link
Member

What exactly makes it impossible? I.e. what's the error?

@attie
Copy link
Member Author

attie commented Dec 29, 2020

Ah, sorry. The error is below... which (to my understanding) contradicts the m.domains.pix = ... line just above...?

$ glasgow run video-vga-output -V 3.3
Traceback (most recent call last):
  File "/home/attie/proj_local/glasgow/venv/bin/glasgow", line 11, in <module>
    load_entry_point('glasgow', 'console_scripts', 'glasgow')()
  File "/home/attie/proj_local/glasgow/glasgow/software/glasgow/cli.py", line 857, in main
    exit(loop.run_until_complete(_main()))
  File "/home/attie/.bin/python3.8.2/lib/python3.8/asyncio/base_events.py", line 616, in run_until_complete
    return future.result()
  File "/home/attie/proj_local/glasgow/glasgow/software/glasgow/cli.py", line 519, in _main
    plan = target.build_plan()
  File "/home/attie/proj_local/glasgow/glasgow/software/glasgow/target/hardware.py", line 93, in build_plan
    return GlasgowBuildPlan(self.platform.prepare(self, **overrides))
  File "/home/attie/proj_local/glasgow/nmigen/nmigen/build/plat.py", line 130, in prepare
    fragment = Fragment.get(elaboratable, self)
  File "/home/attie/proj_local/glasgow/nmigen/nmigen/hdl/ir.py", line 39, in get
    obj = obj.elaborate(platform)
  File "/home/attie/proj_local/glasgow/nmigen/nmigen/hdl/dsl.py", line 538, in elaborate
    fragment.add_subfragment(Fragment.get(self._named_submodules[name], platform), name)
  File "/home/attie/proj_local/glasgow/nmigen/nmigen/hdl/ir.py", line 39, in get
    obj = obj.elaborate(platform)
  File "/home/attie/proj_local/glasgow/nmigen/nmigen/hdl/dsl.py", line 540, in elaborate
    fragment.add_subfragment(Fragment.get(submodule, platform), None)
  File "/home/attie/proj_local/glasgow/nmigen/nmigen/hdl/ir.py", line 39, in get
    obj = obj.elaborate(platform)
  File "/home/attie/proj_local/glasgow/nmigen/nmigen/hdl/dsl.py", line 540, in elaborate
    fragment.add_subfragment(Fragment.get(submodule, platform), None)
  File "/home/attie/proj_local/glasgow/nmigen/nmigen/hdl/ir.py", line 39, in get
    obj = obj.elaborate(platform)
  File "/home/attie/proj_local/glasgow/nmigen/nmigen/hdl/xfrm.py", line 332, in elaborate
    fragment = Fragment.get(self._elaboratable_, platform)
  File "/home/attie/proj_local/glasgow/nmigen/nmigen/hdl/ir.py", line 39, in get
    obj = obj.elaborate(platform)
  File "/home/attie/proj_local/glasgow/glasgow/software/glasgow/applet/video/vga_output/__init__.py", line 72, in elaborate
    platform.add_clock_constraint(m.domains.pix.clk, self.pix_clk_freq)
AttributeError: '_ModuleBuilderDomainSet' object has no attribute 'pix'

@whitequark
Copy link
Member

Oh ok so that's an nMigen quirk that has nothing to do with elaborate order. Try this:

        m.domains.pix = cd_pix = ClockDomain(reset_less=True)
        platform.add_clock_constraint(cd_pix.clk, self.pix_clk_freq)

@attie
Copy link
Member Author

attie commented Dec 29, 2020

Perfect, thanks!

... and I've confirmed still working too.

@attie attie marked this pull request as ready for review December 29, 2020 11:16
@whitequark whitequark merged commit 1e1696e into GlasgowEmbedded:master Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whitequark whitequark whitequark approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@attie @whitequark @attie-argentum