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

quickstart/logicvector example (Bounded) #6

Closed
wants to merge 28 commits into from

Conversation

radonnachie
Copy link

@radonnachie radonnachie commented Apr 13, 2020

umarcor/ghdl/pull/1

This PR adds an example of unconstrained std_(u)logic_vectors. It leans on a selection of needed bits from ghdl.h, as an introduction to that header. I thought this was a good idea because the AccArray example could be documented to show how a bounded logic array (logic vector) can be written from the example's integer array.

Currently crashes, but I left that as is because a patch may be on its way soon (ghdl/ghdl/issues/1224).


  • Addition of example 'accarray', "Access to array". Actually, I'd further split it into 3 different examples:
    • ...
    • ...
    • A last one to show how to handle logic types. Since most of the signals in HDL designs are directly or indirectly std_logic or std_logic_vector, I believe it deserves a specific example. In fact, there might be multiple ways to handle the representation in C.

@umarcor
Copy link
Owner

umarcor commented Apr 13, 2020

I think that this should also be split in two. I know... at first I thought I would add 2 examples only, and they ended up being 9. In you case, we have 5.

It's ok to show how to pass constrainted std_logic_vectors, instead of std_logic only. However, the introduction to using ghdl.h should be a separate example. So, this example would be split into quickstart/logicvector (this PR) and cinterface/intro (new PR agains header). This is mostly because I expect the example about std_logic_vectors alone to be extended in the future with conversions of the C types, to match other HDL kernels.

@radonnachie radonnachie changed the title Logic vectors Bounded LogicVector example Apr 14, 2020
@radonnachie radonnachie changed the title Bounded LogicVector example LogicVector example (Bounded) Apr 14, 2020
@radonnachie radonnachie changed the title LogicVector example (Bounded) quickstart/logicvector example (Bounded) Apr 14, 2020
Copy link
Owner

@umarcor umarcor left a comment

Choose a reason for hiding this comment

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

You are fast! See comments below.

vhpidirect/quickstart/logicvector/main.c Outdated Show resolved Hide resolved
vhpidirect/quickstart/logicvector/main.c Outdated Show resolved Hide resolved
vhpidirect/quickstart/logicvector/main.c Outdated Show resolved Hide resolved
vhpidirect/quickstart/logicvector/run.sh Outdated Show resolved Hide resolved
vhpidirect/quickstart/logicvector/run.sh Outdated Show resolved Hide resolved
vhpidirect/quickstart/logicvector/tb.vhd Outdated Show resolved Hide resolved
vhpidirect/quickstart/logicvector/tb.vhd Outdated Show resolved Hide resolved
vhpidirect/quickstart/logicvector/tb.vhd Outdated Show resolved Hide resolved
umarcor added a commit that referenced this pull request Apr 15, 2020
Copy link
Owner

@umarcor umarcor left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple of minor style issues, the missing addition to the docs, and we are done!

vhpidirect/quickstart/logicvector/run.sh Outdated Show resolved Hide resolved
vhpidirect/quickstart/logicvector/run.sh Outdated Show resolved Hide resolved
vhpidirect/quickstart/logicvector/tb.vhd Outdated Show resolved Hide resolved
Copy link
Owner

@umarcor umarcor left a comment

Choose a reason for hiding this comment

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

Please, see comments regarding the content in the docs, and we are done!

doc/vhpidirect/examples/quickstart.rst Outdated Show resolved Hide resolved
doc/vhpidirect/examples/quickstart.rst Outdated Show resolved Hide resolved
doc/vhpidirect/examples/quickstart.rst Outdated Show resolved Hide resolved
doc/vhpidirect/examples/quickstart.rst Outdated Show resolved Hide resolved
doc/vhpidirect/examples/quickstart.rst Outdated Show resolved Hide resolved
doc/vhpidirect/examples/quickstart.rst Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
@radonnachie
Copy link
Author

An important addition was the HINT that explains the mismatch between integer and boolean arguments between the getVectorSize declarations in C and VHDL, respectively.

I feel that the HINT makes it okay to leave the discrepancy in there. Really I wanted the VHDL arguments to be limited to [0, 1] which is why I started with a boolean. But I didnt want to have to include <bool.h> in C to make it match

Copy link
Owner

@umarcor umarcor left a comment

Choose a reason for hiding this comment

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

I'm copying the plan from #6 (comment) here, for better exposal:

So, #4 is only dependent on #7, and I can work on finishing it.

Then, the chain would be:

I'll finish #12 and merge it into #11 then, High Priority

Great! But not "high priority", just "priority". We do this to learn and have fun! We can finish it today or next year. No hurry at all ;)

Then I need to dive into #10 with a keen eye.

Do not hesitate to ask. To understand what's going on with the VUnit subexample, you might find the following slides useful: https://github.com/umarcor/MSEA/blob/master/doc/2020_03/main.pdf. See the first diagram of slide 23.

Of course, I MUST update those slides as soon as we are done pushing all these "new" examples.

doc/vhpidirect/examples/arrays.rst Outdated Show resolved Hide resolved
doc/vhpidirect/examples/arrays.rst Outdated Show resolved Hide resolved
doc/vhpidirect/examples/arrays.rst Outdated Show resolved Hide resolved
doc/vhpidirect/examples/arrays.rst Outdated Show resolved Hide resolved
doc/vhpidirect/examples/arrays.rst Outdated Show resolved Hide resolved
doc/vhpidirect/examples/arrays.rst Show resolved Hide resolved
vhpidirect/arrays/logicvector/tb.vhd Show resolved Hide resolved
vhpidirect/arrays/logicvector/run.sh Outdated Show resolved Hide resolved
@radonnachie
Copy link
Author

Also hosted https://ghdl-cosim.readthedocs.io/en/logicvectors/vhpidirect/examples/arrays.html#logicvector

Copy link
Owner

@umarcor umarcor left a comment

Choose a reason for hiding this comment

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

See comment about reordering the content in the docs. You can also leave it as is. Change it only if you are going to fix the other minor issues in some rebase.

doc/vhpidirect/examples/arrays.rst Outdated Show resolved Hide resolved
doc/vhpidirect/examples/arrays.rst Show resolved Hide resolved
doc/vhpidirect/examples/quickstart.rst Outdated Show resolved Hide resolved
Copy link
Owner

@umarcor umarcor left a comment

Choose a reason for hiding this comment

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

Good! To be merged right after #12 #15.

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

2 participants