Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add reset-related convenience functions #2539

Merged
merged 2 commits into from Aug 1, 2023
Merged

Conversation

martijnbastiaan
Copy link
Member

@martijnbastiaan martijnbastiaan commented Jul 13, 2023

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Write a changelog wrt added type families
  • Check copyright notices are up to date in edited files

Copy link
Member

@DigitalBrains1 DigitalBrains1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are definitely good to have! Let's get rid of "you need unsafeX to do this"! I'd like to do it a bit different though. Type safety is great. Amassing type constraints not so much.

clash-prelude/src/Clash/Explicit/Reset.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Explicit/Reset.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Explicit/Reset.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Explicit/Reset.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Explicit/Reset.hs Outdated Show resolved Hide resolved
@martijnbastiaan
Copy link
Member Author

@DigitalBrains1 @leonschoorl I've added proposal for the new constraint aliases. I'm reasonably happy with their names and definition (keep in mind the long-term plan is to get rid of KnownDomain except for things like resetGen and clockGen). I also think these two constraints will cover almost all cases, so I don't feel like it's necessary to add constraints for ActiveEdge and friends. Lastly, I'm not super-pleased with the __N.B.__ explanations right now, please make suggestions (unless you guys think it's clear enough!).

@martijnbastiaan
Copy link
Member Author

martijnbastiaan commented Jul 26, 2023

Maybe HasSynchronousReset dom / SetsSynchronousReset dom? (Not a big fan of contracting it to Sync btw, we don't do that anywhere else we don't do that in that module.)

@martijnbastiaan
Copy link
Member Author

I've picked HasX in the end. It's a pattern used in the Haskell ecosystem more often, and I feel it pretty much reads as this domain has X.

@martijnbastiaan martijnbastiaan added this to the 1.8 milestone Jul 30, 2023
@DigitalBrains1
Copy link
Member

I'd like it if the documentation points out how users can manage these constraints efficiently. We discussed how people could create their own type synonyms in a single design with multiple clock domains like

type DesignDomain dom =
  ( HasSynchronousReset dom
  , HasDefinedInitialValues dom
  )

and then write all their domain-polymorphic functions in the design as

f ::
  DesignDomain dom =>
  Signal dom [...]

which is just as much code as KnownDomain dom, but correctly provides constraints whenever they use, say, orReset somewhere.

@DigitalBrains1 DigitalBrains1 dismissed leonschoorl’s stale review August 1, 2023 15:52

Suggested changes have been applied

@martijnbastiaan martijnbastiaan removed the request for review from leonschoorl August 1, 2023 15:53
@martijnbastiaan martijnbastiaan merged commit 3bcc674 into master Aug 1, 2023
12 of 13 checks passed
@martijnbastiaan martijnbastiaan deleted the add-reset-functions branch August 1, 2023 16:34
DigitalBrains1 added a commit that referenced this pull request Aug 2, 2023
Even though the relative links all worked in `Clash.Signal.Internal`,
`Clash.Signal` and `Clash.Explicit.Signal`, it was broken in
`Clash.Prelude`.

This amends PR #2539
martijnbastiaan added a commit to bittide/bittide-hardware that referenced this pull request Aug 9, 2023
martijnbastiaan added a commit to bittide/bittide-hardware that referenced this pull request Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants