-
Notifications
You must be signed in to change notification settings - Fork 267
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
Discussion on ptr_pkg structure and naming #583
Comments
Hi @bradleyharden! Overall, I'm good with improving the structure to make it easier to understand and, hopefully, more efficient. However, I am afraid that your current proposal does not preserve all the features. We can rewrite the four shared varibles you propose as a single record: type string_ptr_t is record
ref : index_t;
mode : storage_mode_t;
end record;
type ext_storage_item_t is record
acc : extstring_access_t;
length : integer;
end record;
type ext_storage_vector_t is array (natural range <>) of ext_storage_item_t;
type ext_storage_vector_access_t is access ext_storage_vector_t;
type ptr_storage is record
int_index : natural;
ext_index : natural;
int_storage : vava_t;
ext_storage : ext_storage_vector_access_t;
end record;
shared variable st : ptr_storage := (null, 0, null, 0); Now, if we compare it side by side, the most meaningful differences are:
The point is that there are two types of external modes. Therefore, we do need three different 'shared' arrays (either 3/6 different variables or a record with as many fields). Apart from that, the I propose the following definitions: type string_ptr_t is record
ref : index_t;
mode : storage_mode_t;
end record;
type extacc_item_t is record
acc : extstring_access_t;
id : index_t;
length : natural;
end record;
type extfnc_item_t is record
id : index_t;
length : natural;
end record;
type extacc_vector_t is array (natural range <>) of extacc_item_t;
type extacc_vecaccess_t is access extacc_vector_t;
type extfnc_vector_t is array (natural range <>) of extfnc_item_t;
type extfnc_vecaccess_t is access extfnc_vector_t;
type ptr_storage is record
int_idx : natural;
acc_idx : natural;
fnc_idx : natural;
int_vec : vava_t;
acc_vec : extacc_vecaccess_t;
fcn_vec : extfcn_vecaccess_t;
end record;
shared variable storage : ptr_storage := (null, 0, null, 0);
It's ok if you want to make them. However, since I'm more familiar with the three types, it might be faster for me. It's up to you. |
@umarcor, yes, I realized in my sleep that I had misunderstood. I didn't fully understand the I re-read the code, and I think I follow everything now. However, one of your statements confused me:
In the existing code, the If that's the case, then Since the
If the answers to those questions are favorable, then why not combine If that would work, then this is how I would implement it. Here, I'm giving a "verbose" definition of the storage types to highlight the differences in their storage requirements. Mode type string_ptr_t is record
ref : index_t;
mode : storage_mode_t;
end record;
type internal_item_t is record
acc : string_access_t;
end record;
type extacc_item_t is record
acc : extstring_access_t;
length : natural;
end record;
type extfnc_item_t is record
length : natural;
end record;
type internal_vector_t is array (natural range <>) of internal_item_t;
type internal_vector_access_t is access internal_vector_t;
type extacc_vector_t is array (natural range <>) of extacc_item_t;
type extacc_vector_access_t is access extacc_vector_t;
type extfnc_vector_t is array (natural range <>) of extfnc_item_t;
type extfnc_vector_access_t is access extfnc_vector_t; In reality, we would probably implement it as type string_ptr_t is record
ref : index_t;
mode : storage_mode_t;
end record;
type extacc_item_t is record
acc : extstring_access_t;
length : natural;
end record;
alias internal_vector_access_t is vava_t;
type extacc_vector_t is array (natural range <>) of extacc_item_t;
type extacc_vector_access_t is access extacc_vector_t;
alias extfnc_vector_access_t is integer_vector_access_t; We would also need to store the current index for each of these storage types. You like to group them into a single record, but I want to split them into three separate records, because I will be adding more to each storage as part of my code. I can get into that if you would like, but that's a minor point compared to the above discussion. |
Your are correct. Currently
When
Currently, the shared buffer between C and VHDL is defined as
The meaning is 'you did something wrong': https://github.com/VUnit/vunit/blob/master/vunit/vhdl/data_types/src/string_ptr_pkg-body-2002p.vhd#L131-L135. It is an invalid default, to fail in case the user used extacc or extfnc without specifying
I'd say yes, although I am not sure to understand the question. How can
I expect users to define constant eids: byte_vector_ptr_t := new_byte_vector_ptr( 256, extacc, 0);
constant eid_lengths: integer_vector_ptr_t := new_integer_vector_ptr( 256, extacc, 1);
-- external modes enabled
constant eid_first_vector := natural := eids(0);
constant eid_fifo_input := natural := eids(1);
constant eid_RAM_content := natural := eids(2);
-- external modes disabled
constant eid_first_vector := natural := -1;
constant eid_fifo_input := natural := -1;
constant eid_RAM_content := natural := -1; and, in C: D[0] = (uint8_t *) malloc(3);
D[0][0] = x; //assign id that will be accessed from VHDL as eid_first_vector
D[0][1] = y; //assign id that will be accessed from VHDL as eid_fifo_input
D[0][2] = z; //assign id that will be accessed from VHDL as eid_RAM_content
D[1] = (uint8_t *) malloc(256*4);
D[1][0] = xx; //assign maximum length of eid_first_vector
D[1][1] = yy; //assign maximum length of eid_fifo_input
D[1][2] = zz; //assign maximum length of eid_RAM_content
D[x] = (uint8_t *) malloc(xx * sizeof(X_TYPE));
D[y] = (uint8_t *) malloc(yy * sizeof(Y_TYPE));
D[z] = (uint8_t *) malloc(zz * sizeof(Z_TYPE)); where x, y and z are in the range Then, assuming that Of course, I am interested on hearing any thoughts you might have about this procedure.
Even if
If we wanted to use the id from the first column (
Let's assume that all the external functions that we define are callbacks. In this context, a
Although it might be possible to achieve this, I see two major drawbacks:
I don't think this is relevant at all. It's ok if you want to split them into separate records, specially if you are going to add more content. I wrote it again as a single record in order to not get distracted with cosmetic changes, because I wanted to focus on |
@umarcor, thanks for the example in both VHDL and C. It was very helpful to understand how you intended the interface to work. Also, thanks for taking the time to explain this all to me. It's very relevant to my additions, so I want to make sure Regarding my comment
They were not, so none of the comments that followed apply. However, now that I understand the system better, I have some additional thoughts. But before I speak, I would like to make sure I really understand the whole system and your design goals when creating it. I have a few questions for you:
|
I understand it and I agree on it being important. Hence, I'm ok with discussing this as long as you wish, and I don't mind slightly repeating ourselves until we get it clear.
Furthermore, if you have multiple tests in a Last, although reference VHDL packages are provided in https://github.com/VUnit/vunit/tree/master/vunit/vhdl/data_types/src/external, it is possible to provide custom versions through
With mode Conversely, mode One use case is to define from Python a callback that is executed in Another use case is to run a service that accepts HTTP requests and contains the virtual model of a memory. Then, multiple simulations can be executed in several boards/workstations, all of them sharing the virtual memory by using Precisely, I am very interested on your contributions regarding the new queue implementation, because I think that providing a generic solution to plug a queue/vector of type |
@umarcor, sorry for the delay. I haven't had much free time lately. I wanted to go through your examples in more detail before responding, but I don't know how long it will be before I have the time. Instead, I think I will lay out my ideas and see what you think. If possible, I would also like feedback from @LarsAsplund and @kraigher, because I think my ideas are far-reaching and would require a lot of work. In the end, though, I think it would be worth it. Overall, I like the new external pointer system, but I think I see some opportunities to simplify the interface for users and more fully integrate external pointers with other VUnit data structures. That being said, my C is a bit rusty, so I might be missing something. In the current system, users must manage external pointers manually. For instance, in the example you provided above, users must allocate two arrays to store the Instead, I would like to see VUnit manage pointers for the user. We already do this for users in VHDL, why not mimic the same interface in C? Essentially, my thought is this: Create a new C library, On the VHDL side, the type ext_ptr_t is record
ref : index_t;
end record; Here, On the C side, each struct ext_ptr_t {
int32_t ref;
uint32_t length;
uint8_t *ptr;
}; I don't see any need to separate pointers by type (i.e. uint8_t * vs. int32_t *), because users can always cast the bare pointer as needed. On the VHDL side, I think we could find a way to implement something like a pointer type cast from type *_ptr_t is record
ref : index_t;
mode : storage_mode_t;
end record; If the So far, I've addressed the management problem, but I haven't addressed the syncing problem. For that, I would propose that we allow users to provide a unique name as an additional argument to the What I've proposed seems to align well with the use cases for mode With the current external pointer system, I don't see an easy way to implement queues between an external actor and a VHDL actor. We could alter the Alternatively, we could use an approach similar to what I proposed above. We could implement a C library, This structure would also mesh well with my new, proposed type system. Right now, Finally, we could do the same for the If we followed this approach, I think the result would be something that is extremely easy to work with in VHDL. Essentially, all of the current features and data structures in VUnit could be extended into C for co-simulation. What do you think? |
I'm the one who is sorry now. I completely overlooked your last comment.
I do partially agree, but there some corner cases. In the current example users must allocate two arrays to store the eid and length of each point, but the pointer does not need to be explicitly allocated, it might be given. For example, when using Octave + VUnit + GtkWave, data is allocated in Octave, and VUnit/C receives a struct with the pointer and some fields describing the size (be it a vector, a matrix, a cube). Assuming that the data is saved by Octave as 32bit IEEE integers, C and/or VHDL can use the pointer/data without needing to explicitly allocate another buffer. This is opposite to current VUnit's built-in and internal integer vector types, where allocation is hidden. Should we reproduce the API in C, it would not be possible to implement the Octave example I just explained. I believe that this is not critical, but we need to consider that
Totally agree.
I am not sure... The model you explained is based on doing a one-by-one equivalence of VHDL and C functions, so that callbacks are used. Although My idea was that As commented in #603, we might later use the VHPIDIRECT feature of binding access types to implement custom records/structs. Regarding memory vs stream, I agree that streams can be better mapped to queues. That's why I have not tried to extend external strings/integer_vectors to AXI Stream verification components. I think it will be better done after these enhancements to support external queues are merged.
Currently, users are expected to copy and modify Therefore, we need to provide a mechanism in the C sources of |
@LarsAsplund and @umarcor, here is the discussion I promised on Gitter.
In their current form, I find the storage data structures for
string_ptr
andinteger_vector_ptr
a bit confusing. I think I can simplify them.So far, you have kept the original definition of
string_ptr_t
, containing only theref
element. The valueptr.ref
indexes intost.idxs
which retrieves astorage_t
element containing themode
and another referenceid
. The value ofid
then indexes eitherst.ptrs
orst.eptrs
, depending onmode
. The types and shared variables in question are given below.I see a few different problems with this. First, you have to index into an array twice for every access of a
string_ptr
. That's a bit awkward. Second, you are storing a uselesslength
parameter for every internalstring_ptr
.I think I can improve the structure by shifting things around slightly.
The
mode
should be added tostring_ptr_t
, because it represents an intrinsic attribute of astring_ptr
. Themode
of astring_ptr
will never change after its creation, so this isn't a problem when treatingstring_ptr
values as constants.Storing a
length
attribute is only necessary for external pointers. Because of this, the storage requirements for internal and externalstring_ptr
s are fundamentally different. Therefore, they should use distinct storage systems.Below, I propose a new set of definitions to resolve these problems.
Now, you only need to perform one indexing operation when accessing a
string_ptr
, because themode
attribute is able to break the ambiguity immediately.If you agree with this new structure, I will make these changes while I make the other modifications necessary for my new queue implementation.
The text was updated successfully, but these errors were encountered: