amaranth-lang / amaranth

A modern hardware definition language and toolchain based on Python
https://amaranth-lang.org/docs/amaranth/
BSD 2-Clause "Simplified" License
1.59k stars 174 forks source link

AttributeError when requesting an output pin with a clock constraint #646

Open jfng opened 3 years ago

jfng commented 3 years ago

Repro:

from nmigen import *
from nmigen_boards.arty_a7 import ArtyA7_35Platform

# Resource("eth_clk50", 0, Pins("G18", dir="o"),
#          Clock(50e6), Attrs(IOSTANDARD="LVCMOS33")),

platform = ArtyA7_35Platform()
eth_clk50 = platform.request("eth_clk50", 0)

m = Module()
m.d.comb += eth_clk50.o.eq(1)

platform.build(m, do_build=False)

Output:

Traceback (most recent call last):
  File "/tmp/repro.py", line 8, in <module>
    eth_clk50 = platform.request("eth_clk50", 0)
  File "/home/jf/src/nmigen/nmigen/build/res.py", line 164, in request
    value = resolve(resource,
  File "/home/jf/src/nmigen/nmigen/build/res.py", line 157, in resolve
    self.add_clock_constraint(pin.i, resource.clock.frequency)
  File "/home/jf/src/nmigen/nmigen/hdl/rec.py", line 146, in __getattr__
    return self[name]
  File "/home/jf/src/nmigen/nmigen/hdl/rec.py", line 157, in __getitem__
    raise AttributeError("{} does not have a field '{}'. Did you mean one of: {}?"
AttributeError: Record 'eth_clk50_0' does not have a field 'i'. Did you mean one of: o?

Currently, only two nmigen-boards platforms are affected (arty_a7 and nexys4ddr).

I'm not sure how to approach this:

  1. we could skip output clock constraints for now, and throw a warning when ResourceManager.request() encounters one. While not ideal, I think it's still better than preventing the design from building.
  2. we could shave this yak by implementing #498 and possibly #425.
whitequark commented 3 years ago

Why not add the constraint to the appropriate signal depending on the direction?

jfng commented 3 years ago

Why not add the constraint to the appropriate signal depending on the direction?

That is actually the workaround I used when I encountered this.

The issue is that on e.g. Xilinx platforms, this ends up as a create_clock constraint, which is inaccurate according to UG835 (p. 239): ug835_clock

I don't know whether although inaccurate, it would still be better than no constraint at all.

whitequark commented 3 years ago

I see. I'd need to take a closer look for this.