Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate diagrams based on SpinalHDL code #1079

Merged
merged 21 commits into from
Aug 15, 2023

Conversation

ZhaokunHu
Copy link
Contributor

@ZhaokunHu ZhaokunHu commented Apr 8, 2023

Context, Motivation & Description

This pull request attempts to implement the idea #635 to automatically generate diagrams based on SpinalHDL code. I have created a new function called GenerateDiagrams that can be used in the following way:

HDElkDiagramGen(SpinalVerilog(new _))

This function will generate a new file named _.html .
If you have an internet connection, you can simply open the HTML file and the dependencies will be loaded automatically from the specified URLs. You can see several diagrams in the pages.
These diagrams are generated by the HDElk tool.(https://github.com/davidthings/hdelk ).
If you do not have an internet connection, you need to download the JS packages from this library and place them in the same folder as your HTML file. Then, according to the readme of this library, you need to add the dependencies to your HTML file. Then you can open the HTML file to see the diagrams.

Example:

import spinal.core._
import spinal.lib._

case class FilterConfig(iqWidth: Int,
                        tapNumbers: Int = 33,
                        hwFreq: HertzNumber = 200 MHz,
                        sampleFreq: HertzNumber = 1.92 MHz)

class Filter(fc: FilterConfig) extends Component{
  val din   = slave Flow(Bits(32 bits))
  val dout  = master Flow(Bits(32 bits))
  val flush = in Bool()

  val clockSMP = ClockDomain.external("smp")
  val clockHW = ClockDomain.external("hw")

  val u_fifo_in = StreamFifoCC(
    dataType = Bits(32 bits),
    depth = 8,
    pushClock = clockSMP,
    popClock = clockDomain
  )

  u_fifo_in.io.push << din.toStream
  dout << u_fifo_in.io.pop.toFlow

}

object Filter {
  def main(args: Array[String]): Unit = {
    val fc = FilterConfig(8)
    HDElkDiagramGen(SpinalVerilog(new Filter(fc)))
  }
}

It will generate this diagram:
image
I have also tested the GenerateDiagrams class on some more complex SpinalHDL projects to ensure the accuracy of the generated diagrams.

One issue that currently exists is that if you use many complex logic operations in your project, the system will automatically generate many registers to implement these operations, and the generated diagrams may become messy as a result.I have tried various methods to simplify the generated diagrams, but the results have been less than satisfactory.Please let me know if you have any thoughts or suggestions on this approach.

Impact on code generation

None

Checklist

  • Unit tests were added
  • API changes are or will be documented:

@ZhaokunHu ZhaokunHu closed this Apr 8, 2023
@ZhaokunHu ZhaokunHu changed the title Add files via upload Generate diagrams based on SpinalHDL code Apr 8, 2023
@ZhaokunHu ZhaokunHu reopened this Apr 8, 2023
@ZhaokunHu ZhaokunHu closed this Apr 8, 2023
@ZhaokunHu ZhaokunHu reopened this Apr 8, 2023
@Readon
Copy link
Collaborator

Readon commented Apr 8, 2023

Good work, Zhaokun.
However, there are still some points could be improved.

  1. Would it better to put the file into directory "lib/src/main/scala/spinal/lib/eda/", which represent it is a tool for eda like funciton.
  2. About the file name GenerateDiagram is really a board topic, how about HDElkDiagramGen or something more specifical, so that diagram generator using other tech could be named properly.
  3. there are still lint errors reported which have been give out at file list. You can run the check locally on your machine first. :)
  4. The function dealAllSignals is really too huge to review and understand. Simplify it would help to improve the quality of code.
  5. On the other hand, the function name dealAllSignals is really vague and it would be better to have a specific name.
  6. It would be nice if you could provide some examples of how to use it as a simulation in the tester subpackage.

@kleinai
Copy link
Collaborator

kleinai commented Apr 8, 2023

This is impressive!

I agree this should be under lib/eda or lib/tools which is where ModuleAnalyzer is.

