Open leonschoorl opened 11 months ago
Overall this looks good, although it will take me more time to review it properly. Concerning the bike shedding, here are my thoughts:
Don't do anything. From the looks of it, we have quite a few "normal" functions in clash-prelude
that want to extract KnownDomain
s. "Normal" in the sense that these are functions our users would probably write. Its probably best to dogfood a solution and couple them to the changes. Still, this solution doesn't necessarily worsen anything. (I.e., a proposal still propagating KnownDomain
in some places is still a strict improvement.)
Make your code monomorphic Similar reasoning to (0), with the addition that you can't really monomorphize library functions.
Use the new provideKnownDomainFrom
. I like this. It is explicit, and our users are already exposed to similar functions. I'd prefer the name withKnownDomain
/withKnownDomainFrom
to make it fit in with the IP functions.
Use the new ExtractClockDom
and ExtractResetDom
PatternSynonyms
to extract the KnownDomain
for you. In general I think pattern synonyms should only be applied whenever the act like a proper constructor (and you never need to access actual constructors). E.g., the way Seq
does it. I think this, because for any other use it's yet another Haskell concept for users to grasp, which syntactically looks exactly like (de)construction potentially making it very confusing.
Change or make variants of (some of) these that take an additional Clock dom argument. I've already found it confusing in practice for unsafeSynchronizer
, so I'd rather not do this. Plus, like you said, this would hard break the current API. (The latter point weighing the most.)
An alternative take on (3) would be to wrap existing types (Clock
/Reset
/Enable
) and make them carry the KnownDomain
constraint. Their constructors could be safely exported. Though this also feels kinda dirty..
My preference would be (2), (0), (3).
Removing the need for KnownDomain constraints
This an implementation of the idea from #978, removing (most of the need for) the KnownDomain constraint. The core change of this PR is to hide the
KnownDomain
constraint inside theClock
andReset
types.The allows us to drop the
KnownDomain
constraint from every function take takes either aClock
or aReset
, which is most of them.The only things that still require a
KnownDomain
constraint are functions that createClock
s orReset
s:noReset
,clockGen
,resetGen
and their variantssimulate
,sample
and all their variants [^1]unsafeFromActive[High,Low]
,unsafeToReset
altpll
,alteraPll
,clockWizard
,clockWizardDifferential
And some tracing functions:
traceSignal
,traceVecSignal
[^1]: Except the Explicit variants of
simulate
,simulateB
,sample
,sampleN
,sample_lazy
andsampleN_lazy
.When using any of these functions that still requires a
KnownDom
there a couple of ways of handling it:KnownDomain
constraints for you.Use the new
provideKnownDomainFrom
Click here for its definition
```haskell class HasKnownDomain a where provideKnownDomainFrom :: a dom -> (KnownDomain dom => r) -> r instance HasKnownDomain Clock where ... instance HasKnownDomain DiffClock where ... instance HasKnownDomain Reset where ... ```This helper function take something containing a KnownDomain contraint as first argument, extracts it and provides it to the second argument.
Use the new
ExtractClockDom
andExtractResetDom
PatternSynonyms to extract the KnownDomain for youClick here for its definition
```haskell pattern ExtractResetDom :: () -- constraint required to match the pattern => KnownDomain dom -- constraint provided by matching the pattern => Reset dom pattern ExtractResetDom <- Reset {} -- and the same for ExtractClockDom {-# COMPLETE ExtractResetDom #-} ```ExtractResetDom
is just a synonyms for the patternReset{}
. But limited in such a way that it is safe to use, and won't generate broken (V)HDL, and therefore is safe to export fromClash.Prelude
etc.Change or make variants of (some of) these that take an additional
Clock dom
argument.I'm think this could be convenient for:
noReset
,traceSignal
,traceVecSignal
and maybeunsafeFromActive[High,Low]
.Options
0.
and1.
are nothing new, they were already possible before this PR. Options2.
and3.
are new options added in this PR, but I'm unsure if it makes sense to add both, or if that would just clutter up the API unnecessarily. Also I'm not happy with the current names, they could do with some bike shedding. Option4.
I haven't implemented, could be convenient, but would be more incompatible API changes, or add more slightly different variants of existing functions.Still TODO: