Blinking light n times

I’m working through the Retroprogramming with Clash book and am currently stuck on the blinking light n times problem. Not sure what’s wrong with my current solution as it’s not blinking at all. Any hints or tips would be greatly appreciated.

{-# LANGUAGE DataKinds #-}
{-# LANGUAGE DeriveAnyClass #-}
{-# LANGUAGE NumericUnderscores #-}
{-# LANGUAGE NoImplicitPrelude #-}
{-# OPTIONS_GHC -Wno-simplifiable-class-constraints #-}

import Clash.Class.Num
import Clash.Prelude
import RetroClash.Clock
import RetroClash.Utils

data OnOff on off = On (Index on) | Off (Index off)
  deriving (Generic, NFDataX)

{-# ANN
  topEntity
  ( Synthesize
      { t_name = "topEntity",
        t_output = PortName "led_0",
        t_inputs =
          [PortName "clk"]
      }
  )
  #-}
{-# OPAQUE topEntity #-}
topEntity :: Clock System -> Signal System Bit
topEntity clk = withClockResetEnable clk resetGen enableGen (blinkN d9)

blinkN ::
  forall n.
  (HiddenClockResetEnable System, KnownNat n, KnownNat (n + 1)) =>
  SNat n ->
  Signal System Bit
blinkN n = mux done (pure low) (boolToBit . isOn <$> r)
  where
    done :: Signal System Bool
    done = (>= snatToNum n) <$> cnt

    cnt :: Signal System (Index (n + 1))
    cnt = register 0 $ satSucc SatBound <$> cnt

    r :: Signal System (OnOff (ClockDivider System (Seconds 1)) (ClockDivider System (Seconds 1)))
    r = register (Off 0) $ countOnOff <$> r

isOn :: OnOff on off -> Bool
isOn On {} = True
isOn Off {} = False

countOnOff :: (KnownNat on, KnownNat off) => OnOff on off -> OnOff on off
countOnOff (On x) = maybe (Off 0) On $ succIdx x
countOnOff (Off y) = maybe (On 0) Off $ succIdx y

Hello east710! Welcome to our Discourse!

This counter is much too quick! It counts 10 clock cycles, which is a really short period. It should count 10 somethings of the r variable in some way.

About that deleted message of mine above: the text was a case of “need coffee before code”. I tried to edit it, accidentally clicked delete somehow, it was gone without needing any more confirmation.

Thank you for the welcome and the tip. After a while I managed to get this

blinkN ::
  forall n.
  (HiddenClockResetEnable System, KnownNat n, KnownNat (n + 1)) =>
  SNat n ->
  Signal System Bit
blinkN n = mux done (pure low) (boolToBit . isOn <$> r)
  where
    done :: Signal System Bool
    done = (>= snatToNum n) <$> c

    c :: Signal System (Index (n + 1))
    c = register 0 $ mux tick (satSucc SatBound <$> c) c

    tick :: Signal System Bool
    tick = (== On maxBound) <$> r

    r :: Signal System (OnOff (ClockDivider System (Seconds 1)) (ClockDivider System (Seconds 1)))
    r = register (Off 0) $ countOnOff <$> r

It works but is there a simpler way to do it?

I think that looks okay. Due to the “modular” construction it’s maybe a bit spread out.

I love the way the book writes Clash code, but I’m also rather unfamiliar with that style. So my efforts might not hit the mark everywhere. Still, I thought if you could move the “blink N times” functionality into the “blink” functionality maybe you can get something less spread out. The first version I was at least somewhat satisfied about is this:

BlinkHiddenIndexN.hs
{-# LANGUAGE NumericUnderscores, PartialTypeSignatures #-}
module BlinkHiddenIndexN where

import Clash.Prelude
import Clash.Annotations.TH

import RetroClash.Utils (withResetEnableGen, succIdx)
import RetroClash.Clock
import Data.Either
import Data.Maybe

createDomain vSystem{vName="Dom100", vPeriod = hzToPeriod 100_000_000}

data OnOff on off
    = On  (Index on)
    | Off (Index off)
    deriving (Generic, NFDataX)

type OnOffN n on off = Maybe (Index n, OnOff on off)

isOn :: OnOff on off -> Bool
isOn On{} = True
isOn Off{} = False

countOnOffN
  :: (KnownNat n, KnownNat on, KnownNat off)
  => OnOffN n on off
  -> OnOffN n on off
countOnOffN Nothing = Nothing
countOnOffN (Just (n, On x)) =
  maybe
    ((,Off 0) <$> succIdx n)
    (Just . (n,) . On)
    $ succIdx x
countOnOffN (Just (n, Off y)) = Just $ (n,) $ maybe (On 0) Off $ succIdx y

topEntity
    :: "CLK" ::: Clock Dom100
    -> "LED" ::: Signal Dom100 Bit
topEntity = withResetEnableGen $ blinkingSecondN d10

blinkingSecondN
  -- :: forall n dom. (HiddenClockResetEnable dom, 1 <= DomainPeriod dom)
  :: forall n dom. (HiddenClockResetEnable dom, _)
    => SNat n
    -> Signal dom Bit
blinkingSecondN SNat = boolToBit . maybe False (isOn . snd) <$> r
  where
    r ::
      Signal
        dom
        ( OnOffN n
            (ClockDivider dom (Milliseconds 500))
            (ClockDivider dom (Milliseconds 500)))
    r = register (Just (0, Off 0)) $ countOnOffN <$> r

makeTopEntity 'topEntity

but sticking to the maybe function didn’t do the readability of countOnOffN any good. So let’s drop that and use pattern guards, where you do a pattern match and bind variables inside of a guard:

countOnOffN
  :: (KnownNat n, KnownNat on, KnownNat off)
  => OnOffN n on off
  -> OnOffN n on off
countOnOffN Nothing = Nothing
countOnOffN (Just (n, On x))
  | Just x0 <- succIdx x = Just (n, On x0)
  | Just n0 <- succIdx n = Just (n0, Off 0)
  | otherwise = Nothing
countOnOffN (Just (n, Off y))
  | Just y0 <- succIdx y = Just (n, Off y0)
  | otherwise = Just (n, On 0)

Yeah, that’s better.

Note that the definition of blinkingSecondN has a commented-out contraint where we specify the full constraints instead of letting GHC decide that for the partial type signature that the book specifies. I found that if I made a mistake somewhere, the only error GHC would give was that the 1 <= DomainPeriod dom constraint wasn’t satisfied, without mentioning the actual error that caused GHC to fail compilation. So that partial type signature was truly getting in the way. Once I removed the bugs, I could do the partial type signature again, but really, I don’t think it still adds something at that point, in this case. Still, I reinstated it as it was in the original code.