dealAllSignals could be several smaller functions to make understanding the flow better. It looks like a candidate for abstracting so that other diagram backends could use it. Not that it has to happen in this PR.

Small suggestion, if the design has a fixed frequency maybe we could put that on the diagram too?

@Readon
Copy link
Collaborator

Readon commented Apr 8, 2023

This is impressive!

I agree this should be under lib/eda or lib/tools which is where ModuleAnalyzer is.

dealAllSignals could be several smaller functions to make understanding the flow better. It looks like a candidate for abstracting so that other diagram backends could use it. Not that it has to happen in this PR.

Small suggestion, if the design has a fixed frequency maybe we could put that on the diagram too?

lib/tools would be better.

@Readon
Copy link
Collaborator

Readon commented Apr 8, 2023

One more thing, as we talked before, I think the clk is not necessarily connect visibly. There is already clockdomain color, though.

Copy link
Collaborator

@andreasWallner andreasWallner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks quite promising, I added a few small comments below that I stumbled over when trying it out/skimming over it.

Apart from that:
I played around a bit, and e.g. this code:

case class Inner() extends Component {
  val io = new Bundle {
    val i = in port Bool()
    val o = out port Bool()
  }
  io.o := RegNext(io.i)
}

will generate this image:
image
with the output of the register not being connected.

Also this:

case class DiagramDemo() extends Component {
  val io = new Bundle {
    val i = in port Bool()
    val i2 = in port Bool()
    val sel = in port Bool()
    val o = out port Bool()

    val i2b = in port Bits(2 bit)
    val muxout = out port Bool()

    val regout = out port Bool()
  }
  io.muxout := io.i2b.mux(
    0 -> (io.i & io.i2),
    1 -> (io.i | io.i2),
    2 -> (io.i ^ io.i2),
    3 -> (io.i)
  )
  io.regout := RegNextWhen(io.i, io.sel)
  io.o := False
  when(io.i) { io.o := True }
}

generates
image
Where we see a similar thing with the outputs not being connected, and the assignments to io.o missing completely.

Nitpick: The blue on blue colorscheme of the examples above is kind of hard to read if you ask me.

PS: please also run scalafmt on the code.

lib/src/main/scala/spinal/lib/sim/GenerateDiagram.scala Outdated Show resolved Hide resolved
|<html>
|<head>
| <meta charset="UTF-8">
| <title>RTL连接图</title>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep with the remainder of SpinalHDL and use an English title

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorrry,i forgot to change it.I have changed it to "RTL diagrams based on SpinalHDL code"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's better, we could also use the component name of the top component in the title e.g. f"{rtl.toplevelName} RTL diagrams"

lib/src/main/scala/spinal/lib/sim/GenerateDiagram.scala Outdated Show resolved Hide resolved
val fileName = rtl.toplevelName + ".html"
val file = new File(fileName)
val pw = new FileWriter(file)
val builder = new StringBuilder()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a writer already, why not use that directly (and also pass it to the sub-components for use there?) There is no need to build the string fully in memory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it based on your advice.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I explained poorly: What I meant was to use the writer directly, without the StringBuilder. The StringBuilder will piece everything together in RAM, whereas if you use the writer it can be flushed to disk. But you'd have to pass the writer to GenerateOneDiagram.beginDraw(writer) to do that.
(and if you want/need the output in a String you can still use StringWriter if you use the base Writer as the parameter type)

@ZhaokunHu
Copy link
Contributor Author

ZhaokunHu commented Apr 9, 2023

This looks quite promising, I added a few small comments below that I stumbled over when trying it out/skimming over it.

Apart from that: I played around a bit, and e.g. this code:

case class Inner() extends Component {
  val io = new Bundle {
    val i = in port Bool()
    val o = out port Bool()
  }
  io.o := RegNext(io.i)
}

will generate this image: image with the output of the register not being connected.

Also this:

