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

No more name mangling #336

Merged
merged 15 commits into from Mar 10, 2022
Merged

No more name mangling #336

merged 15 commits into from Mar 10, 2022

Conversation

josyb
Copy link
Contributor

@josyb josyb commented Apr 12, 2020

This resolves the name mangling issue on converting top level interfaces.
This is an issue that comes up regularly, e.g. https://discourse.myhdl.org/t/unexpected-signal-name-in-converted-vhdl/389, https://discourse.myhdl.org/t/conversion-naming-issue/373, @hgomersall

@josyb
Copy link
Contributor Author

josyb commented Apr 12, 2020

Apparently, this unmangling messes up cosimulation because this expects the mangled names (if any ...)
But I think I can break the current by altering the names; A, OE and Y, of the objects used in the simulation.

reaming the simulation objects
@josyb
Copy link
Contributor Author

josyb commented Apr 12, 2020

It does fail then as well; so I believe it is warranted to publish my change and add a @pytest.xfail to the testOBufInterface() in test/conversion/toVerilog/test_tristate.py

@josyb
Copy link
Contributor Author

josyb commented Apr 12, 2020

@cfelton , @jck I believe you want to comment on this?

@hgomersall
Copy link
Contributor

We should fix the tristate test, surely?

@josyb
Copy link
Contributor Author

josyb commented Apr 13, 2020

We should fix the tristate test, surely?

IMO that is also a contrived test.
If we want to fix it we have to rewrite that module. The OA tried to re-use code to catch two flies ...And it was a fluke as I have proven by simply changing the object names.

@hgomersall
Copy link
Contributor

We should fix the tristate test, surely?

IMO that is also a contrived test.
If we want to fix it we have to rewrite that module.

the test module?

@josyb
Copy link
Contributor Author

josyb commented Apr 13, 2020

We should fix the tristate test, surely?

IMO that is also a contrived test.
If we want to fix it we have to rewrite that module.

the test module?

to start with. I am trying to do this on my machine without updating the PR all the time

@josyb
Copy link
Contributor Author

josyb commented Apr 13, 2020

We should fix the tristate test, surely?

IMO that is also a contrived test.
If we want to fix it we have to rewrite that module.

the test module?

to start with. I am trying to do this on my machine without updating the PR all the time

I guess I need some help to get Cosimulation running, spent all afternoon on it. Note that I want to run it from within Eclipse (all my tools in one place: PyDev, Impulse, Sigasi, XML, MarkDown, etc.) And Windows 10 ...

@hgomersall
Copy link
Contributor

I guess I need some help to get Cosimulation running, spent all afternoon on it. Note that I want to run it from within Eclipse (all my tools in one place: PyDev, Impulse, Sigasi, XML, MarkDown, etc.) And Windows 10 ...

Well, that sounds unnecessarily painful. Why not run a linux virtual machine for testing? It's easy to have it all synced so you mount the relevant folders on the linux machine.

@josyb
Copy link
Contributor Author

josyb commented Apr 14, 2020

Running Linux is a pita, I have to look up about every command. And installing SW is quite often as painful, if not more, as in Windows. I started a Linux box but gave up ...
Running the tests from a command prompt would be fine though.
Anyway, it will have to wait until I have some free time.

@hgomersall
Copy link
Contributor

@josyb I suspect your issues are more around relative comfort. I could build you a virtualbox image if you want?

@hackfin
Copy link

hackfin commented Apr 14, 2020

Might have mentioned before, but it might be worth investing into a one time docker setup for Windows to run prefabricated reference environments for testing, so you don't need to juggle with SW packages.

Here's a container that has fairly recent ghdl/iverilog etc. installed to run all the co-sim tests:
https://hub.docker.com/r/hackfin/myhdl_testing
At the bottom you'll find the two-liners to run the test suite. But attention: the tests will mostly fail with the master branch, see also notes in #322 with stdout/stderr behaviour of newer GHDL versions.

