Adds Orangecrab starter project #38
Conversation
You could do it similarly to how Christiaan did it with the DECA starter: Tests.DECA |
DigitalBrains1
left a comment
DigitalBrains1
left a comment
There was a problem hiding this comment.
Thanks for adding another nice target!
This is quite a different setup than the other starter projects. I'd rather have more uniformity; I feel it should either be similar to simple or to simple-nix. If you feel those are inadequate, let's upgrade them all. I think it's kinda hard to document or support the starter projects if they each use a different project setup and even enable different extensions in the cabal file. It's also confusing to people when they switch dev boards.
| import Clash.Prelude | ||
|
|
||
| -- | 48 MHz oscillator clock of the OrangeCrab board. | ||
| createDomain vSystem |
There was a problem hiding this comment.
I think synchronous resets are usually used on Lattice ECP5? This domain has async resets.
There was a problem hiding this comment.
Ah, it also suddenly popped in my mind the ECP5 doesn't have defined initial values, right? That's another thing not reflected in this domain.
There was a problem hiding this comment.
Honestly, I have no idea. So I'll look into it :)
projects/orangecrab/src/Blink.hs
Outdated
| "BTN" ::: Reset Dom48 -> | ||
| "rgb_led0" ::: Signal Dom48 RGB | ||
| topEntity clk rst = withClockResetEnable clk rst enableGen | ||
| $ driveRGB $ mealy (~~>) (0 :: Index 48_000_000, 0 :: Index 9) $ pure () |
There was a problem hiding this comment.
I would prefer code more accessible to beginners. This has an infix function, a section and a use of the $ operator.
If you just bind a name to the vector of colours, you can replace the latter two in a visually clear way.
One thing that you could use here is the Counter instance for (Index 9, Index 48_000_000).
projects/orangecrab/src/RGB.hs
Outdated
| where | ||
| c ~~> Color{..} = | ||
| ( satSucc SatWrap c | ||
| , RGB (c >= r) (c >= g) (c >= b) |
There was a problem hiding this comment.
Maybe add a comment that "always on" isn't actually possible with this setup, but this is nicely succinct and visually indistinguishable? Explain to the user what's going on.
Alternatively, cycling through 0 to 254 would actually allow you to do both always-off and always-on, wouldn't it?
|
I've gone ahead and simplified the example code. A couple opinionated choices I made:
My solution was to make the example work without needing input (just cycle LEDs), but if pin B8 is driven high (corresponding to breakout PMOD 1_4), then the user can play with the orangecrab example. That way, the example still works without the breakout board, but it becomes interactive with the breakout board (or the user can drive the pin themselves). I'm still working on writing a few more comments and updating README, but feedback welcome. |
|
[edit] I think this post I made is too unnuanced, please read my next message for a more nuanced take [/edit]
I really think we should first make a release of those packages before we encourage new users to use those features... they're welcome to them, but they are also experimental and without any API guarantees. Is this something that was discussed? I don't remember hearing this before. I hope I'll have time to do a bit more review sometime soon. |
|
No, it wasn't discussed or anything. Personally, I'm a strong proponent of pushing them even while they're in the experimental phase. But, leaving it out makes this PR a bit easier, so no complaints on my end. |
|
I was actually rephrasing my post because I was unhappy with it. But now that you answered, I'll put it here. I'm a bit conflicted, since we haven't released these packages and they are still experimental and without API guarantees. There's definitely something to say for just offering it to users with a bunch of caveats, even before a release. We do do this in a sense, as they're right at the top of our Ecosystem page, including instructions for how to install them in the starter project. All the things in the ecosystem page are interesting, so giving our ecosystem page (which contains the instructions to add the packages!) a more prominent place in the README for the starter projects seems very sensible. We might need to be a bit more explicit about the stability of |
So do you do not want to pull in any Haskell dependencies via the Nix flake? Why not? If someone already uses Nix, why not allow them to use the Nix cache for the Haskell dependencies? |
DigitalBrains1
left a comment
DigitalBrains1
left a comment
There was a problem hiding this comment.
You forgot to run render.hs.
Could you add Stack support?
I still have more feedback, but there's no more week left now. I'll continue later; this is already a lot!
There was a problem hiding this comment.
I don't think we should include a LICENSE file in our starter projects. Our users are free to choose a licence for their projects and can create their own LICENSE file if they want to.
| {-| | ||
| Module : Domain | ||
| Copyright : Copyright © 2024 QBayLogic B.V. | ||
| License : MIT |
There was a problem hiding this comment.
Please see here:
The default license for each of the starter project is BSD2. However, this whole repository -including every starter project- is licensed under CC0. That means the authors, to the extent possible under law, have waived all copyright and related or neighboring rights to this "Clash Example Project". Feel free to choose any license for the starter projects that you want.
Would the authors of this Orangecrab code and accompanying files mind releasing it under CC0 1.0 Universal? Otherwise I think we have a bit of a problem regarding the licence. Once these files are relicensed under CC0, I'd like to suggest removing the whole licence header from all files, similar to how the other starter projects do it.
There was a problem hiding this comment.
The makefile in here evolved from a long history of quick-and-dirty updates of some past projects of mine, where I mostly was interested in getting a rudimentary synthesis flow just running back then. You can still find some public origins of it at https://github.com/reactive-systems/KitchenTimer/blob/master/makefile, or a later variant that's more close to the given version at https://github.com/kleinreact/clash-demo/blob/main/makefile. Licenses apply as given by these repositories.
All other lines of Haskell code in here that carry my signature have been created as an employee for QBayLogic. Hence, QBayLogic owns the code and can always re-license it according to their demands.
There was a problem hiding this comment.
Licenses apply as given by these repositories.
Are you saying you don't want to relicense your makefile to be compatible with this repository's license? If so, how do you think we should proceed?
There was a problem hiding this comment.
Afaik MIT does not put any restrictions in terms of reuse or relicensing.
What I wanted to say is that everybody should be able to do with the makefile whatever they want without the need of any action of its originator. That's the joy of MIT, right?
There was a problem hiding this comment.
I'd say this part of the MIT licence
The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.
prevents recipients of the licensed work from redistributing it under CC0 (or potentially other licences; the whole idea is that the user of the starter project is unencumbered in their choice of licence for their project).
But it's fairly inconsequential here; if you, the author, are okay with releasing the Makefile under CC0, that is all that needs to be established here in this PR.
Thank you!
There was a problem hiding this comment.
I notice the README does not mention how to get a starter project going, like we do for simple:
Stack users can run
stack new my-clash-project clash-lang/simple. Cabal users can download a zip containing the project.
Could you make the README look more like the README for simple, just deviating where useful?
There was a problem hiding this comment.
This file differs wildly from the one in simple. Please use the version of simple and only deviate where warranted.
DigitalBrains1
left a comment
DigitalBrains1
left a comment
There was a problem hiding this comment.
Ah woops I'm requesting changes with my review immediately above but forgot to select that again after my choice got reset through a little oopsie that cost me tons of time...
|
Thanks a lot for all your work on the nice starter project! It has a well-written instructive README, it is easy to use and it adds support for a very affordable dev board. You could say a starter dev board. Note that all my little style suggestions to the README could quickly be applied with the "Add suggestion to batch" button and then the batch can be committed. However, GitHub has a new view for the Files tab and it doesn't seem to support that. If you want to be able to add suggestions to a batch, go to the Files tab and in the top right corner find the "Preview - Switch back - Feedback" text. Click "Switch back", and you'll get back to the old view that has "Add suggestion to batch". Of course if instead of "Preview - Switch back" it says "✨ Try the new experience", then you're already in the old view. I wrote:
And I thought it'd be a matter of copying the Perhaps you can add this to USE_STACK=yesand then use that var in the |
| -l ${BUILDDIR}/log/synth.log \ | ||
| -p 'read_verilog $(dir $^)/*.v; synth_ecp5 -top $(TOP) -json $@' | ||
|
|
||
| ${BUILDDIR}/02-hdl/${TOP}.v: src/${NAME}.hs dist-newstyle/packagedb src/* |
There was a problem hiding this comment.
So what this means, I think, is:
cabal run clash will only be invoked again, if:
src/${NAME}.hschangesdist-newstyle/packagedbchanges- any file in the directory
srcchanges
But to take this project as an example, if the module OrangeCrab.Domain changes, and the user invokes say make or make upload, then Clash will not be invoked, because the changed file is in a subdirectory. With make, probably nothing happens as everything is up to date. With make upload, it will upload the old bitstream.
I am unsure how you'd tell make when a Haskell package has changed.
There was a problem hiding this comment.
I can think of a partial solution. It requires a two-step process. I didn't succeed in getting it to first run a recipe for a file, and only then check whether that changed the file.
For the haskell target, I'm just using the Makefile as a glorified shell script executer. It is separate from the rest. I was first going to name it cabal, but then I thought that'd be confusing when Stack is added.
${BUILDDIR}/02-hdl/${TOP}.v: ${BUILDDIR}/log/cabal-build.log
mkdir -p ${BUILDDIR}/01-clash
echo
${CABAL} v2-run clash -- \
-package-env $(abspath).ghc.environment.* \
--verilog \
-outputdir ${BUILDDIR}/01-clash \
${NAME}
rm -Rf ${BUILDDIR}/02-hdl
mv ${BUILDDIR}/01-clash/${NAME}.topEntity ${BUILDDIR}/02-hdl
haskell:
mkdir -p ${BUILDDIR}/log
${CABAL} v2-build --build-summary=${BUILDDIR}/log/cabal-build.log
.PHONY: haskellNow if you invoke make haskell followed by any other target, only the necessary steps are done, because when cabal v2-build concludes that nothing needs to be done, it's log / build summary file will remain untouched. Conversely, when something Haskelly changes, that file gets a new mtime and other targets will pick up on that.
Note I also changed the way the top level Haskell file is passed to cabal run clash.
| -outputdir ${BUILDDIR}/01-clash \ | ||
| $< | ||
| rm -Rf ${BUILDDIR}/02-hdl | ||
| mv ${BUILDDIR}/01-clash/${NAME}.topEntity ${BUILDDIR}/02-hdl |
There was a problem hiding this comment.
This both assumes that the top entity is named literally topEntity and that there are no subentities with a SYNTHESIZE annotation (they would not be copied here). I think the README should mention these requirements.
| ${CABAL} run clash -- \ | ||
| -package-env $(abspath).ghc.environment.* \ | ||
| --verilog \ | ||
| -outputdir ${BUILDDIR}/01-clash \ | ||
| $< |
There was a problem hiding this comment.
This invokes
cabal run clash -- src/Blink.hs
but that is not truly the correct thing for a project with the structure of our starter projects. You should just invoke
cabal run clash -- Blink
which will pick up the entity from the package built by Cabal.
Passing the source file means you again compile Blink.hs from source and only its imports come from pre-compiled stuff. This also means none of the things in orangecrab.cabal or cabal.project apply to Blink.hs, it is compiled with the builtin settings of the Clash compiler. There are situations where the two are simply not equivalent at all.
This was discussed offline. The main reason is that this is a stack template, so it's tailored to stack users. We don't want to run into the situation where we offer an accessible way to obtain the required synthesis tools(flake.nix) that would pull those users out of their ecosystem. For us the alternative was not offering a We can later still make a template for nix users similar to |
Maybe not the right place to discuss this, but we also still ship NB: some of my private projects even still rely on this fact, as they only need basic stuff like the UART and pulling that from the release package is much more convenient than switching to the new repository and checking it out from there instead. |
|
Ah, yes, you should definitely not consider that a released |
Adds orangecrab starter project based on Felix's original orangecrab project.
One choice I'm not 100% on yet is the choice to include both
Blinkandaccumas functionality insrc/Blink.hs. I currently include both so that users can bothmake uploadto see a change in their orangecrab (blink) and also have Hedgehog tests pre-setup for them (accum). However, this might confuse the user and blink and accum are separate functions that have no relation to one another. Thoughts welcome.