case class DiagramDemo() extends Component {
  val io = new Bundle {
    val i = in port Bool()
    val i2 = in port Bool()
    val sel = in port Bool()
    val o = out port Bool()

    val i2b = in port Bits(2 bit)
    val muxout = out port Bool()

    val regout = out port Bool()
  }
  io.muxout := io.i2b.mux(
    0 -> (io.i & io.i2),
    1 -> (io.i | io.i2),
    2 -> (io.i ^ io.i2),
    3 -> (io.i)
  )
  io.regout := RegNextWhen(io.i, io.sel)
  io.o := False
  when(io.i) { io.o := True }
}

generates image Where we see a similar thing with the outputs not being connected, and the assignments to io.o missing completely.

Nitpick: The blue on blue colorscheme of the examples above is kind of hard to read if you ask me.

PS: please also run scalafmt on the code.

I'm sorry for this silly bug. I accidentally deleted a pair of {} while removing the extra ones, which caused the program to produce incorrect results.I have fixed it and now the result is correct now.
image
image
I am a beginner in using Scala and there are some usages that I am not familiar with, which causes issues in my code writing. Thank you for your advice, I will carefully modify my program according to your suggestions.

@ZhaokunHu
Copy link
Contributor Author

ZhaokunHu commented Apr 9, 2023

I have made modifications to the program and fixed some problems based on your suggestions .And I placed it in the spinal.lib.tools package. I have also added a test file which will generate an .html file.
I also add CDN link of HDElk to the HTML file so You won't have to place the js package in your project .You can simply just use your browser to open the HTML file and see the diagrams.

@Readon
Copy link
Collaborator

Readon commented Apr 9, 2023

I have made modifications to the program and fixed some problems based on your suggestions .And I placed it in the spinal.lib.tools package. I have also added a test file which will generate an .html file. You will need to place the js package in https://github.com/davidthings/hdelk at the same directory as the HTML file, and then you can open the HTML file using a web browser.

This is an environment requirement. Just like we should install Quartus before we use Qsysnify.
As a library, SpinalHDL can only passively detect the environment but change it.
So it would be a way to have an environment variable "HDELK_PATH" which save the directory of required js files, thus it can be changed by user.

It's default value can be set as "https://github.com/davidthings/hdelk", so that if user is online no manual copy is required.
Or if we push the js file into SpinalHDL library, the default link could be "https://rawcontents.github.com/SpinalHDL/SpinalHDL/xxxxxx/hdelk".

| })
| }
|</script>
|<script src="https://cdn.jsdelivr.net/gh/davidthings/hdelk@master/js/elk.bundled.js"></script>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a default URL base, however, for users do not have network access might want to change this.
So use a variable is better here.:)

@andreasWallner
Copy link
Collaborator

andreasWallner commented Apr 10, 2023

I am a beginner in using Scala and there are some usages that I am not familiar with, which causes issues in my code writing. Thank you for your advice, I will carefully modify my program according to your suggestions.

No worries - stuff like that happens ;-)

I tested the latest version - the CDN links are much more comfortable than the local ones, but I agree with @Readon that it would be nice to have to option of passing an alternative path as a configuration.

I played around a bit more and stumbled over another small thing that's unexpected - it's cool that bundles are shown as a single signal, but accessing a signal from a bundle does not seem to get rendered/rendered incorrectly (test should be connected directly like testR is now, and testR should be connected to the FF?):

case class ElkDemo() extends Component {
  val io = new Bundle {
    val axii = slave port Axi4(Axi4Config(addressWidth=32, dataWidth=32, useId=false))
    val axio = master port Axi4(Axi4Config(addressWidth=32, dataWidth=32, useId=false))

    val test = out port Bool()
    val testR = out port Bool() setAsReg()
  }
  io.axii >> io.axio
  io.test := io.axii.r.valid
  io.testR := io.axii.w.valid
}

produces
image

add a new edge label.
fix a bug.
@ZhaokunHu
Copy link
Contributor Author

ZhaokunHu commented Apr 10, 2023

I am a beginner in using Scala and there are some usages that I am not familiar with, which causes issues in my code writing. Thank you for your advice, I will carefully modify my program according to your suggestions.

