Move i2c to clash cores #2584

lmbollen · 2023-10-03T09:46:13Z

Currently the i2c core in the examples folder can not be used without copying the files or having a local clone of the reposity. This pull request moves the core over to clash-cores and aims to making it properly documented.

TODO:

There are a few things which I'd like added to this PR:

Add explanation of module in each header
Unify naming of the different acknowledge signals (and remove master/slave references)

I've created an issue for the changes which I think are needed before moving the I2C core out of its Experimental submodule.

DigitalBrains1 · 2023-10-06T14:52:36Z

clash-cores/test/Test/Cores/I2C.hs


-{-# ANN system Synthesize { t_name = "system", t_inputs = [], t_output = PortName "" } #-}
-system = system0 systemClockGen resetGen
+{-# ANN testBench Synthesize { t_name = "testBench", t_inputs = [], t_output = PortName "" } #-}


As discussed, some comments:

testBench is a magic name; I think it's better to give it a different name. The Synthesize annotation probably masks the special machinery.

The record syntax with a space between the constructor and the { is confusing, it looks like function application. How many arguments does ANN take? :-)

We didn't document this yet, but we did discuss steering people away from the constructor and to using defSyn instead. I think mainly so we can add fields without existing use sites suddenly having undefined field contents.

Ah forget about the defSyn that doesn't work... <sad trombone>

Also remove `Synthesize` annotations from `i2c` core and internals.

…binatorial Co-authored-by: lucas@qbaylogic.com

Hide the print statements of the I2C unittest behind debug flags. This debug flag is currently always False. TODO: Expose the debug flag of the I2C unittest

Since the I2C core is subject to changes in the near future, we've moved it to an `Experimental` submodule.

christiaanb

Aside from commit messages stating that specific changes were made, they should explain why the changes were made.

christiaanb · 2024-04-19T13:00:17Z

tests/Main.hs

@@ -158,24 +158,6 @@ runClashTest = defaultMain $ clashTestRoot
    , clashTestGroup "crc32"
        [ runTest "CRC32" def
        ]
-    , clashTestGroup "i2c"


The I2C test is:

The only test in the clash testsuite that has multiple synthesize annotations

Tests SimIO in a common user scenario.

So I would like it if this test wasn't removed. And given that the I2C core in clash cores has a new API, perhaps we can either just:

Keep the old domain-monomorphic code in the examples directory

Or move the old domain-monomorphic code to a test directory
So that we still have a test with multiple Synthesize annotations.

Sounds like the best option to me!

Also agree with 2, we can just put the old code in a test directory.

clash-cores/src/Clash/Cores/I2C/ByteMaster.hs

christiaanb · 2024-04-19T13:24:09Z

clash-cores/src/Clash/Cores/I2C/ByteMaster.hs

@@ -29,10 +29,8 @@ data ByteMasterS
  , _coreTxd    :: Bit              -- coreTxd register
  , _shiftsr    :: Bool             -- shift sr
  , _ld         :: Bool             -- load values in to sr
-  , _i2cOpAck    :: Bool             -- host cmd acknowlegde register


Why is the command ack made combinational? The commit message doesn't state why this change was made.

christiaanb · 2024-04-19T13:29:55Z

clash-cores/test/Test/Cores/I2C/Config.hs

-      -> do display "CONFaddrAck"
-            pure s { i2cConfStateM = CONFreg
-                   , i2cWrite = False
+      -> if rxAck then do


Are these changes made because the ack made combinational in the another commit? Perhaps mention that in the commit message then.

christiaanb · 2024-04-19T13:32:44Z

clash-cores/test/Test/Cores/I2C/Config.hs

@@ -22,8 +22,18 @@ data ConfS = ConfS { i2cConfStateM   :: ConfStateMachine
                   , i2cConfFault    :: Bool
                   }

-type ConfI = (Bool,Bool,Bool,Bool,Bool)
-type ConfO = (Bool,Maybe I2COperation,Bool,Bool)
+data ConfI = ConfI { i2cRst    :: Bool


The commit message just says "refactor", perhaps explain what the refactor does.

christiaanb · 2024-04-19T13:34:29Z

clash-cores/test/Test/Cores/I2C/Config.hs

@@ -49,7 +52,7 @@ configT
  -> SimIO ConfO
 configT s0 ConfI{i2cRst=rst,i2cEna=ena,i2cCmdAck=cmdAck,i2cRxAck=rxAck,i2cAl=al} = do
  s <- readReg s0
-  let ConfS confStateM claim op lutIndex fault = s
+  let ConfS confStateM claim op lutIndex fault debug = s


Why are debug messages made conditional? Perhaps that could be mentioned in the commit message.

Because otherwise they flood the terminal when you run clash-cores:unittests :)

christiaanb · 2024-04-19T13:38:24Z

clash-cores/src/Clash/Cores/Experimental/I2C.hs

+-- | Core for I2C communication. Returns the output enable signals for SCL en SDA
+-- These signals assume that when they are `high`, they pull down SCL and SDA respectively.
+-- For 2-wire I2C, you can use BiSignals (`Clash.Signal.Bidirectional.BiSignalIn` and `Clash.Signal.Bidirectional.BiSignalOut`)
+--
+-- === __Example__
+--
+-- @
+-- i2cComp clk rst ena sclIn sdaIn = (sclOut, sdaOut)
+--  where
+--   sclOut = writeToBiSignal sclIn (mux sclOe (pure $ Just 0) (pure Nothing))
+--   sdaOut = writeToBiSignal sdaIn (mux sdaOe (pure $ Just 0) (pure Nothing))
+--   (sclOe, sdaOe) = unbundle i2cO
+--   i2cIn = bundle (readFromBiSignal sclIn, readFromBiSignal sdaIn)
+--   (dout,i2cOpAck,busy,al,ackWrite,i2cOut) = i2c clk arst rst ena clkCnt claimBus i2cOp ackRead i2cI
+-- @


The outputs for SCL and SDA are Maybe Bit, neither the haddock comment, nor the code example seem to get this rights.

(I did not have a chance to actually look at this code and the changes; and I don't have time anymore to do so now. So I'm making a bunch of assumptions and next week I'll see if I was off base)

SCL and SDA are open-drain I/O pins: they are both input and output, and they are either driven low or high-impedance. I assume the Maybe Bit therefore has two states: Nothing (Hi-Z) and Just 0 (drive low). And I suppose this encoding was chosen because in an FPGA, that's how you would drive the tri-state used to implement an open-drain pin on the FPGA. Specifically, the trivial way to implement open-drain with a tri-state is to have the input from the FPGA tied to ground and put your data on the gate: gated when high, outputting when low.

So this Maybe Bit is purely pragmatic: it encodes how you drive a tristate, but it only has two states. It's also not entirely practical; we have two tri-state primitives, one for the ICE40 and one for the ECP5, and one of those uses an enable signal to gate, and the other a Signal Bool to gate; neither actually uses Maybe to gate.

So I'd like to propose we change the SCL and SDA outputs to just be Bit. And this would then encode the two possible states: 1 = Hi-Z, 0 = drive low, just like I²C defines it afaik.

And related but not for this PR, I'd like to harmonise our two tri-state primitives, and define wrappers that can be used to implement open-drain and open-source, where the data inputs to those wrappers are just Bit and they wrap the underlying tri-state primitives in the logical way (data input tied to rail, gate input tied to data, either inverted or direct). But that's for later; it just clarifies how I see the future for using this I²C core with I/O buffer primitives.

So this Maybe Bit is purely pragmatic: it encodes how you drive a tristate, but it only has two states

Indeed! It's to show that it drives a tristate.

It's also not entirely practical; we have two tri-state primitives, one for the ICE40 and one for the ECP5, and one of those uses an enable signal to gate, and the other a Signal Bool to gate; neither actually uses Maybe to gate.

I have never seen these primitives to be honest, the choice was made to be compatible with BiSignals. at least Vivado simply infers the tristate buffer when you use BiSignal. We don't use any explicit primitives in the Bittide implementations.

Personally I prefer the Maybe Bit over `Bit.

And related but not for this PR, I'd like to harmonise our two tri-state primitives, and define wrappers that can be used to implement open-drain and open-source, where the data inputs to those wrappers are just Bit and they wrap the underlying tri-state primitives in the logical way (data input tied to rail, gate input tied to data, either inverted or direct). But that's for later; it just clarifies how I see the future for using this I²C core with I/O buffer primitives.

Personally I don't find it intuitive to make the behavior depend on the outside world, whats the downside of using Maybe Bit to you?

Maybe Bit can do the wrong thing. You could output Just 1 which is wrong for i2c. I would like to propose a third option. Have a unit data type newtype Low = Low or something like that and output Maybe Low. If you want you can even make a custom BitPack so that it still implemented as 2 bits.

Indeed the core should never output Just 1, and it never does. I don't see a problem in using Maybe Bit as tristrate drive type for i2c. It intuitively hints to the user that its mean to be a tristate driver

at least Vivado simply infers the tristate buffer when you use BiSignal.

Oh that's good to know! I just immediately thought of those two primitives.

Thinking about it a bit more, a wrapper for a BiSignal is less pretty in API than I initially imagined it could be. But still I'm leaning towards it being the right way. It would also hide a bit of the complexity of BiSignal for this use.

We could add this to the Prelude:

openDrainIO :: BiSignalIn ds dom (BitSize a) -> Signal dom a -> (BiSignalOut ds dom (BitSize a), Signal dom a) openSourceIO :: BiSignalIn ds dom (BitSize a) -> Signal dom a -> (BiSignalOut ds dom (BitSize a), Signal dom a)

and people wouldn't have to do all the readFromBiSignal and writeToBiSignal themselves.

Personally I don't find it intuitive to make the behavior depend on the outside world, whats the downside of using Maybe Bit to you?

It's semantically wrong. It is a two-valued signal, not a three-valued one. If you want to force people towards a specific implementation, you should perhaps create a new type OpenDrain or something like that. So what's above becomes:

openDrainIO :: BiSignalIn ds dom (BitSize a) -> Signal dom (OpenDrain a) -> (BiSignalOut ds dom (BitSize a), Signal dom a)

(that's my counter-suggestion to Rowan's Low. I think it is more obvious in its intention.)

It intuitively hints to the user that its mean to be a tristate driver

In an FPGA, that is the usual implementation, yes, since it's there anyway. But it's not supposed to be a tri-state driver in my view. It's supposed to be an open-drain driver. But in an FPGA, every I/O pin is connected to a tri-state driver, and that's what you use. A standard totem-pole output is a tri-state that is never Hi-Z, an open-drain output is a tri-state that is never driven high, and an open-source output is a tri-state that is never driven low. Three types of outputs, all of them having two states. Only the true tri-state output has three states.

In an ASIC, you would never use a tri-state to implement an open drain, it would be wastage of die space on a staggering scale, relatively speaking.

Note that I would hint to shy away from BiSignal use in APIs currently because of #2168 and #1138

I don't intend to codify the BiSignal in the I²C stuff; people are free to choose their pin driver. But I think you either use platform-specific primitives or the BiSignal stuff. As I said, I don't mean to include these Prelude extensions as part of this PR, it's just how I envision it being used, which supports my argument in favour of just Bit.

lmbollen force-pushed the move-i2c-to-clash-cores branch 4 times, most recently from e1989c6 to 8553ddc Compare October 3, 2023 10:08

DigitalBrains1 reviewed Oct 6, 2023

View reviewed changes

lmbollen force-pushed the move-i2c-to-clash-cores branch 2 times, most recently from fb61cae to 2efc4a0 Compare October 11, 2023 13:53

lmbollen requested a review from christiaanb October 18, 2023 07:29

lmbollen force-pushed the move-i2c-to-clash-cores branch 2 times, most recently from bde9655 to f08ba82 Compare October 18, 2023 11:43

lmbollen force-pushed the move-i2c-to-clash-cores branch 5 times, most recently from 6af2276 to d663a1a Compare October 31, 2023 13:27

hiddemoll force-pushed the move-i2c-to-clash-cores branch 2 times, most recently from bfa702d to e9b217b Compare February 1, 2024 10:24

Move I2C example files to clash-cores

c35889b

hiddemoll force-pushed the move-i2c-to-clash-cores branch 4 times, most recently from 0ccc4ac to 41776f8 Compare April 11, 2024 10:37

lmbollen and others added 8 commits April 11, 2024 14:01

Fix warnings in i2c core.

9616fb2

Add comments to i2c core

14af81e

Make I2C core polymorphic in its domain

c4bb357

Remove HDL build test for I2C in examples

7bf0b2f

Also remove `Synthesize` annotations from `i2c` core and internals.

Refactor i2c core to be more user friendly

5e0368e

Add I2C unit test to clash-cores:unittests

a1ce13e

Respect boolean values of acknowledgements. Make acknowledgements com…

c13627b

…binatorial Co-authored-by: lucas@qbaylogic.com

Fix incorrect I2C test

98e806c

hiddemoll added 5 commits April 11, 2024 14:07

Refactor I2C core and unittest

28fdb7a

Add debug flag to I2C unittest

0d2cbb7

Hide the print statements of the I2C unittest behind debug flags. This debug flag is currently always False. TODO: Expose the debug flag of the I2C unittest

Add module headers to all moved I2C modules

ca4a097

Extend haddock documentation for I2C core

7676ced

Move I2C core to Experimental submodule

59cb3c7

Since the I2C core is subject to changes in the near future, we've moved it to an `Experimental` submodule.

hiddemoll force-pushed the move-i2c-to-clash-cores branch from 41776f8 to 59cb3c7 Compare April 11, 2024 15:27

hiddemoll marked this pull request as ready for review April 11, 2024 15:30

hiddemoll requested a review from DigitalBrains1 April 11, 2024 15:30

christiaanb requested changes Apr 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move i2c to clash cores #2584

Move i2c to clash cores #2584

lmbollen commented Oct 3, 2023 •

edited by hiddemoll

DigitalBrains1 Oct 6, 2023

DigitalBrains1 Oct 11, 2023

christiaanb left a comment

christiaanb Apr 19, 2024

DigitalBrains1 Apr 19, 2024

lmbollen Apr 22, 2024

christiaanb Apr 19, 2024

christiaanb Apr 19, 2024

christiaanb Apr 19, 2024

christiaanb Apr 19, 2024

lmbollen Apr 22, 2024

christiaanb Apr 19, 2024

DigitalBrains1 Apr 19, 2024

lmbollen Apr 22, 2024

rowanG077 Apr 22, 2024 •

edited

lmbollen Apr 22, 2024

DigitalBrains1 Apr 22, 2024

rowanG077 Apr 22, 2024 •

edited

DigitalBrains1 Apr 22, 2024

Move i2c to clash cores #2584

Are you sure you want to change the base?

Move i2c to clash cores #2584

Conversation

lmbollen commented Oct 3, 2023 • edited by hiddemoll

TODO:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christiaanb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanG077 Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanG077 Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmbollen commented Oct 3, 2023 •

edited by hiddemoll

rowanG077 Apr 22, 2024 •

edited

rowanG077 Apr 22, 2024 •

edited