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

Push users towards starter projects #77

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DigitalBrains1
Copy link
Member

@DigitalBrains1 DigitalBrains1 commented Jan 24, 2025

They are really the best supported flow, and we should make it clear other methods can have surprises.

The stack exec line uses a working Stackage nightly that has a version of Clash we actually support.

Our Clash starter projects enable -Wall -Wcompat. I took the -Wall and added it to the stack exec line as well, as it is the most important one. Adding -Wcompat would make the invocation more verbose without a significant increase in functionality.

Copy link
Member

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

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

Until Cabal can handle project setup and GHC installations (or at least provided good error messages) this is the way to go 👍

People familiar with the Haskell ecosystem might prefer to use Cabal instead of Stack. To do so, [download the starter project as a zip](https://raw.githubusercontent.com/clash-lang/clash-starters/main/simple.zip) and follow the instructions in [`README.md`](https://github.com/clash-lang/clash-starters/tree/main/simple#simple-starter-project).

#### Run Clash on its own
The following compiles a file `HelloWorld.hs` to VHDL:

Choose a reason for hiding this comment

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

Does it make sense to add the contents of such a file here? That way people can try this command out without first having to know how to write such a file? It could be really small and simple too, but I think something would be nice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good idea! The thought briefly entered my head but went on its way before I could examine it :-).

I can whip up an LED blinker in just a few lines of code. It won't have all the trimmings, but it'd be working code. I'll propose one later.

Copy link
Member Author

@DigitalBrains1 DigitalBrains1 Feb 8, 2025

Choose a reason for hiding this comment

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

Would this be a nice file? Or should we make something even shorter? Or avoid the use of Functor or the dot function composition operator?

{-# OPTIONS_GHC "-Wno-orphans" #-}

module ShortBlinker where

import Clash.Class.Counter
import Clash.Explicit.Prelude

-- 50 MHz clock (period of 20,000 ps), synchronous resets
createDomain vXilinxSystem{vName="DomSys", vPeriod=20_000}

-- Number of cycles in 0.5 seconds
type HalfSecDelay = 25_000_000
-- Or let Clash compute it: 500e9 picoseconds divided by clock period
-- type HalfSecDelay = 500_000_000_000 `Div` DomainPeriod DomSys

topEntity ::
  Clock DomSys ->
  Reset DomSys ->
  Signal DomSys Bit
topEntity clk rst = (bitCoerce . fst) <$> counter
 where
  counter :: Signal DomSys (BitVector 1, Index HalfSecDelay)
  counter = register clk rst enableGen (0,0) (countSucc <$> counter)
{-# OPAQUE topEntity #-}

I would have loved to be able to do topEntity clk rst = fst <$> counter but unfortunately the necessary Counter Bit instance is a Clash 1.10 thing...

Choose a reason for hiding this comment

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

This is probably just my inexperience showing, but is creating a domain necessary to test VHDL translation? What happens if you use System?

I'd intuitively say that since this is not a tutorial on the language itself, some "easy to understand" code that's as small as possible is probably the way to go? So maybe only use the computed delay, not the hardcoded one (or the other way around)? But overall I think this is a nice small and simple example 👍

Copy link
Member Author

@DigitalBrains1 DigitalBrains1 Feb 10, 2025

Choose a reason for hiding this comment

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

It was just a method to allow people to

  1. Adjust the frequency
  2. See how we can actually have Clash adjust the type

Also, it was meant as a really small somewhat complete example. If you enter the frequency of the crystal of your dev board and map the two inputs and one output to the correct pins, you'll get a blinking LED. And by "somewhat complete" I mean "close to what you would actually write", so again, probably with a domain.

Finally, the System domain has asynchronous resets, but I think both Xilinx and Intel, the large players, recommend synchronous resets these days. But using XilinxSystem feels clunky if you're not actually targetting Xilinx.

But if you feel this is not convincing, I can remove the custom domain and the "let Clash compute it" snippet.

some "easy to understand" code that's as small as possible is probably the way to go?

Ah, I think I see the impedance mismatch. I was going for code that was actually useful: it will blink a LED on a dev board. You were going for the minimum code that creates HDL. For that topEntity = (+ 1) would be more than sufficient (with a monomorphic type).

Copy link
Member Author

@DigitalBrains1 DigitalBrains1 Feb 10, 2025

Choose a reason for hiding this comment

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

So maybe only use the computed delay, not the hardcoded one

In that case I'd still like to note in a comment that the outcome is 25,000, so people have an immediate feeling about what this does. Note I explained all the non-trivial numbers I used in the code and I deliberately did that.

In fact, I'd like to make it

-- Let Clash compute the number of cycles in 0.5 seconds:
-- 500e9 picoseconds divided by clock period
type HalfSecDelay = 500_000_000_000 `Div` DomainPeriod DomSys
-- For the 50 MHz clock, this is equivalent to
-- type HalfSecDelay = 25_000_000

Choose a reason for hiding this comment

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

I think this is a small enough example that it's fine, those were just ideas. I think the clash-computed delay is still nicer but keeping the explanation is good!

@DigitalBrains1
Copy link
Member Author

DigitalBrains1 commented Mar 3, 2025

Lara, I've changed the ShortBlinker.hs file to use the Let Clash compute the delay variant, uploaded it to the site and linked it, along with some minor changes. I hope we're all happy with how it is now :-).

I experimented with adding the code inline in the installation instructions as well as having a download link, but felt it got too prominent. Which is a good thing, because I also didn't like how it looked and it would have needed some CSS work, and I'm really inexperienced with CSS.

Note that our starter projects turn on -Wall -Wcompat so I felt that adding -Wall to the stack exec line was very justified. I've edited the commit message and the PR cover letter (which is literally the commit message) to justify this and leaving out -Wcompat.

They are really the best supported flow, and we should make it clear
other methods can have surprises.

The `stack exec` line uses a working Stackage nightly that has a version
of Clash we actually support.

Our Clash starter projects enable `-Wall -Wcompat`. I took the `-Wall`
and added it to the `stack exec` line as well, as it is the most
important one. Adding `-Wcompat` would make the invocation more verbose
without a significant increase in functionality.
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.

3 participants