No worries - stuff like that happens ;-)

I tested the latest version - the CDN links are much more comfortable than the local ones, but I agree with @Readon that it would be nice to have to option of passing an alternative path as a configuration.

I played around a bit more and stumbled over another small thing that's unexpected - it's cool that bundles are shown as a single signal, but accessing a signal from a bundle does not seem to get rendered/rendered incorrectly (test should be connected directly like testR is now, and testR should be connected to the FF?):

case class ElkDemo() extends Component {
  val io = new Bundle {
    val axii = slave port Axi4(Axi4Config(addressWidth=32, dataWidth=32, useId=false))
    val axio = master port Axi4(Axi4Config(addressWidth=32, dataWidth=32, useId=false))

    val test = out port Bool()
    val testR = out port Bool() setAsReg()
  }
  io.axii >> io.axio
  io.test := io.axii.r.valid
  io.testR := io.axii.w.valid
}

produces image

Thank you so much for testing for me! I have solved one bug. As for the signal testR,it is a little bit complex.Please give me some time to solve it.
image

at the latest version:

  1. Add a new label for the edges which connect bus to one signal.
  2. Using the writer instead of builder.
  3. Change the page name to RTL diagrams of ${rtl.toplevelName}.

add colors to ports.
@ZhaokunHu
Copy link
Contributor Author

Now the situation where the output port is also used as a register can be displayed correctly.
image

What's more, the color of the port has been added.
image

@Readon
Copy link
Collaborator

Readon commented Apr 11, 2023

Now the situation where the output port is also used as a register can be displayed correctly. image

What's more, the color of the port has been added. image

Then, it's time to hide the clk & reset port, isn't it? :)

@Readon
Copy link
Collaborator

Readon commented Apr 11, 2023

Maybe it would be good to use black as default node color, and just distinguish the clock domains by port color only?

@wswslzp
Copy link
Contributor

wswslzp commented Apr 11, 2023

Great job! It's really pleasant to see the first circuit diagram generator application building upon ModuleAnalyzer/DataAnalyzer (my apology for still not having doc for them).

Things so far look good to me but still, I have a little advice. From an architectural point of view, the generator is tightly coupled with the SpinalHDL internal data model. The process of data transformation is like SpinalHDL data model -> Elk HTML format. A better way to make things more scalable or extensible is to add a middle layer, such as SpinalHDL data model -> JSON -> Elk HTML. The middle JSON format serves as a module interface database that holds all the necessary information for the generator to draw the diagram.

There will be two benefits if a middle layer is introduced.

  1. the generated interface database (e.g. a JSON file) can be reused by other tools which also need to elaborate the topology of the module.
  2. your diagram generator can accept a universal format, not only for SpinalHDL. And this can be a more valuable open-source project.

Though, it's just a piece of advice and requires time and effort to reconstruct the project (I can see that it's nearly finished and not the early stage of the project). Thus, you can ignore my opinion and keep working. I'm very much looking forward to seeing the final product :)

@Readon
Copy link
Collaborator

Readon commented Apr 11, 2023

Great job! It's really pleasant to see the first circuit diagram generator application building upon ModuleAnalyzer/DataAnalyzer (my apology for still not having doc for them).

Things so far look good to me but still, I have a little advice. From an architectural point of view, the generator is tightly coupled with the SpinalHDL internal data model. The process of data transformation is like SpinalHDL data model -> Elk HTML format. A better way to make things more scalable or extensible is to add a middle layer, such as SpinalHDL data model -> JSON -> Elk HTML. The middle JSON format serves as a module interface database that holds all the necessary information for the generator to draw the diagram.

There will be two benefits if a middle layer is introduced.

  1. the generated interface database (e.g. a JSON file) can be reused by other tools which also need to elaborate the topology of the module.
  2. your diagram generator can accept a universal format, not only for SpinalHDL. And this can be a more valuable open-source project.

Though, it's just a piece of advice and requires time and effort to reconstruct the project (I can see that it's nearly finished and not the early stage of the project). Thus, you can ignore my opinion and keep working. I'm very much looking forward to seeing the final product :)

