feat: protocol simulation API; implemented for AHB

[numero-744]

The API is not protocol-dependent and can be implemented for other protocols as well (even Stream and Flow I guess, will have to try ;).

Notice how the way AHB is implemented matches the ARM specification. Same for its tests.

This is WIP, the API is not stabilized yet (aka function names can still change). Also, it is not possible to check if it works because other parts of the code do not build. (It was working before being integrated, I swear ;)

Discussion to have / things to change:

• What about AhbAttributes for master behavior? Should I use it for all transactions or there is a better way to do this?
• Slave "setOn" functions should be renamed to explicitly manage 3 types of arguments: reader (addr => data) and writer (addr, data => Unit); then Behavior (Req => Resp) and finally Builder (Req => () => resp, aka Req => Handler). Then implicit conversion Behavior => Builder should be removed.
• Should I add a class to just read signals and check that all "must" of the spec are applied or is it already a thing?
• AhbMaster is not implemented yet (it is almost the same, just one signal changes I think)
• Parts of tester/src/test/scala/spinal/tester/scalatest/lib/bus/amba3/ahblite/sim/hardware.scala are already implemented somewhere I guess.

[numero-744]

Also it would be good to have a "faster" await. Currently it returns one clock cycle too late.

[numero-744]

Lol I get a relevant error here, but it fails to compile earlier because of other parts of the code on my machine… https://github.com/SpinalHDL/SpinalHDL/actions/runs/3175845982/jobs/5174410648#step:4:244

EDIT: fixed

[andreasWallner]

Just my 0.02€ after having a quick look at 1AM... ;-)

I had a look over it and I have general thoughts: some part of this I really like, while others feel to me like they are trying to solve non-issues: One such thing is the machinery that you need to create the sequences of accesses, where there already is a way to do this: just fork and e.g. call Read and Write in the fork. To me that could also be nicer to debug in testcases since you can do stuff inbetween steps, e.g. println something. What also often comes up for me is that I need to apply some other stimulus to the module (e.g. a interface transfer or something).
Similar the way how you react to events on the slave side where you return the Handlers: most cases could also be handled with a callback that if needed waits for a few clocks and then returns with e.g. the resp to put onto the bus. I understand that this would might work as nicely for out-of-order responses, but I guess it should be possible to still have the callback waiting in that case as well.
I'd understand the overhead if it was possible to then write test for different bus interfaces - but I don't see this here (since you still need the specific verbs for the AHB bus)?

I also find the seperation that is there in the sim classes for Stream, Apb, Bmb into a driver/agent and a monitor to be quite advantageous - it makes writing testcases often much easier - could this split be implemented here as well?

I think thats enough negativity for 1am

[numero-744]

Creating sequences is useful for pipelined protocols like AHB, and to be able to describe the whole behavior once, it is useful to start transactions before the end of the previous one while managing HREADYOUT.

Similar the way how you react to events on the slave side where you return the Handlers: most cases could also be handled with a callback that if needed waits for a few clocks and then returns with e.g. the resp to put onto the bus

setOnReadsAfter? I'm not sure of my understanding.

I also find the seperation that is there in the sim classes for Stream, Apb, Bmb into a driver/agent and a monitor to be quite advantageous - it makes writing testcases often much easier - could this split be implemented here as well?

I don't know about it, will see that.

I think thats enough negativity for 1am

