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

Add packageless intvector examples (#5 a + c) #11

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

radonnachie
Copy link

@radonnachie radonnachie commented Apr 17, 2020

In order to create an easy ramp through the rest of the examples, it felt that an example of int array passing without packages was needed.

This is #5 a and #5 c

#6 (comment)

  1. #5a (No packages, int array passing)
  2. #5c (No packages, int array, generic passing)
  3. quickstart/logicvector example (Bounded) #6 (No packages, logic array passing)
  4. vhpidirect: add 'quickstart/package' #7 (Packages)
  5. quickstart/matrices: 2D constrained/bound array of reals/doubles #10 (Packages real array passing)
  6. quickstart/sharedvar Example (Set Package shared variable) #4 (Packages, shared integer access)
  7. #5b (Generic Packages, int array passing)
  8. cinterface/intro Example (Header intro) #8
  9. Updated Header: std_u/logic, N Dim unconstrained arrays. #3

radonnachie added a commit to radonnachie/ghdl-cosim that referenced this pull request Apr 17, 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.

See comments below.

vhpidirect/quickstart/intvector/caux.c Outdated Show resolved Hide resolved
vhpidirect/quickstart/intvector/caux.c Outdated Show resolved Hide resolved
}

int* getIntArr_ptr(){//function acts like a constructor so initialise the variable
for (int i = 0; i < SIZE_ARRAY; i++)
Copy link
Owner

Choose a reason for hiding this comment

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

I would limit getIntArr_ptr to retrieving the pointer.

Optionally, you can add a main.c where this caux.c is imported and the content of intArray is redefined before calling ghdl_main. This would not replace the caux.c only example. It'd be two subexamples of the same. This would be interesting, because it would be the example where to show why having caux.c, caux.h and main.c makes sense, and how to use them separated or together.

Don't take me wrong: I like the concept of a get function that acts as a generator, because that's the essence of VUnit/vunit#603, and we are kind of blocked. However, I believe that it fits later, not in this example.

Copy link
Author

@radonnachie radonnachie Apr 18, 2020

Choose a reason for hiding this comment

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

So, I didn't stick to such a simple main.c+caux subexample, but I think you may like what I posted. I have not run through the many ways to compile and run the executable, as it is an early example, and we have a dedicated section for that, and the executable asks for user input.

The subexample prompts the user for the array's length, fills the array, and calls ghdl_main. Only thing I don't like is trusting the user to enter a reasonable number, but I cannot seem to break the executable with outrageous values so I think it is safe. Only thing is getting the test to provide runtime stdin input...

Copy link
Owner

Choose a reason for hiding this comment

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

I have not run through the many ways to compile and run the executable, as it is an early example, and we have a dedicated section for that, and the executable asks for user input.

My point was not about different ways to compile and run the executable, but about different requirements dependending on the number of C files.

  • -Wl,main.c or -Wl,caux.c only. Already covered.
  • -Wl,caux.c -Wl,main.c. Not used yet. Questions that users might have:
    • Is it -I./ required?
    • Does caux.h need to exist?

Of course, this can be explained in a different example instead of here.

The subexample prompts the user for the array's length, fills the array, and calls ghdl_main.

I think it would be better to use/parse argv[1]. Three options:

  • If argc == 1, use argv[1] as an integer (atoi).
  • Pase argv and look for --size= or -s. Then use atoi as in the previous point.
  • Search for -- in argv and consider all previous args to be "custom args for C". Then search for --size or -s as in the previous point. Pass args after -- to GHDL. Open question would be: what to do when -- is not found? Are all args for C or for ghdl_main?

Either of these would be interesting because they'd show how to add custom args (that need to be removed from the argv passed to GHDL). I would not use any lib such as optargs, tho. We want to support a single custom argument.

Only thing I don't like is trusting the user to enter a reasonable number, but I cannot seem to break the executable with outrageous values so I think it is safe.

Even if they try to, it's ok. See ghdl/ghdl#822.

Only thing is getting the test to provide runtime stdin input...

I think it is better to pass argv, which allows ./tb 26 or ./tb -s 26 or ./tb --size=26 or ./tb --size 26 -- --wave=mywave.ghw.

vhpidirect/quickstart/intvector/caux.c Outdated Show resolved Hide resolved
vhpidirect/quickstart/intvector/pkg.vhd Outdated Show resolved Hide resolved
vhpidirect/quickstart/intvector/run.sh Outdated Show resolved Hide resolved
vhpidirect/quickstart/intvector/tb.vhd Outdated Show resolved Hide resolved
radonnachie pushed a commit to radonnachie/ghdl-cosim that referenced this pull request Apr 18, 2020
@radonnachie radonnachie changed the title Add packageless intvector example (#5 a) Add packageless intvector examples (#5 a + c) Apr 18, 2020
@radonnachie
Copy link
Author

radonnachie commented Apr 18, 2020

The last open point in #12 was:

Yeah, I like the idea of the Interface Generic setting the size of the C array... makes things more reasonable. Will work it.

I do feel that the allocateIntArr could just initialise the array with the classic 11*(i+1) jazz. I'll make it that subsequent calls will free the pointer and (if the size > 0) re-malloc, and re-fill.

Otherwise it feels like vhdl will be able to create more than one malloc'd array and each new one would need to be kept track of in a dynamic array of pointers, to be freed at the end. Possible but a little >excessive when we are just trying to show generics defining vector bounds, no? Have I misread you completely?

See the caux.c file. I think you'll be happy with it.

To which umarcor responded:

I see your point. Let's keep this example as is, just splitting allocation and initialization. Then, we can add a shintvectors as done with shint.

@radonnachie
Copy link
Author

https://ghdl-cosim.readthedocs.io/en/intvector/vhpidirect/examples/arrays.html

You seem very capable of reading the rst files, but it is hosted above if you prefer

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 generics below.

@@ -21,12 +21,15 @@ jobs:
vhpidirect/quickstart/math,
vhpidirect/quickstart/customc,
vhpidirect/quickstart/package,
vhpidirect/arrays/intvectorgeneric,
Copy link
Owner

Choose a reason for hiding this comment

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

I think this was mislocated as a result of merging/cherry-picking.

In this example the custom entrypoint prompts the user for the size of the array and then handles the variables appropriately before
going on to execute the GHDL simulation.

:cosimtree:`A Bounded Array Sized in VHDL <vhpidirect/arrays/intvectorgeneric>`
Copy link
Owner

Choose a reason for hiding this comment

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

I think it is better to keep this as a subexample of intvector. If you want, just move the current content of intvector to say intvector/sizedinc and rename this one to intvector/sizedinvhdl.

The main point of this (intvectorgeneric) is NOT the usage of generics, but the fact that the size is provided from VHDL. Hence, the introduction of the subexample should not focus on generics.

We can keep the description of the generics in a HINT. However, it is not required, because generics can have defaults. Moreover, it might make sense to move use a constant instead, and move the usage of generics to a different quickstart/cli example. This is because it is also possible to manipulate argc and argv before calling ghdl_main. Hence, it is feasible to have some size defined by a generic in VHDL, but have it defined in C in practice, by passing it as a -gGENERIC= to ghdl_main. If such an entrypoint is combined with the current intvectorgeneric example, then VHDL will pass that arg back to C when calling getIntArr_ptr. Although this might feel as a rare use case, it is easy for it to happen when adapting a design that is not aware of VHPIDIRECT features, and we want to optionally enhance it with them. Which is, precisely, the foundation of the non-intrusive philosophy of VUnit (and, hence, VUnit/cosim).

@@ -0,0 +1,9 @@
#include "caux.h"
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't this somehow redundant to caux.c in the parent dir? Sure, here the size is saved in a variable, instead of using a define. However, initializing int arraySize = (sizeof(intArray)/sizeof(int)); should work for both cases. The same applies to intArray. Nothing prevents the subexample from overwriting the pointer. The hardcoded content would be useless, but I think it pays off because we want to show that users can use main optionally and without modification of helper C functions.

Copy link
Author

Choose a reason for hiding this comment

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

sizeof(intArray) in the subexamples returns sizeof(int*), not sizeof(int[])... In order make it more closely match the original example I can make this array statically defined. then we start to lose out on the extensive main() entry point passing examples to ghdl for various reasons.

I think that may be for the better though. Let's simplify this to exactly the same as the intvector example, just with the values being printed out in main before the ghdl_main() call.

An we can make the extensive examples of the cosimulation in a different PR??

Copy link
Owner

Choose a reason for hiding this comment

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

then we start to lose out on the extensive main() entry point passing examples to ghdl for various reasons.

I don't understand this sentence. Can you please reword it?

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I want to be extra clear so that you can tell me if I have any misunderstandings on the examples you want.

The sub example (maindefined) is to show how to use a custom entry point (main.c) to preprocess the VHPIDIRECT shared int[] (in caux.c).
I think I made the subexample a little more complicated than it needs to be by asking for user input to defined the size of the int[].

  • This meant that the variable had to be a malloc'd int*
  • It also then strayed away from the parent example (intvector)
  • It also is a little too much too soon because it starts begging the questions of all the ways to pass arguments to main() and even arguments to ghdl_main() through main(). **

I am proposing that we cut all of the discussion out by making the sub example much more similar to the parent example (exactly the same, except a main.c that wraps ghdl_main. main() does nothing, at most prints out "ghdl simulation\n*****************".)

We then 'lose out' on the 3 points above, but I think that they start to get extensive enough that a whole section could be written on them (at least have their own examples in a different section). Those examples can be linked to in this one's documentation....

Copy link
Author

Choose a reason for hiding this comment

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

I see your point. Let's keep this example as is, just splitting allocation and initialization. Then, we can add a shintvectors as done with shint.

I think that I am agreeing with your earlier response.

Copy link
Owner

Choose a reason for hiding this comment

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

I think that I am agreeing with your earlier response.

Can you open a new PR with the status of this one when I did that comment? I believe that we can have that merged almost as is. We just want two subcases: set the size in C or set the size in VHDL. In both cases, the allocation is done in C. In the first one, it's hardcoded. In the second one, malloc is called once only, and a second helper function is used to fill it in C.

Then, we will have a better perspective about what to add here. I created #14, so this can be about the auto-reallocation logic that you have.

Copy link
Owner

@umarcor umarcor Apr 19, 2020

Choose a reason for hiding this comment

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

Bear this in mind. The question we want to answer is "How do I allocate in C and share a pointer/access while defining the size in one language only?". We need to provide two cases: hardcode the size in C (and pass it to VHDL) and harcode the size in VHDL (and pass it to C).

@umarcor
Copy link
Owner

umarcor commented Apr 21, 2020

@RocketRoss, now that the first set of examples about 'Arrays' is upstreamed, how do you want to repurpose this PR? See #15 (comment).

@radonnachie
Copy link
Author

@umarcor I want to quickly double check my understanding of which examples are up at the moment, then I'll get back to you!

@radonnachie
Copy link
Author

A quick recap, seeing as it has been so long.

This was an attempt to create a main.c which prompted the user for the length of the integer array, which would then be defined in C. This was my first step in moving from bounded arrays to unbounded ones (this is a dynamically bounded one).

The allocation of an integer array in C and separately in VHDL are both covered. So this doesn't need to show that. The only thing it is showing is that the main() can ask for user input before calling the ghdl_main. I think this isn't something we would need to tell people after showing the wrapping of ghdl_main in a custom main().. but here it is, already written up.

Personally, I think that it should be made more interactive: because that is where I went with it in my personal learnings! I made the caux.c contain a setter that would prompt the user for a value, maybe even the index.

It definitely feels like it should end up becoming a CLI example.

@umarcor
Copy link
Owner

umarcor commented Jul 15, 2020

It definitely feels like it should end up becoming a CLI example.

Agree. Now that CLI examples were added, it feels like this "more interactive" example can be a subexample of it.

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