Good idea!
Unfortunately, in this stage, it would be hard to archieve that.

  1. Hdelk need to be modified to support json load function, whoes difficulty is still not investigated.
  2. Zhaokun has a tight schedule on his under graduation report, so that it's not able to support a huge modifications now.
    Maybe we could do that later.

There are still problems to be solved, as soon as we tried to draw diagram for Spinal cord or um, whose top level also contains amount of logic inside.

@ZhaokunHu
Copy link
Contributor Author

Great job! It's really pleasant to see the first circuit diagram generator application building upon ModuleAnalyzer/DataAnalyzer (my apology for still not having doc for them).

Things so far look good to me but still, I have a little advice. From an architectural point of view, the generator is tightly coupled with the SpinalHDL internal data model. The process of data transformation is like SpinalHDL data model -> Elk HTML format. A better way to make things more scalable or extensible is to add a middle layer, such as SpinalHDL data model -> JSON -> Elk HTML. The middle JSON format serves as a module interface database that holds all the necessary information for the generator to draw the diagram.

There will be two benefits if a middle layer is introduced.

  1. the generated interface database (e.g. a JSON file) can be reused by other tools which also need to elaborate the topology of the module.
  2. your diagram generator can accept a universal format, not only for SpinalHDL. And this can be a more valuable open-source project.

Though, it's just a piece of advice and requires time and effort to reconstruct the project (I can see that it's nearly finished and not the early stage of the project). Thus, you can ignore my opinion and keep working. I'm very much looking forward to seeing the final product :)

I appreciate your PR very much. Without the ModuleAnalyzer/DataAnalyzer , this project would have been very difficult.

However, since this project relies heavily on the internal structure of SpinalHDL, it would be difficult to reconstruct to support a universal format. Additionally, I am not yet familiar enough with JSON and Scala, so I may not be able to complete the reconstruction. I apologize for not being able to accept your suggestion.

@@ -0,0 +1,552 @@
package spinal.lib.tools


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should run scalafmt on this code to avoid fmt-lint reports.

/** Initializing data */
private val fileName = topLevelName + ".html"
private val file = new File(fileName)
private val pw = new FileWriter(file, true)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pw could be named as writer?

