-
Notifications
You must be signed in to change notification settings - Fork 1
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/sharedvar Example (Set Package shared variable) #4
Conversation
I plan on pushing documentation soon-ish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add new tests to CI. Just ensure that a run.sh
exists, and add the subdir/location to https://github.com/umarcor/ghdl-cosim/blob/master/.github/workflows/test.yml#L21-L29
Overall, I think that this example is too complicated because there is a race between both entities to write and read the shared variable and new users might be distracted. The point should be (only) how to declare a foreign resource in a package. Hence, I'd suggest to define printInt(int n)
in C and remove getInt_prt
and the shared variable from the example.
Actually, you might want to keep this PR as is, and open another simpler one. It makes sense to explain how to use a package in one example, and explain when a shared variable is required in a different example. Then, #5 would be a third step about how to use arrays as shared variables.
From this point of view, I believe that the location of the three examples would be quickstart/package
, quickstart/sharedvar
and quickstart/accarray
.
You can delay writing the corresponding documentation until the examples are written. Specially because we have yet to decide the specific number and content.
Was wondering how to do that. Thanks. Will update PRs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say I had already written this, but I cannot find where: note that you can open all PRs against Already said in #9master
in ghdl/ghdl-cosim, which will make them more visible to other users. You do need to open PRs here when you are targeting feature branches which are only available in my fork. I will review the PRs regardless of where you open them.
Regarding the content of this PR, please, see comments below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are almost there... I think you can add the modification to the docs in the next iteration.
ugh, sorry you had to go through that. I've enabled rendering of whitespace and found a quick way to assess my files. I'll be more consistent now! |
@RocketRoss, if you are ok, I will take care of finishing this PR, so you can focus on others. |
Sure thing. I've lost a little steam. Let me try pick up the pace again! To be honest, when it comes to the documentation, I am now lost on how the references between cosim's docs and ghdl's docs work. I understood being able to reference labels like |
The trick to get that is https://github.com/umarcor/ghdl-cosim/blob/master/doc/conf.py#L130-L133. So, essentially, we can cross-reference as if they where local labels. Should a conflict exist, we can specify which "domain" to use. See http://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html. To find the labels to other sections of the main docs, you need to check the rst sources at https://github.com/ghdl/ghdl/tree/master/doc.
I don't understand what you mean with the directory manner.
Yes. In the worst case, I can let you know if there is any redundancy, while reviewing. |
Superseded by #13 |
umarcor/ghdl/pull/1
An example on how to package VHDL functions that are foreign C functions.
I suspect I will learn a better way to use an int access than to .all it.
I think that it may be a little more than what was requested, but isn't too much.
I also hope that I nailed the location for it. 🙋