Move BackPressure and Simulate to Protocols.Internal.Classes
#114
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lmbollen
force-pushed
the
lucas/move-classes
branch
from
a33445e
to
3a4af6e
Compare
DigitalBrains1
requested changes
Sep 26, 2024
There was a problem hiding this comment.
I think these also should have been moved in #74.
I'm not very familiar with the code base yet, so I don't really have an opinion on this. In my contribution to that PR, I simply moved those things that needed to be moved. In general, when you're moving stuff around to avoid import loops, if you move too much you'll just have the exact same import loop but with a different module name, or it happens next time you want to do such a thing.
However, since this module just defines classes (well, it should, see further feedback), it seems fairly safe to put too many classes there. I don't readily see how you'd introduce an import loop for your TH that way.
This is preparation for template Haskell generators for tuple instances for
Simulate(and friends?)
Why isn't it simply part of the PR that introduces those?
| } | ||
| deriving (Show) | ||
|
|
||
| instance Default SimulationConfig where |
There was a problem hiding this comment.
This is an instance, not a class.
| directly, as it may be extend with other options in the future. Use 'def' | ||
| instead. | ||
| -} | ||
| data SimulationConfig = SimulationConfig |
There was a problem hiding this comment.
Hmmm types are not classes either; I see I made the same transgression with Circuit. I suggest we move Circuit back to Protocols.Internal. It was in all likelihood a silly mistake I made.
| input component is not sending any data. Note that, in the Df protocol, | ||
| protocols may send arbitrary acknowledgement signals when this happens. | ||
| -} | ||
| data StallAck |
There was a problem hiding this comment.
Again a type not a class.
|
I feel PR #75 made a wrong choice. It put a function into I also noticed So I think we need about these changes to be applied as well: Diffdiff --git a/clash-protocols-base/clash-protocols-base.cabal b/clash-protocols-base/clash-protocols-base.cabal
index a35f14e..8dd2756 100644
--- a/clash-protocols-base/clash-protocols-base.cabal
+++ b/clash-protocols-base/clash-protocols-base.cabal
@@ -113,7 +113,6 @@ library
exposed-modules:
Protocols.Cpp
Protocols.Internal
- Protocols.Internal.Classes
Protocols.Internal.TaggedBundle
Protocols.Internal.TaggedBundle.TH
Protocols.Internal.TH
@@ -122,4 +121,7 @@ library
Protocols.Plugin
Protocols.Plugin.Internal
+ other-modules:
+ Protocols.Internal.Classes
+
default-language: Haskell2010
diff --git a/clash-protocols-base/src/Protocols/Internal.hs b/clash-protocols-base/src/Protocols/Internal.hs
index d82318b..1905529 100644
--- a/clash-protocols-base/src/Protocols/Internal.hs
+++ b/clash-protocols-base/src/Protocols/Internal.hs
@@ -45,6 +45,119 @@ import GHC.Generics (Generic)
>>> import Protocols
-}
+{- | A /Circuit/, in its most general form, corresponds to a component with two
+pairs of an input and output. As a diagram:
+
+@
+ Circuit a b
+
+ +-----------+
+ Fwd a | | Fwd b
+ +------->+ +-------->
+ | |
+ | |
+ Bwd a | | Bwd b
+ <--------+ +<-------+
+ | |
+ +-----------+
+@
+
+The first pair, @(Fwd a, Bwd a)@ can be thought of the data sent to and from
+the component on the left hand side of this circuit. For this pair, @Fwd a@
+is the data sent from the circuit on the left hand side (not pictured), while
+@Bwd a@ is the data sent to the left hand side from the current circuit.
+
+Similarly, the second pair, @(Fwd b, Bwd)@, can be thought of as the data
+sent to and from the right hand side of this circuit. In this case, @Fwd b@
+is the data sent from the current circuit to the one on the right hand side,
+while @Bwd b@ is the data received from the right hand side.
+
+In Haskell terms, we would say this is simply a function taking two inputs,
+@Fwd a@ and @Bwd b@, yielding a pair of outputs @Fwd b@ and @Bwd a@. This is
+in fact exactly its definition:
+
+@
+ newtype Circuit a b =
+ Circuit ( (Fwd a, Bwd b) -> (Bwd a, Fwd b) )
+@
+
+Note that the type parameters /a/ and /b/ don't directly correspond to the
+types of the inputs and outputs of this function. Instead, the type families
+@Fwd@ and @Bwd@ decide this. The type parameters can be thought of as
+deciders for what /protocol/ the left hand side and right hand side must
+speak.
+
+Let's make it a bit more concrete by building such a protocol. For this
+example, we'd like to build a protocol that sends data to a circuit, while
+allowing the circuit to signal whether it processed the sent data or not. Similarly,
+we'd like the sender to be able to indicate that it doesn't have any data to
+send. These kind of protocols fall under the umbrella of "dataflow" protocols,
+so lets call it /DataFlowSimple/ or /Df/ for short:
+
+@
+ data Df (dom :: Domain) (a :: Type)
+@
+
+We're only going to use it on the type level, so we won't need any
+constructors for this datatype. The first type parameter indicates the
+synthesis domain the protocol will use. This is the same /dom/ as used in
+/Signal dom a/. The second type indicates what data the protocol needs to
+send. Again, this is similar to the /a/ in /Signal dom a/.
+
+As said previously, we'd like the sender to either send /no data/ or
+/some data/. We can capture this in a data type very similar to /Maybe/:
+
+@
+ data Data a = NoData | Data a
+@
+
+On the way back, we'd like to either acknowledge or not acknowledge sent
+data. Similar to /Bool/ we define:
+
+@
+ newtype Ack = Ack Bool
+@
+
+With these three definitions we're ready to make an instance for /Fwd/ and
+/Bwd/:
+
+@
+instance Protocol (Df dom a) where
+ type Fwd (Df dom a) = Signal dom (Data a)
+ type Bwd (Df dom a) = Signal dom Ack
+@
+
+Having defined all this, we can take a look at /Circuit/ once more: now
+instantiated with our types. The following:
+
+@
+ f :: Circuit (Df dom a) (Df dom b)
+@
+
+..now corresponds to the following protocol:
+
+@
+ +-----------+
+ Signal dom (Data a) | | Signal dom (Data b)
+ +------------------------>+ +------------------------->
+ | |
+ | |
+ Signal dom Ack | | Signal dom Ack
+ <-------------------------+ +<------------------------+
+ | |
+ +-----------+
+@
+
+There's a number of advantages over manually writing out these function
+types:
+
+ 1. It reduces syntactical noise in type signatures
+
+ 2. It eliminates the need for manually routing acknowledgement lines
+-}
+newtype Circuit a b
+ = Circuit ((Fwd a, Bwd b) -> (Bwd a, Fwd b))
+
-- | Protocol-agnostic acknowledgement
newtype Ack = Ack Bool
deriving (Generic, C.NFDataX, Show, C.Bundle, Eq, Ord)
@@ -759,3 +872,30 @@ tupCircuits :: Circuit a b -> Circuit c d -> Circuit (a, c) (b, d)
tupCircuits (Circuit f) (Circuit g) = Circuit (reorder . (f *** g) . reorder)
where
reorder ~(~(a, b), ~(c, d)) = ((a, c), (b, d))
+
+{- | Force a /nack/ on the backward channel and /no data/ on the forward
+channel if reset is asserted.
+-}
+forceResetSanityGeneric ::
+ forall dom a fwd bwd.
+ ( C.KnownDomain dom
+ , C.HiddenReset dom
+ , IdleCircuit a
+ , Fwd a ~ C.Signal dom fwd
+ , Bwd a ~ C.Signal dom bwd
+ ) =>
+ Circuit a a
+forceResetSanityGeneric = Circuit go
+ where
+ go (fwd, bwd) =
+ C.unbundle $
+ C.mux
+ rstAsserted
+ (C.bundle (idleBwd $ Proxy @a, idleFwd $ Proxy @a))
+ (C.bundle (bwd, fwd))
+
+#if MIN_VERSION_clash_prelude(1,8,0)
+ rstAsserted = C.unsafeToActiveHigh C.hasReset
+#else
+ rstAsserted = C.unsafeToHighPolarity C.hasReset
+#endif
diff --git a/clash-protocols-base/src/Protocols/Internal/Classes.hs b/clash-protocols-base/src/Protocols/Internal/Classes.hs
index cba5a44..489a555 100644
--- a/clash-protocols-base/src/Protocols/Internal/Classes.hs
+++ b/clash-protocols-base/src/Protocols/Internal/Classes.hs
@@ -21,149 +21,9 @@ class Protocol a where
-- existence of 'Bwd'.
type Bwd (a :: Type)
-{- | A /Circuit/, in its most general form, corresponds to a component with two
-pairs of an input and output. As a diagram:
-
-@
- Circuit a b
-
- +-----------+
- Fwd a | | Fwd b
- +------->+ +-------->
- | |
- | |
- Bwd a | | Bwd b
- <--------+ +<-------+
- | |
- +-----------+
-@
-
-The first pair, @(Fwd a, Bwd a)@ can be thought of the data sent to and from
-the component on the left hand side of this circuit. For this pair, @Fwd a@
-is the data sent from the circuit on the left hand side (not pictured), while
-@Bwd a@ is the data sent to the left hand side from the current circuit.
-
-Similarly, the second pair, @(Fwd b, Bwd)@, can be thought of as the data
-sent to and from the right hand side of this circuit. In this case, @Fwd b@
-is the data sent from the current circuit to the one on the right hand side,
-while @Bwd b@ is the data received from the right hand side.
-
-In Haskell terms, we would say this is simply a function taking two inputs,
-@Fwd a@ and @Bwd b@, yielding a pair of outputs @Fwd b@ and @Bwd a@. This is
-in fact exactly its definition:
-
-@
- newtype Circuit a b =
- Circuit ( (Fwd a, Bwd b) -> (Bwd a, Fwd b) )
-@
-
-Note that the type parameters /a/ and /b/ don't directly correspond to the
-types of the inputs and outputs of this function. Instead, the type families
-@Fwd@ and @Bwd@ decide this. The type parameters can be thought of as
-deciders for what /protocol/ the left hand side and right hand side must
-speak.
-
-Let's make it a bit more concrete by building such a protocol. For this
-example, we'd like to build a protocol that sends data to a circuit, while
-allowing the circuit to signal whether it processed the sent data or not. Similarly,
-we'd like the sender to be able to indicate that it doesn't have any data to
-send. These kind of protocols fall under the umbrella of "dataflow" protocols,
-so lets call it /DataFlowSimple/ or /Df/ for short:
-
-@
- data Df (dom :: Domain) (a :: Type)
-@
-
-We're only going to use it on the type level, so we won't need any
-constructors for this datatype. The first type parameter indicates the
-synthesis domain the protocol will use. This is the same /dom/ as used in
-/Signal dom a/. The second type indicates what data the protocol needs to
-send. Again, this is similar to the /a/ in /Signal dom a/.
-
-As said previously, we'd like the sender to either send /no data/ or
-/some data/. We can capture this in a data type very similar to /Maybe/:
-
-@
- data Data a = NoData | Data a
-@
-
-On the way back, we'd like to either acknowledge or not acknowledge sent
-data. Similar to /Bool/ we define:
-
-@
- newtype Ack = Ack Bool
-@
-
-With these three definitions we're ready to make an instance for /Fwd/ and
-/Bwd/:
-
-@
-instance Protocol (Df dom a) where
- type Fwd (Df dom a) = Signal dom (Data a)
- type Bwd (Df dom a) = Signal dom Ack
-@
-
-Having defined all this, we can take a look at /Circuit/ once more: now
-instantiated with our types. The following:
-
-@
- f :: Circuit (Df dom a) (Df dom b)
-@
-
-..now corresponds to the following protocol:
-
-@
- +-----------+
- Signal dom (Data a) | | Signal dom (Data b)
- +------------------------>+ +------------------------->
- | |
- | |
- Signal dom Ack | | Signal dom Ack
- <-------------------------+ +<------------------------+
- | |
- +-----------+
-@
-
-There's a number of advantages over manually writing out these function
-types:
-
- 1. It reduces syntactical noise in type signatures
-
- 2. It eliminates the need for manually routing acknowledgement lines
--}
-newtype Circuit a b
- = Circuit ((Fwd a, Bwd b) -> (Bwd a, Fwd b))
-
{- | Idle state of a Circuit. Aims to provide no data for both the forward and
backward direction. Transactions are not acknowledged.
-}
class (Protocol p) => IdleCircuit p where
idleFwd :: Proxy p -> Fwd (p :: Type)
idleBwd :: Proxy p -> Bwd (p :: Type)
-
-{- | Force a /nack/ on the backward channel and /no data/ on the forward
-channel if reset is asserted.
--}
-forceResetSanityGeneric ::
- forall dom a fwd bwd.
- ( KnownDomain dom
- , HiddenReset dom
- , IdleCircuit a
- , Fwd a ~ Signal dom fwd
- , Bwd a ~ Signal dom bwd
- ) =>
- Circuit a a
-forceResetSanityGeneric = Circuit go
- where
- go (fwd, bwd) =
- unbundle $
- mux
- rstAsserted
- (bundle (idleBwd $ Proxy @a, idleFwd $ Proxy @a))
- (bundle (bwd, fwd))
-
-#if MIN_VERSION_clash_prelude(1,8,0)
- rstAsserted = unsafeToActiveHigh hasReset
-#else
- rstAsserted = unsafeToHighPolarity hasReset
-#endif
diff --git a/clash-protocols/src/Protocols/Wishbone.hs b/clash-protocols/src/Protocols/Wishbone.hs
index ac22ed7..78307e8 100644
--- a/clash-protocols/src/Protocols/Wishbone.hs
+++ b/clash-protocols/src/Protocols/Wishbone.hs
@@ -15,7 +15,7 @@ import Prelude hiding (head, not, (&&))
import Clash.Signal.Internal (Signal (..))
import Control.DeepSeq (NFData)
import Protocols
-import Protocols.Internal.Classes
+import Protocols.Internal
import qualified Clash.Prelude as C
|
|
I suspect there may be a mismatch between what different developers think the
Of course I made the rather dumb mistake of calling Now I come back a few months later and the module is {- |
These class definitions are needed to be able to write Template Haskell quotes
for instances. They are defined separately to avoid import loops.
This module is not exported; the classes and their (orphan) instances are
exported elsewhere.
-}Which apparently did not achieve what I hoped it would and is currently a lie, as the module is [edit] |
`clash-protocols-base` should only contain code and definitions related to the circuit plugin. This includes the `Circuit` definition and `Protocol` typeclass. Furthermore we include instances for types imported from underlying clash libraries such as tuples, `Vec` and `Signal`.
It not only contains classes, but also types
…e to separate modules
The separation between plugin related code and protocol related code should be obvious from the module hierarchy
- `unitsTupleInstances` - `taggedBundleTupleInstances` - `protocolTupleInstances`
lmbollen
force-pushed
the
lucas/move-classes
branch
from
3a4af6e
to
2a33d73
Compare
lmbollen
force-pushed
the
lucas/move-classes
branch
from
2a33d73
to
21ee825
Compare
DigitalBrains1
force-pushed
the
lucas/reorder-modules
branch
from
c304653
to
34d51cb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I think these also should have been moved in #74. This is preparation for template Haskell generators for tuple instances for
Simulate(and friends?)