Add stateful mealyS

[axman6]

Add a mealyS :: ... (i -> State s o) -> s -> Signal dom i -> Signal dom o function to Clash.Explicit.Mealy and
Clash.Prelude.Mealy, along with doctests showing how lenses can be used for defining the state modifications.

I've also updated the tutorial to use this, as it simplifies the example a lot and avoids the extra plumbing needed to fix the order of arguments in macS.

The dependency on mtl (Control.Monad.State.Strict) could easily be a dependency on transformers, (Control.Monad.Trans.State.Strict), I just added the one I seem more frequently but happy to change if needed.

Still TODO:

• Doctests currently fail for un-logged reasons
• Update tutorial
• Consider if it makes sense to have a Unbundled mealySB
• Consider if it makes sense to have a Moore equivalent
• Write a changelog entry (see changelog/README.md)
• Check copyright notices are up to date in edited files

[axman6]

Some notes on the WIP

[axman6]

I've run into some issues with this PR that I'm struggling to track down. I have an extended version of the problem I'm seeing with both the mealy and my mealyS functions:

{-# LANGUAGE DataKinds, TypeApplications, DeriveGeneric, DeriveAnyClass #-}


import Clash.Explicit.Prelude as C
-- import Clash.Explicit.Mealy (mealyS)
import qualified Data.List as L
import Control.Lens (Lens', (%=), (-=), uses, use)
import Control.Monad.State.Strict (State, runState)

data DelayState = DelayState { _history :: Vec 4 Int , _untilValid :: Index 4 } deriving (Generic,NFDataX)

history :: Lens' DelayState (Vec 4 Int)
history f = \(DelayState d u) -> (`DelayState` u) <$> f d

untilValid :: Lens' DelayState (Index 4)
untilValid f = \(DelayState d u) -> DelayState d <$> f u

-- Maybe Int is the actual output, the rest are debugging
type Out = (Maybe Int, Vec 4 Int, Vec 4 Int, Index 4, Index 4)

delayS :: Int -> State DelayState Out
delayS n = do
  remaining <- use untilValid
  hist <- use history -- debugging
  if remaining /= 0
  then do
    history    %= (n +>>)
    untilValid -= 1
    -- debugging
    histAfter  <- use history
    remAfter    <- use untilValid
    return (Nothing, hist, histAfter, remaining, remAfter)
  else do
    out        <- uses history C.last
    history    %= (n +>>)
    -- debugging
    histAfter  <- use history
    remAfter   <- use untilValid
    return (Just out, hist, histAfter, remaining, remAfter)

initialDelayState = DelayState (C.repeat 7) maxBound

-- From PR - `mealy` using State
mealyS
  :: ( KnownDomain dom
     , NFDataX s )
  => Clock dom
  -- ^ 'Clock' to synchronize to
  -> Reset dom
  -> Enable dom
  -- ^ Global enable
  -> (i -> State s o)
  -- ^ Transfer function in mealy machine handling inputs using @Control.Monad.Strict.State s@.
  -> s
  -- ^ Initial state
  -> (Signal dom i -> Signal dom o)
  -- ^ Synchronous sequential function with input and output matching that
  -- of the mealy machine
mealyS clk rst en f iS =
  \i -> let (o,s') = unbundle $ (runState . f) <$> i <*> s
            s      = register clk rst en iS s'
        in o
{-# INLINABLE mealyS #-}


-- >>> P.mapM_ print $ L.take 7 $ simulate (topEntity systemClockGen systemResetGen enableGen) [1,2,3,4,5,6,7,8]
-- (Nothing,7 :> 7 :> 7 :> 7 :> Nil,1 :> 7 :> 7 :> 7 :> Nil,3,2)
-- (Nothing,1 :> 7 :> 7 :> 7 :> Nil,2 :> 1 :> 7 :> 7 :> Nil,3,2)
-- (Nothing,2 :> 1 :> 7 :> 7 :> Nil,3 :> 2 :> 1 :> 7 :> Nil,2,1)
-- (Nothing,3 :> 2 :> 1 :> 7 :> Nil,4 :> 3 :> 2 :> 1 :> Nil,1,0)
-- (Just 1,4 :> 3 :> 2 :> 1 :> Nil,5 :> 4 :> 3 :> 2 :> Nil,0,0)
-- (Just 2,5 :> 4 :> 3 :> 2 :> Nil,6 :> 5 :> 4 :> 3 :> Nil,0,0)
-- (Just 3,6 :> 5 :> 4 :> 3 :> Nil,7 :> 6 :> 5 :> 4 :> Nil,0,0)
--
-- Actual result
-- clashi> P.mapM_ print $ L.take 7 $ simulate (topEntity systemClockGen systemResetGen enableGen) [1,2,3,4,5,6,7,8]
-- (Nothing,7 :> 7 :> 7 :> 7 :> Nil,1 :> 7 :> 7 :> 7 :> Nil,3,2)
-- (Nothing,7 :> 7 :> 7 :> 7 :> Nil,2 :> 7 :> 7 :> 7 :> Nil,3,2) -- note 1 is missing
-- (Nothing,2 :> 7 :> 7 :> 7 :> Nil,3 :> 2 :> 7 :> 7 :> Nil,2,1)
-- (Nothing,3 :> 2 :> 7 :> 7 :> Nil,4 :> 3 :> 2 :> 7 :> Nil,1,0)
-- (Just 7,4 :> 3 :> 2 :> 7 :> Nil,5 :> 4 :> 3 :> 2 :> Nil,0,0)
-- (Just 2,5 :> 4 :> 3 :> 2 :> Nil,6 :> 5 :> 4 :> 3 :> Nil,0,0)
-- (Just 3,6 :> 5 :> 4 :> 3 :> Nil,7 :> 6 :> 5 :> 4 :> Nil,0,0)
topEntity :: Clock System -> Reset System -> Enable System -> Signal System Int -> Signal System Out
topEntity clk rst en = mealyS clk rst en delayS initialDelayState

-- Also happens with mealy:
-- clashi> P.mapM_ print $ L.take 7 $ simulate (topEntity' systemClockGen systemResetGen enableGen) [1,2,3,4,5,6,7,8]
-- (Nothing,7 :> 7 :> 7 :> 7 :> Nil,1 :> 7 :> 7 :> 7 :> Nil,3,2)
-- (Nothing,7 :> 7 :> 7 :> 7 :> Nil,2 :> 7 :> 7 :> 7 :> Nil,3,2) -- 1 also missing in second loop
-- (Nothing,2 :> 7 :> 7 :> 7 :> Nil,3 :> 2 :> 7 :> 7 :> Nil,2,1)
-- (Nothing,3 :> 2 :> 7 :> 7 :> Nil,4 :> 3 :> 2 :> 7 :> Nil,1,0)
-- (Just 7,4 :> 3 :> 2 :> 7 :> Nil,5 :> 4 :> 3 :> 2 :> Nil,0,0)
-- (Just 2,5 :> 4 :> 3 :> 2 :> Nil,6 :> 5 :> 4 :> 3 :> Nil,0,0)
-- (Just 3,6 :> 5 :> 4 :> 3 :> Nil,7 :> 6 :> 5 :> 4 :> Nil,0,0)
topEntity' :: Clock System -> Reset System -> Enable System -> Signal System Int -> Signal System Out
topEntity' clk rst en = mealy clk rst en (\s i -> let (o, s') = runState (delayS i) s in (s',o) ) initialDelayState

Somehow the first iteration of the simulation seems to somehow reset part of the state, with everything working as expected after that.

[rowanG077]

I didn't look at the code in depth. But this is probably an artifact of resetGen keeping reset high for the first clock cycle. See the example below with resetGen vs unsafeFromHighPolarity $ pure False.

ghci> import qualified Data.List as L
ghci> import Clash.Explicit.Prelude
ghci> foo clk rst en s = register clk rst en (undefined :: Bool) s
ghci> showX <$> L.take 10 $ simulate (foo systemClockGen resetGen enableGen) (L.repeat True)
"[undefined,undefined,True,True,True,True,True,True,True,True]"
ghci> showX <$> L.take 10 $ simulate (foo systemClockGen (unsafeFromHighPolarity $ pure False) enableGen) (L.repeat True)
"[undefined,True,True,True,True,True,True,True,True,True]"

[axman6]

Hmmm, that seems like a likely explanation! Any advice on how to work around that?

[rowanG077]

If mealyS behaves the same as mealy then I'd think this is exactly what you want? You can add a dummy input to your testcase for the first clock cycle where the circuit is in reset.

[axman6]

Thanks @rowanG077, with that bit of insight I now have working doctests. I do find the discrepancy between the Prelude and Explicit modules a bit disconcerting, is there a good reason for the difference in behaviour?

Other than the asymmetry between the two version's tests, I think this is ready to go (assuming the idea is a acceptable).

[rowanG077]

To make a long story short: It's a historical accident that we pay the price for. Perhaps we should deprecate the implicit simulate and direct people to sample instead.

[christiaanb]

@kloonbot run_ci e3b55ad

[alex-mckenna]

@kloonbot run_ci 1cbfc58

[axman6]

@kloonbot run_ci 1cbfc58

Any chance of one more CI run @alex-mckenna? Hoping that's sorted the haddock issue (took me ages to track down where the functions were being being imported and exported from!). Also is there any docs about tests that should be run locally before CI? It'd be nice to not to need someone else in the loop of checking what I've broken.

[alex-mckenna]

Sure thing

@kloonbot run_ci 026d78a

[DigitalBrains1]

If someone doesn't beat me to it, I will fix the broken CI tomorrow or the day after.

[axman6]

Thanks @DigitalBrains1, yeah it looks like I've resolved all my issues.

The PR could probably do with a review, I've made the changes I could find that made sense, but haven't done things like add tests (I didn't see any for the other mealy functions, probably because they're so simple and obviously correct). I'm not sure if there is other documentation needed, or things like since annotations, etc.

[martijnbastiaan]

@kloonbot run_ci 0d91384

[DigitalBrains1]

I really like this addition! Actually providing some basic pointers about using lenses and the state monad is very helpful to get users going.

I noticed that you worked on your examples a bit more but forgot to copy the updated code to the actual example in Haddock. These suggestions should fix that; I verified they actually work (if you put it in a .hs file and add some import statements).

[axman6]

Thanks heaps for the review @DigitalBrains1, I've incorporated your changes, I had indeed forgotten to go back and update them (getting the doctests was such a pain I completely forgot about the actual haddocks).

I've now also added the mealySB variants which handle bundling, I've kept their docs brief because it should be fairly obvious from the context of mealyS and mealyB.

[DigitalBrains1]

Oh yes, the doctests can be a real pain. Usually with extensions, but especially the fact that there is no way to do NoImplicitPrelude, whereas that is a basic tenet of Clash... :-(

It is a good idea to add mealySB, thanks!

I don't think this is the right moment to introduce a linter pragma... I feel if we start doing that, we need to be more systematic about it, not just drop a single one here. I suggest we don't do it in this PR.

[axman6]

Oh sorry about that @DigitalBrains1, that was left in by mistake, that was a HOS suggestion I forgot about. If you suggest the change I'll accept it, I'm away from my computer at the moment.

[DigitalBrains1]

@kloonbot run_ci 51bb400

[DigitalBrains1]

I see you still have a TODO note open:

Consider if it makes sense to have a Moore equivalent

Personally, I think it's not needed because writing a Moore circuit with State is easy to accomplish using register as the memory element (you could look at the UART in Clash.Examples although that module could use some tender love and care. It does show how easy it is.)

I was going to merge this and then noticed the unfinished task. Are you happy with the current state, can I merge it?

[DigitalBrains1]

Also, this is going to be a Squash and merge style merge. Do you have a preference regarding commit message or can I cobble something together from your PR cover letter? :-)

[axman6]

Yep happy to merge, that task was just there to spark discussion but the more so thought about it the less it made sense. Also happy with squash and merge, with whatever message is appropriate for the repo. Thanks for all your persistence!

[DigitalBrains1]

Thanks for all your persistence!

Hahaha, I should be the one saying that to you :-). Thanks for the contribution!