|-(

[Dolu1990]

Funny pattern XD

[numero-744]

Isn't it too unidiomatic?

[Dolu1990]

So, it is a bit too much i think ^^
As instead you can use case class and named argument :

case class AhbAttributes(
    val hburst: Option[Int] = None,
    val hmastlock: Option[Boolean] = None,
    val hprot: Option[Int] = None,
    val hsize: Option[Int] = None,
    val htrans: Option[Int] = None
) 

val x = AhbAttributes(hprot=Some(2))
val y = x.copy(hsize=Some(32), htrans=Some(64))

[numero-744]

Indeed I started with a case class, then I wanted to add def hburst(value) and it obviously did not work so I removed case, then I realized I wanted to access values by these names so I left things half done etc.

Now I've added withAttribute and put case class

[Dolu1990]

But overall, is the following a good pattern ?

def apply(attr: Hburst): AhbAttributes = copy(hburst = Some(attr))

I would say, it is fancy, but overall add quite some verbosity in the library + extra complexity. So i guess the best is to remove it ?

[numero-744]

Indeed, as there is implicit conversion from Hanything to Int, the withAttr would work anyway. Maybe I should also remove attribute classes which do not enable to build values… Yes, I'll definitely clean things here ^^'

[numero-744]

Actually I am still wondering (since I wrote the first commit) if gathering attributes in a struct is really useful? I mean, I the PR I show the version with it and without it, and sometimes parts of it are ignored, maybe I can just remove this class after all… And this notion is not in the protocol…

[Dolu1990]

Actually I am still wondering (since I wrote the first commit) if gathering attributes in a struct is really useful? I mean, I the PR I show the version with it and without it, and sometimes parts of it are ignored, maybe I can just remove this class after all… And this notion is not in the protocol…

Ahh that's very much usage dependent, as you feel it. i don't realy have an opinon on it. Just that sometime, keeping things more raw, isn't a bad thing ^^

[Dolu1990]

hmm maybe start instead of boot ?

[numero-744]

👍 Wilco

[numero-744]

Well actually, to me boot conveys the idea that I start this one and then it will start the next one etc. Start is just "I start this one". It is related to your other question about transfer vs. sequence. (All is related lol) What do you think about it?

[Dolu1990]

to me boot conveys the idea that I start this one and then it will start the next one etc.

Ahhh, this is usage dependent, is see.

so, in some ways, i'm not sure we should bake in the linked list to the Task, but more decouplate things, where a sequance would be a Task which would run multiple sub task sequancialy.

Maybe that the first question to be answer ^^

[numero-744]

Indeed I agree with splitting logic. (If it is easier to explain it is easier to maintain 😉) I am responding in main thread.

[Dolu1990]

What is the difference between a transfer and a sequance ?

[numero-744]

Interesting point for docs (maybe design too actually).

A transfer is a single transfer, and a sequence is several transfers set to start the next one when appropriate / ASAP.

I find it useful to describe master's behavior once (but you can still start another sequence later if you want), especially for pipelined protocols like AHB. For instance: (Write(A) >> Read(A)).boot() will yield in the waves (no wait states inserted by slave in this example):

* Address phase for Write
* Data phase for Write + Address phase for Read
* Data phase for Read
* Done

[Dolu1990]

Hmm, so, What's about changing the name of Transfer to Task ? i mean, that would be a bit more usage agnostic.

Also, maybe the support of Task agregation should not be backed in the Task class ? But more be like a Task specialisation :

class Task{
  def start ...
  def await ...
}

class TaskQueue extends Task{
  val tasks = Queue[Task]()
  ...
}

val t = TaskQueue(t1, t2, t3)
t  += t4

?

I'm not sure of any thing, just guessing XD

[numero-744]

What's about changing the name of Transfer to Task ?

I think it is a good point, for instance for I2C there would be elementary Tasks (START, ACK, STOP) that are composed to do actual transfers? On the other hand I am afraid the name is too generic and lacks an clear definition.

Also, maybe the support of Task aggregation should not be backed in the Task class ?

I would like that too, but I IMHO ideally only the current transfer knows when the next one can start, so I think splitting this way would require cyclic references. When I wrote this, I was inspired by a methodology where a C program is run in a core to drive a bus, so to imitate this (with a more lightweight approach), I make it possible to define the master's behavior once and then the other stuff, but maybe it is not what we want.

[Dolu1990]

On the other hand I am afraid the name is too generic and lacks an clear definition.

If clearer definition is required for some cases, we can always create children classes.
Also, staying abstract may allows to decouplate complexity, it may allows to get the feature primitives right, instead of baking in "superfluous" features

I would like that too, but I IMHO ideally only the current transfer knows when the next one can start, so I think splitting this way would require cyclic references.

cyclic references ? where ?

Also, what's about having a clean Task interface, and having a LinkedTask which would be close to your current implementation ?

[numero-744]

By too generic I mean it is not specific to the notion of protocol and the name could conflict for instance with, I don't know, a part of Spinal's phases, or something related to pipelines or anything. This is why I used Transfer (which is used in AHB spec, maybe in other protocols too?). But anyway everything is in a namespace/package here so it should not be an issue ^^'

To explain what I am thinking to with "cyclic references", let's use the almost-pseudo-code-looks-like-scala example below:

val a, b = new Task
val s = new Seq[Task](a, b)

Then, when a can start the next transfer, it does not have the information that it is b, but s has. So a has to communicate with s, hence a needs a reference to s while s has a reference to a. EDIT: Except if s manages everything? I mean s could update a then check its canStartNext to start b? I'll see if I can provide a safe API for that.

Spliting logic into Task vs LinkedTask LGTM too.

[Dolu1990]

This is why I used Transfer (which is used in AHB spec, maybe in other protocols too?)

Hmm i often have seen "Transaction" or else "Beat" (for burst oriented stuff)

EDIT: Except if s manages everything?

Exactly, logic of s would be some thing like :

while(queue.notEmpty){
  val t =  queue.head
  t.start()
  t.await()
  queue.pop()
}

[numero-744]

About driver vs. monitor, I have read the sim/bus/wishbone. Here when a software abstraction of hardware is built, the signal names are readers to signal values, to make it easy to write everything; and read actions have a result so the actual read value is accessible as the result of the read transfer.

Hence, about the draft implementation, we do not want to pop or delete tasks / transfers / transactions because in the end they contain the read value + useful debug information (for instance for AHB, the number of wait states inserted for this specific transfer, and the total duration of the transfer, in clock cycles).

I could use something like awaitCanStartNext but my current implementation of await yields one cycle too late, because it checks isDone on every cycle, before isDone is updated so it waits the cycle after isDone is raised, not the cycle when isDone is raised. Any idea for a better implementation?

I must admit that I was thinking "transaction" when I was writing the API first, then I mixed with "transfer" when implementing AHB, and finally I made things uniform with "transfer" 😁

Maybe "transaction" should be used, because it is more generic because it may not transfer actual data?

[Dolu1990]

@numero-744

I could use something like awaitCanStartNext but my current implementation of await yields one cycle too late, because it checks isDone on every cycle, before isDone is updated so it waits the cycle after isDone is raised, not the cycle when isDone is raised. Any idea for a better implementation?

Here is a self contained example (may need a few imports, note you need SpinalHDL upstream), it introduce abolutly zero superflu waits. :

object SimTasker extends App{

  def TaskBody[T](body : => T) : TaskBody[T] = new TaskBody(body)
  class TaskBody[T](body : => T) extends Task{
    var yielding : T = null.asInstanceOf[T]

    val mutex = SimMutex()
    mutex.lock()
    override def run() : Unit = {
      yielding = body
      mutex.unlock()
    }

    override def await(): Unit = mutex.await()

    def getValue() : T = {
      await()
      yielding
    }
  }


  class TaskQueue extends Task{
    val queue = mutable.Queue[Task]()
    val mutex = SimMutex()
    mutex.lock()

    override def run() = {
      while(queue.nonEmpty) queue.dequeue().run()
      mutex.unlock()
    }
    override def await(): Unit = mutex.await()
  }

  SimConfig.withWave.compile(new Component{
    val value = in UInt(8 bits)
    RegNext(value) //Dummy
  }).doSim{dut =>
    dut.clockDomain.forkStimulus(10)
    dut.value #= 0

    // Create tasks
    val taskA = TaskBody{
      dut.value #= 1
      dut.clockDomain.waitSampling()
      dut.value #= 2
      dut.clockDomain.waitSampling()
      println(s"taskA completion at time ${simTime}")
      "miaou"
    }
    val taskB = TaskBody{
      dut.value #= 3
      dut.clockDomain.waitSampling()
      dut.value #= 4
      println(s"taskB completion at time ${simTime}")
      "rawrr"
    }
    val taskC = TaskBody{
      dut.clockDomain.waitSampling()
      dut.value #= 5
      dut.clockDomain.waitSampling()
      dut.value #= 6
      println(s"taskC completion at time ${simTime}")
      "wuff"
    }

    //Create a sequance of tasks
    val tasks = new TaskQueue()
    tasks.queue ++= List(taskA, taskB, taskC)

    //Fork a thread which will print the result of taskA
    fork{
      println(s"taskA gived ${taskA.getValue()} at sim time ${simTime}")
    }

    // Start of the main thread active waits
    dut.clockDomain.waitSampling()
    tasks.run() //This is a blocking call
    println(s"taskB gived ${taskB.getValue()} (end of sim at  ${simTime})")
  }
}

Print :
taskA completion at time 190
taskA gived miaou at sim time 190
taskB completion at time 200
taskC completion at time 220
taskB gived rawrr (end of sim at 220)

Maybe "transaction" should be used, because it is more generic because it may not transfer actual data?

Hmmm transaction is in itself kind of specific, overall here we are describing code which has to run, so why not just Task or something neutral ?

[numero-744]

Thanks for the mutex thing, I'll use it for await.

After thinking to it, I do not want to use awaitReady in TaskSeq because it needs to add a thread and I prefer to have optimal design as far as it does not complexifies stuff.

Below is my current draft implementation (not tested on AHB) for the new API. There is the same principle of links with onReady but here it is just an action (sounds more flexible & neutral, removes the notion of sequence that is booted etc.), and is not to be used by most users (maybe not to be documented on RTD, only in scaladoc).

It has a defaultOnReady which should be defined by the implementor of a protocol, for instance to reset outputs to make simulation waves more readable, and is replaced by the wanted action (for instance start next task).

When I mentioned that a TaskSeq would update its sub-Tasks, I think it is not a good thing I think, because it has to know when to do so (clock sampling but maybe not for all protocols), therefore it puts restrictions that I do not want.

So there is the trait, and an object to build a Task from a sequence of tasks.

package spinal.lib.sim.protocolSim.master

object Task {
  def apply[T <: Task](ts: Seq[T]): TaskSeq[T] = new TaskSeq(ts)
}

trait Task {
  def start(): Unit

  final def run(): Unit = { start(); await() }

  def await(): Unit

  def isDone: Boolean

  val defaultOnReady: () => Unit

  protected var onReady: Option[() => Unit] = None

  protected final def beReady(): Unit = (onReady.getOrElse(defaultOnReady))()

  final def setOnReady(handler: => Unit): Unit = {
    assert(onReady.isEmpty, "setOnReady called twice on the same transaction")
    onReady = Some(() => handler)
  }

  final def unsetOnReady(): Unit = onReady = None
}

The definition of a sequence of tasks is below, with at quick test.

It is still possible to use TaskSeq as a Seq (safe because of immutability, can you confirm?) and as a Task, which makes it possible to use the sequence like a task (task composition we discussed earlier) while preventing user errors that were easy to do in the previous API with await vs awaitAll etc. This removes confusion between sequence of transfers and one transfer which are actually the same type that was in my previous API.

package spinal.lib.sim.protocolSim.master

class TaskSeq[T <: Task](seq: Seq[T]) extends Seq[T] with Task {

  def iterator: Iterator[T] = seq.iterator

  def length: Int = seq.length

  def apply(idx: Int): T = seq.apply(idx)

  def start() = head.start()

  def await() = last.await()

  def isDone: Boolean = last.isDone

  val defaultOnReady: () => Unit = last.defaultOnReady

  for (i <- 0 until length - 1)
    seq(i).setOnReady(seq(i + 1).start())
  last.setOnReady(beReady())
}

object Test extends App {
  case class Printer(text: String) extends Task {
    private var _isDone = false
    def start(): Unit = { println(text); _isDone = true; beReady() }

    def await(): Unit = assert(isDone)

    def isDone: Boolean = _isDone

    val defaultOnReady: () => Unit = { () => println("reset prints") }

    def specificStuff() = println("specific")
  }

  case class Printer2(n: Int) extends Task {
    private var _isDone = false

    def start(): Unit = { println(n); _isDone = true; beReady() }

    def await(): Unit = assert(isDone)

    def isDone: Boolean = _isDone

    val defaultOnReady: () => Unit = { () => println("reset prints") }
  }

  val ts = Task(Seq(Printer("Hello"), Printer("World")))
  val ts2 = Task(Seq(Printer("hello"), Printer2(5)))
  ts.setOnReady(println("done"))
  ts.run()
  ts.foreach { t => t.specificStuff() }
  ts2.run()
  //ts2.foreach { t => t.specificStuff() }
}

Notice that types are uniformized to the more specific type possible by Scala, which makes it possible to do specificStuff only in the first case. In case of mixed tasks, it is still possible to access methods of each task by their name. (For instance it will be possible to build a burst of reads and a burst of writes, then to put them in a TaskSeq and to access to read values by the name of the burst of reads because it only contains reads).

I did not want to use implicit class because Seq is to be converted only once, to chain actions.

[numero-744]

^ Comments appreciated before I migrate the whole thing to this new API

[Dolu1990]

It is still possible to use TaskSeq as a Seq (safe because of immutability, can you confirm?)

You don't have to botther about multi threading race condition, as SpinalHDL enforce things to always happen on the same core in a cooperative maner (coroutine like)

val defaultOnReady: () => Unit
protected var onReady: Option[() => Unit] = None
protected final def beReady(): Unit = (onReady.getOrElse(defaultOnReady))()
final def setOnReady(handler: => Unit): Unit = {
assert(onReady.isEmpty, "setOnReady called twice on the same transaction")
onReady = Some(() => handler)
}
final def unsetOnReady(): Unit = onReady = None

What is the purpose of those ?

Is var onReady a way to have some callback on the completion of a task ?
If yes, it should have been an ArrayBuffer[() => Unit] to allow multiple callback to coexist i would say ?

[numero-744]

If I do not update callbacks when the Seq is modified, the data structure becomes inconsistent. For instance if I add a transaction at the end, then it will be skipped, which is not the expected behavior. So I hope I cannot add a transaction at the end.

onReady is only to set a callback to restore state of bus (put 0 to make simulation waves easier to read) or trigger next task. For now it is checked to be set only once (to avoid a task to start two tasks which would imply kind of undefined behavior). I can:

• transform it into private var onReadyActions: ArrayBuffer[() => Unit]
• rename setOnReady to onReady
• remove unsetOnReady
• add same for onStart and onDone.

Okay for you?

[Dolu1990]

@numero-744

add same for onStart and onDone.

Those two seems good for me ^^

But the onReady realy seems like a hacky thing. To me it should be a task in itself or something.

More generaly, what kind of feature the PR want to add ?
An API to describe sequances in the context of directed tests (not much randomization, looking at AhbSlaveProtocolSpec.scala) ?
So writing testbench by specifying body of code to run, instead of specifying data to feed to some driver / monitor / scoreboard agents ?
I'm trying to figure out the context ^^