search

dan-fritchman / Hdl21

Hardware Description Library
BSD 3-Clause "New" or "Revised" License
69 stars 16 forks source link

Making `MosParams` more consistent #164

Closed kcaisley closed 1 year ago

kcaisley commented 1 year ago

I'm working on building my own Hdl21 package for an onsite PDK, and noticed some places I could add to the existing open source examples; mostly comments to help others understand the expressions for transistor instance parameters.

Also, some small spellcheck and formatting fixes.

codecov[bot] commented 1 year ago

Codecov Report

Merging #164 (5434614) into main (cf31f94) will increase coverage by 0.00%. The diff coverage is 93.54%.

@@           Coverage Diff           @@
##             main     #164   +/-   ##
=======================================
  Coverage   87.22%   87.23%           
=======================================
  Files         109      109           
  Lines        9678     9680    +2     
=======================================
+ Hits         8442     8444    +2     
  Misses       1236     1236
Files Changed Coverage Δ
hdl21/instance.py 88.81% <ø> (ø)
hdl21/instantiable.py 80.00% <ø> (ø)
hdl21/params.py 93.33% <0.00%> (ø)
hdl21/pdk/corner.py 88.88% <ø> (ø)
hdl21/pdk/installation.py 100.00% <ø> (ø)
hdl21/pdk/pdk.py 52.05% <ø> (ø)
hdl21/scalar.py 85.18% <ø> (ø)
hdl21/sim/data.py 91.63% <ø> (ø)
hdl21/slice.py 90.54% <ø> (ø)
hdl21/tests/test_doc_examples.py 85.36% <ø> (ø)
... and 12 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

kcaisley commented 1 year ago

I'm not sure if this is really an issue, but having all these small changes broken across 10 commits seems a bit of a mess. Moving forward, would it be better for me to use something like git squash to collate similar changes, before opening a pull request?

dan-fritchman commented 1 year ago

The PR is a great venue to view them in aggregate. No worries there.

dan-fritchman commented 1 year ago

Re: the as vs As parameter name.
I expect this will work, for this case and for now.
But I do think it'll be a problem in general. Filed #171.

kcaisley commented 1 year ago

Converting this to a draft, as I think I'll be adding some more small changes this week.

dan-fritchman commented 1 year ago

This ready to roll?
Probably needs a merge of main to pick up some VLSIR changes.

dan-fritchman commented 1 year ago

I think we got the wrong idea here about replacing the m Mos parameter name. 

def replace_mos_params(hparams: h.Mos.Params) -> MyMosParams: 
  return MyMosParams(
    **hparams,  # real examples will do more modifications of these
    m=hparams.mult # <= the point is right here
  )
kcaisley commented 1 year ago

Thanks again for the feedback.

I've un-done the improper changes to m multiplier params. However, aside fromSky130.MosParams and Gf180.MosParams paramclasses having two (unnecessary?) aliases for the multiplier, I didn't notice anything regarding the multiplier that would need to be changed in the first place?

The MosParams (in Sky130 and Gf180) with the redundant alias:

https://github.com/dan-fritchman/Hdl21/blob/cf31f94e186cf28ffaf2078d1175ebfd0d9f6dc2/pdks/Gf180/gf180_hdl21/pdk_data.py#L109-L110

The Sky130 compiling behavior (primitive MosParams -> Sky130 MosParams):

https://github.com/dan-fritchman/Hdl21/blob/cf31f94e186cf28ffaf2078d1175ebfd0d9f6dc2/pdks/Sky130/sky130_hdl21/pdk_logic.py#L202-L226

The Gf180 compiling behavior (primitive MosParams -> Gf180 MosParams):

https://github.com/dan-fritchman/Hdl21/blob/cf31f94e186cf28ffaf2078d1175ebfd0d9f6dc2/pdks/Gf180/gf180_hdl21/pdk_logic.py#L159-L182

Did you have something else in mind?

ThomasPluck commented 1 year ago

Yeah, this odd behavior is something we've inherited from xschem and appears to be an on-going source of confusion for users, here are some links from the Slack:

In Gf180, there is no mult: https://open-source-silicon.slack.com/archives/C03R1GRP9LG/p1689929167330209?thread_ts=1689217859.856629&cid=C03R1GRP9LG

In Sky130, mult and m do different things, where one the former just simulates a single transistor and multiplies its attributes to simulate parallel devices and the latter appears to indicate to ngspice that there are n parallel devices to be individually simulated.

Source: https://open-source-silicon.slack.com/archives/C017P3RAD42/p1665691876110269?thread_ts=1665690422.377599&cid=C017P3RAD42

So, this clears things up a little but not much.

kcaisley commented 1 year ago

Great, thank you for those references @ThomasPluck; I will just leave those sections as they are.

I think this pull request should be ready for merge now @dan-fritchman. Locally on end, the test suite looks like this:

hdl21/pdk/test_pdk.py ...
hdl21/pdk/sample_pdk/test_sample_pdk.py ...
hdl21/sim/tests/test_sim.py ........FFx.
hdl21/tests/test_builtin_generators.py ..
hdl21/tests/test_bundles.py .............x......
hdl21/tests/test_conns.py ..............
hdl21/tests/test_doc_examples.py .............
hdl21/tests/test_exports.py x................
hdl21/tests/test_hdl21.py ...............x...................x....................
hdl21/tests/test_params.py ......
hdl21/tests/test_prefix.py ..............................
hdl21/tests/test_source_info.py .
pdks/Asap7/asap7_hdl21/test_pdk.py .
pdks/Gf180/gf180_hdl21/test_netlists.py .......
pdks/Gf180/gf180_hdl21/test_pdk.py ........
pdks/Sky130/sky130_hdl21/test_netlists.py ........
pdks/Sky130/sky130_hdl21/test_pdk.py ...........

The two failures are for test_empty_sim1 and test_empty_sim2, which I identically see when testing the unmodified main branch, so I guess they're related somehow to my local configuration? (Perhaps ngspice related?)

dan-fritchman commented 1 year ago

Eh I don't need to bug ya with this one-character one-comment typo, I got it.

MERGE MERGE MERGE.