The only thing on Windows you'll need is to install the Docker Toolbox (https://www.docker.com/products/docker-desktop). And I don't know how painful it is these days to mount local volumes.
Also, you could of course run all in a browser on the Docker playground (https://labs.play-with-docker.com, requires a docker login).

@josyb
Copy link
Contributor Author

josyb commented Apr 14, 2020

You see, all this virtualenv and docker stuff is only needed because software gets broken even between minor releases ... It is bad enough that my disk is filling up with different Vivado and Quartus versions ... Just package the complete mess and defer it to the customer. (I must be ranting ...)
We should not have to fiddle with SW packages. Code should never be broken. IMHO a new release must support everything of the previous, unaltered.
Anyway, it'll have to wait ... until cosimulation runs on Windows

@hgomersall
Copy link
Contributor

@josyb ?? The problem seems to be around installing the relevant tools to do testing. What does that have to do with backwards compatibility? Moreover, are we really at the point we should lock down our API?

@josyb
Copy link
Contributor Author

josyb commented Apr 14, 2020

@hgomersall You are right, it is about installing tools to test. It took me several installations of MingW/MSys before I managed to execute iverilog-vpi.exe to build myhdl.vpi and yet it doesn't work. In (my) ideal world, that myhdl.vpi would come ready with the MyHDL package and would run with any version of iverilog (not older than the myhdl.vpi, I agree)
About API: once you publish it is essentially locked, changing functionality of a public function has to have good reasons. Something I used to do (for our small camera API) was the Microsoft-trick of adding an Ex suffix to the newer function.

@jck jck self-requested a review April 16, 2020 15:45
@simon-zumbrunnen
Copy link

@josyb Don't forget the current release is 0.11. Major API changes are still possible before 1.0. Or am I missing something?

BTW: Thank you so much for your hard work on myhdl. I really want to use it at work but it's hard to convince my boss because the community around it is too small 😥

@josyb
Copy link
Contributor Author

josyb commented Apr 17, 2020

@josyb Don't forget the current release is 0.11. Major API changes are still possible before 1.0. Or am I missing something?

It is a matter of principle: IMO you try to avoid breaking code, unless absolutely necessary.

BTW: Thank you so much for your hard work on myhdl. I really want to use it at work but it's hard to convince my boss because the community around it is too small 😥

Convincing the boss is never easy, but he/she will appreciate the quality and speed of your work in MyHDL

@hgomersall
Copy link
Contributor

BTW: Thank you so much for your hard work on myhdl. I really want to use it at work but it's hard to convince my boss because the community around it is too small disappointed_relieved

You can start by writing modules in MyHDL and then using the resultant generated code.

@hgomersall
Copy link
Contributor

It is a matter of principle: IMO you try to avoid breaking code, unless absolutely necessary.

I do appreciate the sentiment, and I basically agree, but the question is what is absolutely necessary.

I'm coming round more to the idea that you should version pin dependencies, then it matters less.

josyb added a commit to josyb/myhdl that referenced this pull request Apr 19, 2020
changing  more names (in parallel with PR myhdl#336)
@josyb
Copy link
Contributor Author

josyb commented Apr 19, 2020

@hgomersall @jck I am afraid that in cosimulation one has strict rules to follow or it breaks. You can see in PR #339 that it can be broken too in the current state of the master branch.

…ws 10, properly (auto-)formatted the source

util.py: changed the myhdl.vpi path to defaukt to iverilog's known system path
test_tristate.py: the TestTristate class re-used the tristate_obuf.o for the test with the interface; which put iverilog on the wrong foot ...
@josyb
Copy link
Contributor Author

josyb commented Apr 20, 2020

@hgomersall @jck I am afraid that in cosimulation one has strict rules to follow or it breaks. You can see in PR #339 that it can be broken too in the current state of the master branch.

After all the symptom there in PR #339 is what I cured here. I took me a while before I spotted that iverilog re-used tristate_obuf.o

@hgomersall
Copy link
Contributor

What's happening with this PR?

@hgomersall
Copy link
Contributor

Let's rerun the tests with the latest test matrix. I've been running this change for a while now and it all seems good.

Copy link
Contributor

@hgomersall hgomersall left a comment

Choose a reason for hiding this comment

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

This all looks good to me. It's also been tested extensively at my end for since the original PR.

@josyb josyb merged commit c7662a0 into myhdl:master Mar 10, 2022
@josyb josyb deleted the NoMoreNameMangling branch March 10, 2022 13:00
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

4 participants