for (topInOut <- topInOuts) {
if (haveParent(topInOut)) {
val rootParent = findParent(topInOut)
if (rootParent.flatten.head.isInput) topNode.inPorts.add(ElkPort(rootParent.getName(),findPortHighlight(topInOut)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about use pattern match here?

else if (allClk.size == 1) topNode.highlight = clkMap(allClk.head.toString())
}

private def GenAllNodes(): Unit = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should use a hierarchy class architecture to help to clean this logic.

}

private def GenAllEdges(): Unit = {
/** Getting the sonName of net */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really too big to look over.

pw.write(s"""{id:"${thisNode.labelName}",\n""")
if (thisNode.typeName != "") pw.write(s"""type:"${thisNode.typeName}",\n""")
if (thisNode.highlight != 0) pw.write(s"""highlight:${thisNode.highlight},\n""")
if (thisNode.inPorts.nonEmpty) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about moving this logic into port specific class? so all related logic can be put into that.
Above code seems identical while it is in or out.

Copy link
Collaborator

@andreasWallner andreasWallner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments of stuff that I observed. Most are suggestions that I think would make the code easier to read, some are small issues.
Keep up the good work!

val parentList = thisSon.getRefOwnersChain()
var returnParent = thisSon.parent
val loop = new Breaks
loop.breakable {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using breakable I'd could just return when you have found your parent, faster and IMHO easier to read

If you want to keep the breakable there is no need for the loop object in this case (since the breakable is not nested), you can

import Breaks.{breakable, break}
breakable {
  // ...
  break
  // ...
}

}

private def haveRegParent(thisSon: BaseType): Boolean = {
var judge = false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, simple return true when you have found your answer - simpler to read and faster since it would stop iterating as soon as you know there is a Data
On another note: The function is called haveRegParent indicating that you are looking for a register, but you are never checking that the Data is a register?

val allClk = moduleAnalyze.getClocks
var clkCounter = 1
for (thisClk <- allClk) {
if (!clkMap.contains(thisClk.clock.getName())) {
Copy link
Collaborator

@andreasWallner andreasWallner Apr 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason why you are using the names in the dictionary instead of the signals themselves? Two CDs can actually have the same name...
Nitpick: clkMap is maybe a little bit misleading since you also add reset and softReset signals to the map

Using the name of the clock lead to this diagram:
image

For this code:

case class DiagramDemo() extends Component {
  val io = new Bundle {
    val i = in port Bool()
    val subo = out port Bool()
    val subo2 = out port Bool()
  }
  val sub = new Component {
    val i = in port Bool()
    val o = out port Bool()

    val myClockDomain = ClockDomain.external("clk")
    val coreArea = new ClockingArea(myClockDomain) {
      o := RegNext(i)
    }
  }
  val sub2 = new Component {
    val i = in port Bool()
    val o = out port Bool()

    val myClockDomain = ClockDomain.external("clk")
    val coreArea = new ClockingArea(myClockDomain) {
      o := RegNext(i)
    }
  }
  sub.i := io.i
  io.subo := sub.o
  sub2.i := io.i
  io.subo2 := sub2.o
}

In reality this can happen when reusing multiple modules (that's why spinal internally only uses the name of the clock as a weak name, in the HDL the two are named clk_clk and clk_clk_1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your reviews .They are very helpful .I have already made the changes for the other reviews, but I encountered some issues that I couldn't solve when dealing with this particular review. I was unable to obtain the necessary information to distinguish between two clock domains with the same name, so I have submitted an issue. #1090

private val allInOuts = moduleAnalyze.getPins(_ => true)
private val everyRegisters = allRegisters ++ systemRegisters

private def haveParent(thisSon: BaseType): Boolean = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private def haveParent(thisSon: BaseType): Boolean = {
private def hasParent(thisSon: BaseType): Boolean = {

/** Generating the color mapping for clk */
private def GenColorMap(): Unit = {
val allClk = moduleAnalyze.getClocks
var clkCounter = 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you could skip counting here and use val clkCounter = clkMap.size + 2 inside the loop

}

for (topInOut <- topInOuts) {
if (haveParent(topInOut)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code itself
If I understand correctly you are using findParent to find the bundle a signal is part of? And then after flattening it check the direction to see if the bundle is an input or output? If so then this code won't reliably work for IMasterSlave bundles since you are not guaranteed that head will reflect whether the bundle in a master (=output) or slave (=input).

case class Example() extends Bundle with IMasterSlave {
  val aa = Bool()
  val bb = Bool()
  def asMaster() = {
    in(aa)
    out(bb)
  }
}
case class DiagramDemo() extends Component {
  val io = new Bundle {
    val slave_port = slave port Example()
    val master_port = master port Example()
  }
  io.master_port <> io.slave_port
}

generates:
image
(if it does not show for you, just swap in/out: out(aa) and in(bb))

repetition
The same (?) code is used just a few lines further down (176). Make a function that you call twice.

alternative implementation
I have observed that every time you use haveParent you also use findParent.
Consider changing findParent to return an Option[Data] and None if there is no parent. That would remove the need to loop through the signals twice.
Here you could then match...

returnParent = dataParent
loop.break()
} else {
dataParent.setName(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diagrammer should not modify that data model of the HDL code.

}

/** Integrating all methods to draw an image */
def beginDraw(): Unit = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should pass the writer as a parameter instead of opening it for append during construction and closing it here.

}

/** Generating HDElk language for Node */
private def drawNodes(thisNode: ElkNode): Unit = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this code into the ElkNode

}</button></a>&nbsp;\n""")
}
writer.write(s"""</div><br><br><br><br>\n""")
writer.close()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above note about not opening/closing the writer multiple times. It should really only be opened once and then passed as a parameter.

@ZhaokunHu
Copy link
Contributor Author

I have refactored my code. Please let me know if there are any shortcomings in terms of functionality and which parts of the code still need modification. I will do my best to make the necessary changes.

@Dolu1990
Copy link
Member

Dolu1990 commented Aug 7, 2023

Hi ^^
I think it can be merged.

overall, maybe the issue currently is for none-small design.
maybe having some modes which :

  • omit combinatorial signal draw to keep things smaller (but keep connectivity done across them), so focussing on states
  • Preserve the Area structure / locality, but i guess this maybe not possible using HDElk ?
  • In some modes, we could even imagine that the diagram would "only" represent the connectivity between Area / sub component / io

One typical example could be recent reimplementation of the StreamFifo, which is now very much structured using Area

Change some judgement of bus.
@ZhaokunHu
Copy link
Contributor Author

Hi ^^ I think it can be merged.

overall, maybe the issue currently is for none-small design. maybe having some modes which :

  • omit combinatorial signal draw to keep things smaller (but keep connectivity done across them), so focussing on states
  • Preserve the Area structure / locality, but i guess this maybe not possible using HDElk ?
  • In some modes, we could even imagine that the diagram would "only" represent the connectivity between Area / sub component / io

One typical example could be recent reimplementation of the StreamFifo, which is now very much structured using Area

image
Thank you for your suggestions.

  1. I didn't fully understand what constitutes a combinatorial signal. To ensure the correct connectivity, I can only merge the nodes and edges instead of directly eliminating them. In the latest commit, I attempted some simplifications, but the results were only moderately successful.
  2. 2 & 3. Based on your suggestions, representing an Area in HDElk is indeed challenging. Additionally, it's difficult to differentiate signals within an Area as inputs or outputs. Therefore, I cannot add ports to the Area as I do for modules. One possible solution I can think of is to annotate the signal names connecting the Area's internal and external components, as shown in the diagram above.

@Dolu1990
Copy link
Member

Dolu1990 commented Aug 8, 2023

Hi ^^

I didn't fully understand what constitutes a combinatorial signal

So i would say, in your screenshot above, combinatorial signals (stuff which isn't reg nor io) as for instance when_test12_l64, instead of being a box, could just represent connectivity.

it's difficult to differentiate signals within an Area as inputs or outputs

right :) something things are even mixed :/

@ZhaokunHu
Copy link
Contributor Author

So i would say, in your screenshot above, combinatorial signals (stuff which isn't reg nor io) as for instance when_test12_l64, instead of being a box, could just represent connectivity.

Thank you again for your suggestions. ^^
I'm also eager to remove signals like "when_test12_l64"(stuff which isn't reg nor io) and have made some attempts in that direction.
image
Directly removing the node and converting it into a connection line is not feasible due to scenarios with multiple inputs and outputs, as illustrated in the diagram above.
Since converting this node into a connection isn't possible, I'm left with the option of renaming it. I've tried using reflection to access information defined within the code, aiming to replace the vague and ambiguous "when_test12_l64" label with the actual function names or conditions that the designer specified while writing the code. #1067 Unfortunately,due to my limited programming skills, I've been unable to successfully retrieve the desired information using reflection, and this idea remains unrealized.
As I lack the ability to implement this optimization myself, I may not be able to carry out this enhancement. :(

@Readon
Copy link
Collaborator

Readon commented Aug 14, 2023

Hi ^^ I think it can be merged.

overall, maybe the issue currently is for none-small design. maybe having some modes which :

  • omit combinatorial signal draw to keep things smaller (but keep connectivity done across them), so focussing on states
  • Preserve the Area structure / locality, but i guess this maybe not possible using HDElk ?
  • In some modes, we could even imagine that the diagram would "only" represent the connectivity between Area / sub component / io

One typical example could be recent reimplementation of the StreamFifo, which is now very much structured using Area

I think we can merge this with a compatible API first. Then we could modify it if it is necessary later.

@Readon Readon merged commit 38149a8 into SpinalHDL:dev Aug 15, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants