Closed DigitalBrains1 closed 9 months ago
Ah, I had neglected to mention that I was still working on the Intel documentation, I simply copied the Xilinx one and still need to adjust those things.
Since your review, I've made the following changes:
Clash.Intel.ClockGen
Prelude
as I think this is more familiar to a noviceunsafe
functionsAdjusted the commit message for the Improve readability of Clash.Clocks.Deriving commit, from No functional change to:
No functional change, except that the
clocks
function is now annotated with aCLASH_OPAQUE
"pragma" rather than a mereNOINLINE
.
We discovered this had been overlooked for PR #2511, but since this PR fundamentally changed the code, we decided to adjust it here.
We have the convention that if a function can lead to timing violations, we prefix the name of that function with
unsafe
. The functions inClash.Intel.ClockGen
andClash.Xilinx.ClockGen
before this PR do not follow that convention. Thelocked
output of these clock generators is an asynchronous signal. All our examples showed the correct way to handle this, but it should be called out by anunsafe
prefix, and we should offer an easy-to-use alternative. It was too easy for a user to forget to synchronise thelocked
signal.This PR deprecates the Intel functions and adds new functions, both a safe variant and, for advanced use cases, an unsafe variant. The safe variant incorporates
resetSynchronizer
so there's just a ready-to-useReset
instead of the asynchronouslocked
signal.For Xilinx, it was decided to not deprecate the functions but rather scrap them altogether and provide new, safe ones with the same name as the old unsafe ones. The functions in Clash 1.6 are so broken that keeping them around as deprecated functions doesn't add much utility for our users; better to bite the bullet. PR #2427 had already changed the API for Xilinx, by the way.
unsafeFromActiveLow
. Now people can follow the recommendation without being exposed tounsafe
stuff.Clash.Magic.setName
if they need a predictable name.ResetPolarity
of the input domain instead of always expecting an active-high signal.Still TODO: