[1/3] Add external models to string_ptr; add byte_vector* as an alias #507

umarcor · 2019-06-03T19:26:49Z

Coming from #476. Related to #470.

In this PR, external modes are added to string_ptr.vhd through VHPIDIRECT (see https://ghdl.readthedocs.io/en/latest/using/Foreign.html). With 'external mode' we mean:

VHDL can use get/set callback functions defined in C (or any C-alike compatible language) to interact with strings (byte vectors).
An array of pointers to arrays of chars (char* []) can be shared between VHDL an any C-alike compatible language.

The usage is as follows:

new_string_ptr(len, mode, eid, value) creates a new array of length len with mode mode and identifier id.
- If mode = internal (default), the array is allocated in the testbench and it is initialized with value.
- If mode = extfnc, the array is not allocated. When values are get/set, VHPIDIRECT functions/procedures read_char/write_char are called.
- If mode = extacc, the array is not allocated. A pointer is created and the value is retrieved through VHPIDIRECT function get_string_ptr. The returned value must be a pointer to an array of type uint8_t (size of 1 byte). When data is get/set, content is directly modified in the shared memory.

Notes:

When mode /= internal, value is not used and len is used to tell VHDL the maximum size of the external buffer. Currently, this is a fixed value, i.e., there is no 'synchronization' of this value.
When mode /= internal, eid is used to decide which of the shared buffers to use.
These features have been tested with C, Python and C++ (octave) tools.

In order to support using string_ptr with or without VHPIDIRECT, two different implementations of the external resources are provided:

external_string_pkg-vhpi.vhd, declares the functions/procedures as external; therefore, C implementations must be provided.
external_string_pkg-novhpi.vhd does not declare the functions/procedures as external; C implementations are not required, but it is not possible to create vectors with mode /= internal (an assertion of level error is raised).

The user needs to select whether to use external C objects. Furthermore, alternative implementions of *vhpi.pkg sources might be provided. This is not expected to be required for most use cases, but it is supported. This is achieved through a new external parameter to from_args and from_argv:

None (defaults to False)
{'string': False}
{'string': True}
{'string': ['path/to/custom/file']}

deallocate, reallocate and resize are not implemented for external modes (mode /= internal). For the case extacc, it would be easy to implement it. Since the 'connection' is a pointer, it is possible to use malloc or mmap in the C implementation. Coherently, it would be possible to update the length in the VHDL model according to the actual size in the C app.

However, when extfnc, it can be complex or not possible. Since read_char/write_char can be used to interact with external services/processes through pipes, sockets, packets, messages, etc., the implementation is likely to be very specific. Nonetheless, in this example the implementation can be the same for both modes.

byte_vector is added as an alias of string.

umarcor · 2019-06-03T19:47:30Z

@kraigher, I think that the most important part to understand this PR is https://github.com/dbhi/vunit/blob/ext-strings/vunit/vhdl/data_types/src/string_ptr_pkg-body-200x.vhd#L56-L154. Precisely, https://github.com/dbhi/vunit/blob/ext-strings/vunit/vhdl/data_types/src/string_ptr_pkg-body-200x.vhd#L65-L72:

type ptr_storage is record
  id    : integer;
  ptr   : integer;
  eptr  : integer;
  ids   : storage_vector_access_t;
  ptrs  : vava_t;
  eptrs : evava_t;
end record;

id, ptr and eptr are just pointers to the latest element in ids, ptrs and eptrs. Precisely, id is exactly the same as current_index in the master branch.

ids contains an element/reference for each allocated string/vector, no matter if it is external or internal.
ptrs contains an element/reference for each internally allocated string/vector, i.e. when the user uses new_string_ptr(id=>0).
eptrs contains an element/reference for each externally allocated string/vector, i.e. when the user uses new_string_ptr(id=>n) where n>0.

When the user uses new_string_ptr(id=>n) where n<0, the reference is only located in ids, no additional metainfo is required.

As a result, each time get, set, length, etc. are called, first we retrieve the corresponding storage_t from ids, which will tell us what kind of vector we are dealing with. Then, we execute up to three different snippets. E.g.: https://github.com/dbhi/vunit/blob/ext-strings/vunit/vhdl/data_types/src/string_ptr_pkg-body-200x.vhd#L213-L220

kraigher

The pull request does not contain any testing that the external mechanism works creating a technical debt.

vunit/builtins.py

vunit/vhdl/data_types/src/byte_vector_ptr_pkg.vhd

umarcor · 2019-06-04T21:01:56Z

The pull request does not contain any testing that the external mechanism works creating a technical debt.

The fact that all the existing tests pass is a proof that id=0 works as it did before, so backwards compatibility is ensured. Unfortunately, testing that data can/is actually read/written for externally allocated buffers is a little more complex.

Nonetheless, the external_buffer example in #476 (which is just one commit ahead of this PR), is precisely meant to test this feature. Please, see: https://github.com/dbhi/vunit/blob/feat-external-arrays/examples/vhdl/external_buffer/run.py. I didn't add it here in order to keep this PR as simple as possible, but I can do it if you want.

I must ask for some trust here.

vunit/builtins.py

kraigher · 2019-06-05T18:04:27Z

If the example serves as the test of id != 0 I would like to include it in this PR.

umarcor · 2019-06-05T19:40:18Z

If the example serves as the test of id != 0 I would like to include it in this PR.

It is added now.

~~BTW, I expect py27-acceptance tests to pass when GHDL is 'fixed' or when #509 is merged.~~

kraigher

I have looked through this again and found some issues. I will look further after they are fixed. Sorry for this taking such a long time but you are trying to change a core data type in VUnit which requires a lot of care.

examples/vhdl/external_buffer/run.py

examples/vhdl/external_buffer/src/main.c

examples/vhdl/external_buffer/src/tb_byte_vector.vhd

examples/vhdl/external_buffer/src/tb_external_byte_vector.vhd

vunit/vhdl/data_types/src/string_ptr_pkg-body-93.vhd

umarcor · 2019-06-11T22:06:50Z

@kraigher, I think I addressed all of the suggested changes. Let me know when you have another look at it.

kraigher · 2019-06-12T17:51:10Z

Ok I will look again. By the way I though the integer vector length -> len revert would become a separate PR. It would have been cleaner as that could be merged on its own.

examples/vhdl/external_buffer/run.py

examples/vhdl/external_buffer/src/tb_ext_string.vhd

examples/vhdl/external_buffer/run.py

umarcor · 2019-06-12T18:58:50Z

By the way I though the integer vector length -> len revert would become a separate PR. It would have been cleaner as that could be merged on its own.

I ~~will make~~ now did a separate PR from it: #515.

BTW, regarding this PR, do you think it is worth having separated vunit/vhdl/data_types/src/types/string_pkg.vhd and vunit/vhdl/data_types/src/types/byte_vector_pkg.vhd files? Can we merge them in a single file? Each of them is just 10 lines, so the file will not be longer than 40 lines (including the license) even after external integer_vector is added later.

vunit/ui.py

kraigher · 2019-09-25T18:03:56Z

vunit/vhdl/data_types/src/string_ptr_pkg-body-93.vhd

  impure function new_string_ptr (
    length : natural := 0;
+    mode   : storage_mode_t := internal;
+    id     : integer := 0;


I assume id=0 means automatic id assignment. This is assumed with internal string. Maybe an check is needed that is it 0 when it cannot be a custom id.

Not really. Note that this id is NOT the same id that was in the codebase until 6 months ago. That was renamed to ref: https://github.com/VUnit/vunit/blob/master/vunit/vhdl/data_types/src/string_ptr_pkg.vhd#L20 and https://github.com/VUnit/vunit/blob/master/vunit/vhdl/data_types/src/integer_vector_ptr_pkg.vhd#L21.

The mechanism to share buffers between C and VHDL is a char* [], i.e. an array of pointers to an arrays of chars. This id is used to decide which of those arrays of chars corresponds to a string in VHDL. See examples/vhdl/external_buffer/src/main.c:

If mode is extacc, VHDL will execute get_string_ptr(id) to retrieve that pointer address.

If mode is extfnc, VHDL will pass the id in each call to write_char or read_char.

In this example, using write_char (mode extfnc) is equivalent to VHDL writing directly with mode extacc. However, this is only because of the specific C implementation of write_char. In a practical example write_char is expected to do some more complex task, such as sending data through HTTP. That's where the id is handy.

When mode is internal, field id is not used at all.

Nonetheless, ref still exists. Each string, no matter the mode, has a ref; it is assigned automaticallly, as it has always been. See:

vunit/vunit/vhdl/data_types/src/string_ptr_pkg-body-200x.vhd

Lines 131 to 158 in ed75c87

case mode is

when internal =>

st.ids(st.id) := (

id => st.ptr,

mode => internal,

length => 0

);

reallocate_ptrs(st.ptrs, st.ptr);

st.ptrs(st.ptr) := new vec_t'(1 to length => value);

st.ptr := st.ptr + 1;

when extacc =>

st.ids(st.id) := (

id => st.eptr,

mode => extacc,

length => length

);

reallocate_eptrs(st.eptrs, st.eptr);

st.eptrs(st.eptr) := get_ptr(id);

st.eptr := st.eptr + 1;

when extfnc =>

st.ids(st.id) := (

id => id,

mode => extfnc,

length => length

);

end case;

st.id := st.id + 1;

return st.id-1;

Let's suppose we want to use four strings:

internal

extacc with id 0

internal

extfnc with id 1

st.ids st.ptr st.eptr string0 0 0 string1 1 0 string2 2 1 string3 3

So,

st.ids is an array of ref that contains four entries, one for each string.

st.ptr is the array of refs/pointers to internal strings (this is the element that has always existed, everything else is wrapped around it), and contains two entries.

st.eptr is the array of refs/pointers to extacc strings (id is used to get_string_ptr once only).

As a result:

st.ids(0).id is 0 (index to read/write from/to st.ptr)
st.ids(1).id is 0 (index to read/write from/to st.eptr)
st.ids(2).id is 1 (index to read/write from/to st.ptr)
st.ids(3).id is 1 (id to read/write through read_char and write_char)

Maybe I am overusing the name id? Currently, it is:

A parameter for extacc to get the pointer from C.

A parameter for extfnc to pass to read_char/write_char.

The name of the field st.ids, which contains a number (id) for each string.

The name of field st.ids().id which contains a number representing either:

The corresponding index in st.ptr.

The corresponding index in st.eptr.

The id parameter used when creating an extfnc string.

EDIT

After reading this, I think that it might be easier to understand #507 (comment). I feel that it should be possible to merge st.ptr and st.eptr in a single array. But VHDL seems to fight against me doing it...

Ok I understand a bit better, but it still means it is user error to call the function with internal and an explicit id either by name of position?

-- Case 1 new_string_ptr(length, mode => internal, id => 1) -- Not valid -- Case 2 new_string_ptr(length, mode => internal, id => 0) -- Also not valid -- Case 3 new_string_ptr(length, mode => internal) -- Valid

Maybe it is not so important to check "Case 2" but "Case 1" could easily give an error which might help debugging.

Sure. I'd say that such a test existed: id /= 0 is not supported with mode internal. Maybe I unwantedly dropped it during some rebase/modification. Let me check.

BTW, I'm also understanding and having a look at the add_builtins comment above.

I added an assertion for case 1. Note that case 3 defaults to case 2. Therefore, we have no mechanism to tell them apart. Should the default of id be some non-supported value, we could identify case 2.

Now:

I moved index_t to types_pkg.

I renamed id fields to eid and idx, to make it easier to tell them apart.

eid is now of type index_t and it defaults to -1.

Assertions are added to detect cases 1 and 2, and also when extacc or extfnc are used without providing eid.

@kraigher, shall I add tests for these assertions? If so, do they fit in vunit/vhdl/data_types/test/tb_byte_vector_ptr.vhd?

LarsAsplund

I'm ok with these changes now

umarcor · 2019-10-05T10:55:49Z

I'm ok with these changes now

🚀🚀🚀

However, let's not merge for now. After lunch, I will re-read all the comments in this issue again, to ensure that me/we are not missing anything. I will also add some further tests (e.g., to use different eid values, i.e., multiple vectors).

Thanks for your support @go2sh. Much appreciated.

go2sh · 2019-10-05T10:59:17Z

Awesome work in total (your PR series), this will rock. A did a quick test on my setup and seams to work fine, but I haven't checked the code in detail. Right now I use your code to get comfortable with VPI etc.

umarcor · 2019-10-07T03:07:21Z

I added four new files to the external_buffer example (cp.py, cp.c, tb_extcp_string.vhd and tb_extcp_byte_vector.vhd). Tests are similar to the other set of tbs, but data is copied from the array with eid=0 to the array with eid=1. There was no explicit test for this feature before, only eid=0 was tested.

@kraigher, if you are ok with the status of this PR, we can merge it and move forward.

kraigher · 2019-10-12T16:50:20Z

I am ok with this.

bradleyharden · 2019-10-13T21:07:36Z

Now that this is merged, will string_ptr_pkg be stable? I would like to update #522, but not if I will have to change it again soon.

In that PR, I modify queue_t to store string pointers, rather than characters, and I modify string_ptr_pkg to keep track of old, unused pointer indices in stack.

umarcor · 2019-10-13T21:42:56Z

The second part of this PR is still pending: #476. Here, string_ptr was modified. In #476, the same modifications are applied to integer_vector_ptr. Since #522 modifies both types, I'd suggest to either:

split New queue implementation #522 in two PRs, to have the string_ptr part merged now;
or, wait until [2/3] Add external modes to string_ptr and integer_vector_ptr (through VHPIDIRECT) #476 is merged and do everything at the same time.

Note that there are other PRs in the series (such as #470 and #568). However, any of those are to be evaluated after your contributions. This is because those higher level features might be adapted/improved based on #522.

bradleyharden · 2019-10-13T23:34:29Z

The modifications to integer_vector_ptr were really only to provide consistency between the types. It's not required. I'll probably get it up and running on top of this PR and then update integer_vector_ptr after #476

umarcor force-pushed the ext-strings branch from eb0f15f to 6f527c4 Compare June 3, 2019 19:31

umarcor marked this pull request as ready for review June 3, 2019 19:48

umarcor force-pushed the ext-strings branch from 6f527c4 to dc44958 Compare June 3, 2019 19:53

kraigher requested changes Jun 4, 2019

View reviewed changes

vunit/builtins.py Outdated Show resolved Hide resolved

vunit/builtins.py Outdated Show resolved Hide resolved

vunit/vhdl/data_types/src/byte_vector_ptr_pkg.vhd Show resolved Hide resolved

umarcor force-pushed the ext-strings branch from 966ed00 to 17fecdb Compare June 4, 2019 23:38

kraigher reviewed Jun 5, 2019

View reviewed changes

vunit/builtins.py Show resolved Hide resolved

umarcor force-pushed the ext-strings branch from 3692c2b to 6fb5194 Compare June 5, 2019 19:22

umarcor force-pushed the ext-strings branch 5 times, most recently from 523d5df to 5a27188 Compare June 6, 2019 18:23

kraigher requested changes Jun 11, 2019

View reviewed changes

umarcor force-pushed the ext-strings branch 5 times, most recently from 43e7f86 to 351b301 Compare June 11, 2019 21:27

umarcor force-pushed the ext-strings branch from 351b301 to be60f84 Compare June 12, 2019 00:50

kraigher requested changes Jun 12, 2019

View reviewed changes

examples/vhdl/external_buffer/run.py Outdated Show resolved Hide resolved

examples/vhdl/external_buffer/src/tb_ext_string.vhd Outdated Show resolved Hide resolved

examples/vhdl/external_buffer/run.py Show resolved Hide resolved

umarcor mentioned this pull request Jun 12, 2019

revert renaming 'length' to 'len' #515

Merged

umarcor force-pushed the ext-strings branch 2 times, most recently from 07420e0 to fa5d6ed Compare June 12, 2019 21:28

umarcor force-pushed the ext-strings branch from b175a0a to ed75c87 Compare September 20, 2019 21:34

kraigher reviewed Sep 25, 2019

View reviewed changes

umarcor force-pushed the ext-strings branch 3 times, most recently from 1143bb3 to 614bdf1 Compare September 25, 2019 23:22

umarcor force-pushed the ext-strings branch 4 times, most recently from a2879d0 to fe81505 Compare October 3, 2019 19:04

LarsAsplund approved these changes Oct 5, 2019

View reviewed changes

umarcor added 5 commits October 7, 2019 04:44

feat: add external models to string_ptr

16f101e

add byte_vector_* as an alias of string_*

27c6014

add(examples/vhdl/external_buffer)

860d362

move index_t to types_pkg

8e5095d

examples/vhdl/external_buffer: add cp.py run script and tbs

d9d4b51

umarcor force-pushed the ext-strings branch from fe81505 to d9d4b51 Compare October 7, 2019 02:46

umarcor mentioned this pull request Oct 7, 2019

Do not add builtins by default #559

Closed

eine merged commit 8bf9bed into VUnit:master Oct 13, 2019

umarcor deleted the ext-strings branch October 13, 2019 04:03

umarcor mentioned this pull request Oct 13, 2019

[2/3] Add external modes to string_ptr and integer_vector_ptr (through VHPIDIRECT) #476

Merged

eine mentioned this pull request Oct 24, 2019

Add support for Vivado XSIM #209

Open

eine added the CoSim label Feb 10, 2020

umarcor mentioned this pull request Nov 9, 2021

add 'compile_builtins' deprecation warning #757

Merged

	case mode is
	when internal =>
	st.ids(st.id) := (
	id => st.ptr,
	mode => internal,
	length => 0
	);
	reallocate_ptrs(st.ptrs, st.ptr);
	st.ptrs(st.ptr) := new vec_t'(1 to length => value);
	st.ptr := st.ptr + 1;
	when extacc =>
	st.ids(st.id) := (
	id => st.eptr,
	mode => extacc,
	length => length
	);
	reallocate_eptrs(st.eptrs, st.eptr);
	st.eptrs(st.eptr) := get_ptr(id);
	st.eptr := st.eptr + 1;
	when extfnc =>
	st.ids(st.id) := (
	id => id,
	mode => extfnc,
	length => length
	);
	end case;
	st.id := st.id + 1;
	return st.id-1;

[1/3] Add external models to string_ptr; add byte_vector* as an alias #507

[1/3] Add external models to string_ptr; add byte_vector* as an alias #507

Uh oh!

Conversation

umarcor commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

umarcor commented Jun 3, 2019

Uh oh!

kraigher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

umarcor commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kraigher commented Jun 5, 2019

Uh oh!

umarcor commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kraigher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

umarcor commented Jun 11, 2019

Uh oh!

kraigher commented Jun 12, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

umarcor commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kraigher Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

umarcor Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kraigher Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

umarcor Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

umarcor Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

umarcor Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

umarcor Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

LarsAsplund left a comment

Choose a reason for hiding this comment

Uh oh!

umarcor commented Oct 5, 2019

Uh oh!

go2sh commented Oct 5, 2019

Uh oh!

umarcor commented Oct 7, 2019

Uh oh!

kraigher commented Oct 12, 2019

Uh oh!

bradleyharden commented Oct 13, 2019

umarcor commented Jun 3, 2019 •

edited

Loading

umarcor commented Jun 4, 2019 •

edited

Loading

umarcor commented Jun 5, 2019 •

edited

Loading

umarcor commented Jun 12, 2019 •

edited

Loading

umarcor Sep 25, 2019 •

edited

Loading