Signal: use reset.value if that field exists.

[nmigen-issue-migration]

Issue by Fatsie Sunday Jan 05, 2020 at 19:17 GMT Originally opened as https://github.com/m-labs/nmigen/pull/294

---

This will allow a Const or an Enum to be passed as reset value when calling Signal().

---

Fatsie included the following code: https://github.com/m-labs/nmigen/pull/294/commits

[nmigen-issue-migration]

Comment by codecov[bot] Sunday Jan 05, 2020 at 19:29 GMT

---

Codecov Report

Merging #294 into master will decrease coverage by 0.02%. The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #294      +/-   ##
=========================================
- Coverage   82.13%   82.1%   -0.03%     
=========================================
  Files          34      34              
  Lines        5647    5649       +2     
  Branches     1160    1161       +1     
=========================================
  Hits         4638    4638              
- Misses        864     865       +1     
- Partials      145     146       +1

Impacted Files	Coverage Δ
nmigen/hdl/ast.py	86.99% <0%> (-0.2%)	:arrow_down:
nmigen/tracer.py	94.59% <0%> (ø)	:arrow_up:
nmigen/hdl/ir.py	94.43% <0%> (ø)	:arrow_up:
nmigen/back/pysim.py	92.32% <0%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 476ce15...4640f87. Read the comment docs.

[nmigen-issue-migration]

Comment by whitequark Monday Jan 06, 2020 at 08:12 GMT

---

I think this is not the right approach. There are several issues:

1. Sure, Python uses duck typing, but it doesn't mean you can use arbitrary properties of objects without considering what interface they implement. That there are both Const.value and Enum.value is a complete coincidence: they were never intended to implement the same interface. This is in contrast with something like a "file-like object", where there's a reasonably well-defined interface anyone can comply with.
2. I'm not actually sure if handling Const as a reset value is the right thing to do. Right now it is clear what values are valid as reset values: literals. If we allow ASTs there, then it raises a question of whether we should allow constant expressions or even arbitrary expressions. Should Const(123, 8)[:4] be allowed as a reset value? Should other_comb_sig + 10 be?

That said, allowing integral Enum values is obviously the right choice here. Feel free to send another PR implementing that or opening an issue and I'll implement it.