Inout fix #1046

Dolu1990 · 2023-02-27T15:15:23Z

This PR reimplement how Analog are managed in order to allows much more flexibility and also fix existing issues.

Now, analog connections are tracked bit by bit (Island) then aggreged together for the final connection.
All the assignements between analogs are deleted and then regenerated into a solved manner.

This is kind of a big change XD

andreasWallner

First off: thanks for spending the effort on this!

Some good news first: I had a look at my initial issue - that is fixed and Vivado is also happy with the outcome.

I had a bit of a look on the code and noticed a few corner cases

In addition we have a small inconsistency that I did not want to hunt down today (too late ;-)):
We can leave all analog inputs of one signal unconnected

  case class Top() extends Component {
    val y = inout(Analog(Bits(2 bit)))
  }

But once we connect one of them, we have to all?

  case class Top() extends Component {
    val y = inout(Analog(Bits(2 bit)))
    y(0) := True
  }

core/src/main/scala/spinal/core/internals/Phase.scala

andreasWallner · 2023-03-01T00:38:11Z

core/src/main/scala/spinal/core/internals/Phase.scala

-    })
+        for(bitId <- 0 until widthOf(seed)){
+          val flushes = ArrayBuffer[(AggregateKey, AggregateCtx)]()
+          val island = bitToIsland(Bit(seed, bitId, c.dslBody))


Two issues here

First
Island building in 341 has a small issue in the sense that an assignment like y(0 downto 0) := sub.x works, while sub.x := y(0 downto 0) doesn't. I tried fixing that carrying along the analog flag when creating the extract, but that breaks if a user calls setAsAnalog after making the extract.

It's not per-se an issue if we have to call setAsAnalog on the extracts, but if one forgets we currently crash here (y(0) will never be added to any island since the target is not analog) with

Exception in thread "main" java.util.NoSuchElementException: key not found: Bit((toplevel/y : inout Bits[2 bits]),0,spinal.core.internals.ScopeStatement@ec38bef)

test code for that

case class Sub() extends Component { val x = inout(Analog(Bits(1 bit))) x(0) := True } case class Top() extends Component { val sub = Sub() val y = inout(Analog(Bits(2 bit))) sub.x := y(0 downto 0) y(1) := True }

We could make that a bit more understandable if we check here like:

val island = bitToIsland.get(Bit(seed, bitId, c.dslBody)) match { case Some(x) => x case None => SpinalError("DISCONTINUITY IN ANALOG CONNECTION ...") }

As far as I see it I think we can do either:

add in the error check from above (and possibly a explanation to RTD ;-))

extend all the extraction functions to carry along the analog attribute (and document the weird corner cases in RTD :-( )

I also tried extending the check in 328 to also build the island if the source is analog, but that creates a host of other problems...

So just bite the bullet and require .setAsAnalog()?

Edit: remove the "second" issue, that was due to some local changes...

The issue you had was that sub.x := y(0 downto 0).setAsAnalog() was required when you tested it.

Now that .setAsAnalog() isn't necessary anymore (today push) it is all good.

Can you confirm ? (as far as i can see)

Dolu1990 · 2023-03-01T14:14:16Z

First off: thanks for spending the effort on this!

Some good news first: I had a look at my initial issue - that is fixed and Vivado is also happy with the outcome.

I had a bit of a look on the code and noticed a few corner cases

In addition we have a small inconsistency that I did not want to hunt down today (too late ;-)): We can leave all analog inputs of one signal unconnected
  case class Top() extends Component {
    val y = inout(Analog(Bits(2 bit)))
  }
But once we connect one of them, we have to all?
  case class Top() extends Component {
    val y = inout(Analog(Bits(2 bit)))
    y(0) := True
  }

Fixed with 28636e8

…e update)

andreasWallner · 2023-03-01T23:31:04Z

Most of the issues are gone, but the changes now seem to have broken the "normal" generation of a driver for the inout (but only if the pad signal is multi-bit...):

    val x = new Component {
      val read = out Bool()
      val write = in Bool()
      val en = in Bool()
      val pad = inout(Analog(Bits(width bit)))

      for (i <- 0 until width) {
        read := pad(i)
        when(en) {
          pad(i) := write
        }
      }
    }

in which case the current code suspects that MULTIPLE INOUT interconnected in the same component

This time I pushed my test cases (currently w/o real verification) here: https://github.com/andreasWallner/SpinalHDL/tree/inout-fix

Dolu1990 · 2023-03-02T08:05:03Z

