Skip to content

Active path and FSM#59

Open
SteliosKaragiorgis wants to merge 6 commits into
mortbopet:masterfrom
SteliosKaragiorgis:active_path
Open

Active path and FSM#59
SteliosKaragiorgis wants to merge 6 commits into
mortbopet:masterfrom
SteliosKaragiorgis:active_path

Conversation

@SteliosKaragiorgis

Copy link
Copy Markdown

No description provided.

Add to set if a component as active path, active finite state machine(fsm) for this state or the component was active fsm
Add also to check if a component is active path, active finite state machine(fsm) for this state or the component was active fsm
If a port is on the active path or FSM then it will set the colour of the port
The changes of active path will not affect RISCV processors
getText rerurn the label of the port
Add methods for component for active path and active fsm
Set colour the component if it is active without affect RISCV processors
@SteliosKaragiorgis

Copy link
Copy Markdown
Author

I reuploaded it. I removed the comments and it is qt6. Also it is working fine with the Ripes

@mortbopet mortbopet left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Two fundamental issues with this PR:

  • You're adding VSRTL Graphics related stuff into the VSRTL core library. VSRTL core is fully isolated from VSRTL graphics - deliberately. Core is strictly a simulation library, that exposes hooks which core then uses to inspect the circuit and create a visualization of it. The point of vsrtl_interface.h is to define the interface that a simulation backend must adhere to such that VSRTL graphics can work with it. The design descision for this is based on the fact that you in theory could bring any simulation backend (or HDL language) and implement the VSRTL interface, and then the graphics library can work with it.
  • You're adding implementation-specific logic into the library. That is, you have logic which explicitly has been written for your processor model inside the framework - this is obviously a no go. It would be similar to writing a program in a programming language, and you then trying to get your program logic into the programming language itself :).

I understand that these might be big changes that you have to make to your PR, but it's important to recognize that the framework was designed this way for a reason. Whatever changes you make to VSRTL must not have anything specific to do with your processor model/whatever you're using VSRTL for. If you do find that VSRTL is lacking in capability, then it is fine to submit a PR to fix that, but then recognize that whatever change that you make to VSRTL has to be a generic change that applies to all circuit simulations one may want to visualize using VSRTL.

@SteliosKaragiorgis

Copy link
Copy Markdown
Author

So is it better to upload the PRs on Ripes mortbopet/Ripes#290 without my VSRTL changes and then add them?

@mortbopet

mortbopet commented Sep 4, 2023

Copy link
Copy Markdown
Owner

Before considering PRs into either repository you need to address the above comments.
This will require that you sit down, look at your current implementation, identify where you're adding things into VSRTL that shouldn't be in VSRTL (as described above) and then figure out a way to reimplement that either:

  1. outside of VSRTL (i.e. in your processor model implementation (the stuff that uses VSRTL which you're going to eventually PR into Ripes)
  2. If not possible outside of VSRTL (i.e. in your implementation), this means that you need to consider a change to VSRTL itself.

I cannot stress this enough. Whereever you have non-generic code inside VSRTL this is an indication that you need to change that code. VSRTL is a framework and thus implementation-specific logic should not be anywhere within the framework.

And the same goes with an eventual Ripes PR. If you study the current Ripes codebase you will see that ISA-specific information is only found in two places:

  1. The processor models: https://github.com/mortbopet/Ripes/tree/master/src/processors/RISC-V
  2. The ISA information files: https://github.com/mortbopet/Ripes/tree/master/src/isa

So similarly, wherever your code does not follow this pattern means that you'll have to refactor it.

@SteliosKaragiorgis

Copy link
Copy Markdown
Author

Okay. First I will try to reimplement it outside the VSRTL. I think I tried it again on the past but I will try again.
The problem is that I added some lines that cointains some things from Ripes, correct?
Like if(QString::fromStdString(m_component->getHierName()).contains("MIPS"))

@SteliosKaragiorgis

Copy link
Copy Markdown
Author

Do you want first to upload my PRs on Ripes without the VSRTL changes?

@mortbopet

Copy link
Copy Markdown
Owner

The problem is that I added some lines that cointains some things from Ripes, correct?
Like if(QString::fromStdString(m_component->getHierName()).contains("MIPS"))

Correct!

Do you want first to upload my PRs on Ripes without the VSRTL changes?

If you have small, atomic, PRs which do not depend on the changes in VSRTL, and represent incremental improvements to Ripes, then by all means, go ahead and submit those PRs! :)

@SteliosKaragiorgis

Copy link
Copy Markdown
Author

Yes I have small PRs ready to upload but I have some tests that fail because it is for different ISA. I mentioned exact the prblem here: mortbopet/Ripes#290

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.

2 participants