Most of the issues are gone, but the changes now seem to have broken the "normal" generation of a driver for the inout (but only if the pad signal is multi-bit...):
    val x = new Component {
      val read = out Bool()
      val write = in Bool()
      val en = in Bool()
      val pad = inout(Analog(Bits(width bit)))

      for (i <- 0 until width) {
        read := pad(i)
        when(en) {
          pad(i) := write
        }
      }
    }
in which case the current code suspects that MULTIPLE INOUT interconnected in the same component

This time I pushed my test cases (currently w/o real verification) here: https://github.com/andreasWallner/SpinalHDL/tree/inout-fix

Should be fixed with 56b956c

Note :

    val read = out Bits(width bit) //!!!
    val write = in Bool()
    val en = in Bool()
    val pad = inout(Analog(Bits(width bit)))

    for (i <- 0 until width) {
      read(i) := pad(i)
      when(en) {
        pad(i) := write
      }
    }

andreasWallner

Thanks for all the effort!

LGTM - should we also pull add a few testcases
I added a few here: https://github.com/andreasWallner/SpinalHDL/tree/inout-fix - while there is currently no simulation with them, but at least they are testing generation

Dolu1990 · 2023-03-07T01:26:08Z

should we also pull add a few testcases

It would be great, especialy corner cases.

It was my plan to add some, but currently i'm in a 2 week work trip, so realy busy, i realy lagging behind, trying to catch up emails XD

andreasWallner · 2023-03-19T18:20:24Z

I'm so sorry, writing tests it looks like I stumbled over another corner case:

object Blub extends App {
  SpinalVerilog(new Component {
    val pad = inout(Analog(Bits(2 bit)))
    val sliced_sub = new Component {
      val pad = inout(Analog(Bits(1 bit)))
    }
    pad(0 downto 0) <> sliced_sub.pad
  })
}

Which fails with

Static bits extraction (1 downto 0) is outside the range (0 downto 0) of (toplevel/sliced_sub/pad : inout Bits[1 bits]) at
    spinal.tester.scalatest.Blub$.delayedEndpoint$spinal$tester$scalatest$Blub$1(AnalogConnectionTester.scala:52)

Also: something seems to be weird with apply since pad.apply(0 downto 1) <> sliced_sub.pad is not cought early but trips over the dummy error in Phase.scala@326

Dolu1990 · 2023-03-20T17:22:26Z

I'm so sorry, writing tests it looks like I stumbled over another corner case:
object Blub extends App {
  SpinalVerilog(new Component {
    val pad = inout(Analog(Bits(2 bit)))
    val sliced_sub = new Component {
      val pad = inout(Analog(Bits(1 bit)))
    }
    pad(0 downto 0) <> sliced_sub.pad
  })
}
Which fails with
Static bits extraction (1 downto 0) is outside the range (0 downto 0) of (toplevel/sliced_sub/pad : inout Bits[1 bits]) at
    spinal.tester.scalatest.Blub$.delayedEndpoint$spinal$tester$scalatest$Blub$1(AnalogConnectionTester.scala:52)
Also: something seems to be weird with apply since pad.apply(0 downto 1) <> sliced_sub.pad is not cought early but trips over the dummy error in Phase.scala@326

Good catch ^^
Thanks ! I will take a look.

Dolu1990 · 2023-03-21T06:09:35Z

@andreasWallner Hi ^^
I pushed a fix for the two last issue you mentionned.

andreasWallner · 2023-03-23T00:23:31Z

@Dolu1990
Thanks!
I pushed my testcases - it would be quite interesting whether they work on e.g. gitlab CI, on my machine the VHDL stuff is doing weird things (i.e. a write & writeEnable are high and the pad does not get driven to 1 after a delay)
Verilog passes.

I initially tried to also test the direction from driving the pad, having writeEnable switched off and checking read - but there cocotb seems to do weird stuff (when trying to deposit/release on the inout signal). I left that testcase in for reference but I could not get it to do something useful, e.g. what cocotb reports as signal levels does not match what ends up in the vcd...

Add testcases for analog connections

Dolu1990 · 2023-03-24T11:33:42Z

@andreasWallner I pushed a thing to disable VHDL from AnalogConnectionCocotbBoot.
So, the VHDL fail on my machine too.
You think it's because of GHDL/Cocotb issue ? or SpinalHDL VHDL generation issue ?

andreasWallner · 2023-03-25T09:27:14Z

I had a look at the generated VHDL and that looks good to me... just to be sure I'll try testing it with xcelium in case I overlooked something, I should find some time to do that today.

Any deposit to a pad signal breaks the analog tests since these act as forces on the signal. Therefore driving 'z' fixes the pad level to 'z'. `Release` also does not improve the situation - so simply never touch those signals.

andreasWallner · 2023-03-26T15:38:41Z

@Dolu1990 I had a look - it looks like the issue with the original test cases was that writing the pad signals seems to work like a force, overriding the signal fully, Release also did not help there. I now just removed all the writes to xxx_pad - now they pass...
Working tests are in #1065

Fix cocotb test cases

Dolu1990 · 2023-03-26T18:11:32Z

Nice :D
So i think all is ready to merge right ?

andreasWallner · 2023-03-26T21:14:05Z

@Dolu1990 sounds good to me!

Dolu1990 added 7 commits February 23, 2023 23:25

wip

5d04532

wip

db6312d

wip

a4d2faf

wip now bit per bit

f6013f5

Seems mostly ok

66d48d6

good wip

96a5b9c

Analog management rewrite done ?

ce6c4b5

andreasWallner requested changes Mar 1, 2023

View reviewed changes

Allow analog seed to not be fully assigned

28636e8

Dolu1990 added 4 commits March 1, 2023 15:15

Fix typo

cf481b0

Analog phase catch unsupported bitvector access

ce46e8b

analog BitVector(range) now behave like a analog itself (as before th…

c30aeba

…e update)

Add analog width missmatch assertion

f05b2bf

analog phase, prevent digital drivers to create island merges

56b956c

andreasWallner approved these changes Mar 5, 2023

View reviewed changes

Dolu1990 added 2 commits March 21, 2023 06:58

fix 1046 partially assigned analog seeds genereration

88cec7d

fix 1046 zero width analog assignement

134faeb

Add testcases for analog connections

303735f

andreasWallner mentioned this pull request Mar 23, 2023

Add testcases for analog connections #1062

Merged

Dolu1990 added 2 commits March 23, 2023 17:50

Merge pull request #1062 from andreasWallner/inout-fix

d0e2a8f

Add testcases for analog connections

Disable AnalogConnectionCocotbBoot VHDL

6c7268d

Fix cocotb test cases

45dad01

Any deposit to a pad signal breaks the analog tests since these act as forces on the signal. Therefore driving 'z' fixes the pad level to 'z'. `Release` also does not improve the situation - so simply never touch those signals.

Merge pull request #1065 from andreasWallner/inout-fix

9410fb8

Fix cocotb test cases

Dolu1990 merged commit 601f0e8 into dev Mar 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inout fix #1046

Inout fix #1046

Dolu1990 commented Feb 27, 2023

andreasWallner left a comment

andreasWallner Mar 1, 2023 •

edited

Dolu1990 Mar 1, 2023

Dolu1990 commented Mar 1, 2023

andreasWallner commented Mar 1, 2023

Dolu1990 commented Mar 2, 2023

andreasWallner left a comment •

edited

Dolu1990 commented Mar 7, 2023

andreasWallner commented Mar 19, 2023

Dolu1990 commented Mar 20, 2023

Dolu1990 commented Mar 21, 2023

andreasWallner commented Mar 23, 2023

Dolu1990 commented Mar 24, 2023

andreasWallner commented Mar 25, 2023 •

edited

andreasWallner commented Mar 26, 2023 •

edited

Dolu1990 commented Mar 26, 2023

andreasWallner commented Mar 26, 2023

Inout fix #1046

Inout fix #1046

Conversation

Dolu1990 commented Feb 27, 2023

andreasWallner left a comment

Choose a reason for hiding this comment

andreasWallner Mar 1, 2023 • edited

Choose a reason for hiding this comment

Dolu1990 Mar 1, 2023

Choose a reason for hiding this comment

Dolu1990 commented Mar 1, 2023

andreasWallner commented Mar 1, 2023

Dolu1990 commented Mar 2, 2023

andreasWallner left a comment • edited

Choose a reason for hiding this comment

Dolu1990 commented Mar 7, 2023

andreasWallner commented Mar 19, 2023

Dolu1990 commented Mar 20, 2023

Dolu1990 commented Mar 21, 2023

andreasWallner commented Mar 23, 2023

Dolu1990 commented Mar 24, 2023

andreasWallner commented Mar 25, 2023 • edited

andreasWallner commented Mar 26, 2023 • edited

Dolu1990 commented Mar 26, 2023

andreasWallner commented Mar 26, 2023

andreasWallner Mar 1, 2023 •

edited

andreasWallner left a comment •

edited

andreasWallner commented Mar 25, 2023 •

edited

andreasWallner commented Mar 26, 2023 •

edited