Why does this array indexing not work in verilog?

[FvM]

I hate simulating. Usually I start with a simplified version (here I would have tried 1 1-bit-wide bus), make a circuit conected to switches and LEDs and make sure it works.

I agree in so far, that you should primarly know which circuit you want to design. If so, you won't hopefully lose the hardware connection when writing HDL.

The present problem isn't particularly related to simulation, just the difference between what you meaned to code and what you did actually write.

 

[mrflibble]

Debugging tip: if you verify something with hardcoded constants, then make sure that you also verify that case in your testbench.

What I mean is you used 2 as hardcoded address, and verified that that worked. Which is good! But then you don't check 2 in your testbench (not so good). Because as FvM pointed out, 3 is out of bounds for your [0:2] address range of your current ram. Just for the fun of it change that 3 to a 2, and you should get the same result as for your hardcoded version of a while back.

Anyways, that out of bounds problem is less likely if you use the parameterized verion that j_andr posted.

- - - Updated - - -

Or if you want a quick fix, change this in your current code:

// OLD:
reg [2:0] level[2:0];

// NEW
reg [2:0] level[0:7];
//reg [2:0] level[7:0]; // or this if you like a decrementing address range

Personally I try to adhere to [MSB:LSB] ranges for bit vectors, and [MIN:MAX] ranges for addresses. But use whatever you like, as long as you don't confuse yourself.

 

[userx2]

userx2 you've changed data_io from inout to input
in your last attempt, I wonder how you can test the design
with no output ports;

try this version:

 module array_test
  #( parameter DATA_W = 8, ADDR_W = 4, MEM_D = 2**ADDR_W )
  
 (
   input [ADDR_W-1:0] address_in,
   inout [DATA_W-1:0] data_io, //changed input->inout
   input              wr, rd,
   input              rd, m_clock // added
 );

reg [DATA_W-1:0] level[0:MEM_D-1]; 
reg [ADDR_W-1:0] address_latched;
reg [DATA_W-1:0] data_latched;
reg [DATA_W-1:0] data_out;

reg wr_reg, // added
    rd_reg;

always @(posedge m_clock) // added
  begin
    wr_reg <= wr;
    rd_reg <= rd;
  end

//clock master_clock(m_clock);  //instantiate clock //cl = output // commented out

assign data_io = rd_reg ? data_out : {DATA_W{1'bz}}; //added

always @ (posedge m_clock)//slightly changed
	begin
	if (!wr) 
		begin
		data_latched <= data_io;
		address_latched <= address_in;
		end
    else
      if (!wr_reg)
          level[address_latched] <= data_latched;
         
    data_out <= level[address_in];
    
	end
    
endmodule

once you have something working you can do your
experiments;

j.a

Hello I have tried you code version and... wait for it.... It has the same problem.
level[] never changes.

I think I now see what the problem is but I can't code it so please help me.

Where we have
reg [DATA_W-1:0] level[0:3] ;

The compiler seems to assume that the 0:3 are intergers of size 31 bits. When we then write to the level[] with address that is only 3 bits wide, it just does not work.

what we need is this:
reg [DATA_W-1:0] level[3'd0: 3'd3] ; (ignore the exact ranges)

Now I can't translate that into parameters. I have tried all morning.
Here is the non working result:

reg [DATA_W-1:0] level[{ADDR_W{'b0}}: {ADDR_W{'d{LEVELS}}}];

Is there a reference somewhere on how to on how to use all those {} and stuff to make the parameters convert to the right syntax and numbers?

Regards
x

 

[stacksmith]

I can't wait to see how this turns out!

 

[mrflibble]

Hence my pre j_andr "quick fix" code adjustment. But I am sure you are getting the idea now. Make sure your address range and the number of bits for address AND the actual address in your testbench all play nice.

On the testbench ... do you have any control over the testbench content, or was it "magically generated" for you? I ask since you said something to the effect of not knowing where to find it... Anyways, if you can find it, use your oooold code, and change address from 3 to 2, and see what you get.

Or just continue in the current parameterized direction towards a solution.

what we need is this:
reg [DATA_W-1:0] level[3'd0: 3'd3] ; (ignore the exact ranges)

Now I can't translate that into parameters. I have tried all morning.
Here is the non working result:

reg [DATA_W-1:0] level[{ADDR_W{'b0}}: {ADDR_W{'d{LEVELS}}}];

Red herring alert. Ignore the red herring.

The compiler seems to assume that the 0:3 are intergers of size 31 bits. When we then write to the level[] with address that is only 3 bits wide, it just does not work.

Right assumption (give or take a bit), wrong conclusion. You do not need to get funky rewriting your address ranges like that.

reg [15:0] level[0:255];

reg [15:0] level[8'd0:8'd255];

Those 2 should give you the exact same result. That result being a memory 256 deep, with 16 bit words.

- - - Updated - - -

Quick ref for your perusal: http://www.asic-world.com/verilog/memory_fsm1.html

 

[userx2]

Hello

Thanks for helping. I am still pulling my hair our here.

i currently have no control over the testbench apart from what the Wizard imports. I created the stimulus though by driving the clock and values onto the inputs.

I deleted the old code earlier as you folks correctly stated it was terrible.
I have since tried the code as posted earlier by j_and_r as follows:

module bus_interface
	#( parameter ADDR_W= 4,	DATA_W = 3, LEVELS = 4, MEM_D = 2**ADDR_W)
	(
	input [ADDR_W-1:0] address_in,
	inout [DATA_W-1:0] data_io,
	input wr, rd, reset, m_clock
	);
	
	
	reg [DATA_W-1:0] data_latched;
	reg [DATA_W-1:0] data_out;	
	reg [ADDR_W-1:0] address_latched;
	
	
		
	reg [DATA_W-1:0] level[0:15];
	
	
	
	reg wr_reg;
	reg rd_reg;


	assign data_io = (!rd && !reset) ? data_out : {DATA_W{'bz}};


	//clock master_clock(m_clock);  //instantiate clock //cl = output


	always @(posedge m_clock)
		begin
		wr_reg <= wr;
		rd_reg <= rd;
		end

	
	always @(posedge m_clock)
		begin
		if (!wr) 
			begin
			data_latched <= data_io;
			address_latched <= address_in;
			end
		else
		if (!wr_reg)
          level[address_latched] <= data_latched;
         
		data_out <= level[address_in];
    	end
    
endmodule

Note, I have just now removed the paramters from the leve[] declaration and according to your earlier post, it should give me 16 possible levels.

The test bench confirms that it does this but yet again, it only writes values into leve[] when I remove the address_latched and hardcode a number in there.

So overall, I have learnt a lot but still have made no progress towards a working solution.

Regards
X

 

[mrflibble]

I re-read your code 2 times and it really does look correct to me.

The only plausible thing that I can think of is that address_latches doesn't get a sensible value. And assuming the testbench is unchanged compared to your original screenshot, I don't see it.

By way of extra debug, what you can add to your current module is an initial block with some data.

module bus_interface
	#( parameter ADDR_W= 4,	DATA_W = 3, LEVELS = 4, MEM_D = 2**ADDR_W)
	(
	input [ADDR_W-1:0] address_in,
	inout [DATA_W-1:0] data_io,
	input wr, rd, reset, m_clock
	);
	
	
	reg [DATA_W-1:0] data_latched;
	reg [DATA_W-1:0] data_out;	
	reg [ADDR_W-1:0] address_latched;
	
	
		
	reg [DATA_W-1:0] level[0:15];
	
	
	
	reg wr_reg;
	reg rd_reg;


	assign data_io = (!rd && !reset) ? data_out : {DATA_W{'bz}};


	//clock master_clock(m_clock);  //instantiate clock //cl = output

	initial begin
		// some initial data just for the fun of it.
		level[0] <= 3'b101;
		level[1] <= 3'b010;
		level[2] <= 3'b111;
		level[15] <= 3'b001;
	end



	always @(posedge m_clock)
		begin
		wr_reg <= wr;
		rd_reg <= rd;
		end

	
	always @(posedge m_clock)
		begin
		if (!wr) 
			begin
			data_latched <= data_io;
			address_latched <= address_in;
			end
		else
		if (!wr_reg)
          level[address_latched] <= data_latched;
         
		data_out <= level[address_in];
    	end
    
endmodule

That should give you some filled in level data as a sort of sanity check. Last thing: I noticed that reset is an input, but not listed in your simulation. And now that I think some more about it ... how about making data_io an uni-directional input for the time being. You know, purely to exclude that as potential cause.

 

[j_andr]

I have tried you code version and... wait for it....
It has the same problem. level[] never changes.

I did simulations of my example without any change, both rtl and p&r;
screenshot of rtl sim;



j.a

 

[mrflibble]

Nice. And data_io suddenly also looks a lot more reasonable than in the OP's screenshot. What I mean is that in the original screenshot there were no 'Z' parts, which was kinda suspicious... So lets just blame the highly undefined testbench.

 

[userx2]

Hello

First of all, thank you all for helping me with this!!
I did get this working yesterday. It was overall a combonation of problems.
Fist it was the size problems of the registers. Then, after everything, it was testbench problems which I still have not fully overcome but I am working on it.

I now have another question with the same interface code. I now need to use it with other modules.

I have this

module bus_interface
	#( parameter ADDR_W= 4,	DATA_W = 3, CHANNELS = 12)
	(
	output [DATA_W-1:0] level[0:CHANNELS-1],
	input [ADDR_W-1:0] address_in,
	inout [DATA_W-1:0] data_io,
	input wr, rd, reset, m_clock
	);

It keeps giving me a syntax error near
"output [DATA_W-1:0] level[0:CHANNELS-1],"
Can't I use/ pass an array in or out of a module this way or do I need to use a different syntax to do this?

Best regards
X

 

[FvM]

As far I know, two-dimensional vectors aren't supported for Verilog module interfaces. You need to flatten it to 1-D, just a matter of index manipulations in a generate loop. System Verilog can do structured interface objects, also VHDL.

 

[userx2]

As far I know, two-dimensional vectors aren't supported for Verilog module interfaces. You need to flatten it to 1-D, just a matter of index manipulations in a generate loop. System Verilog can do structured interface objects, also VHDL.

Hello

You post was right over my head.

Ok, I I need to flatten the array.

What is a generate loop? My pdf book does not have any reference to that at all.

Could you please provide some sort of eiher example or reference I can go read?

I thought that (I admit this is wrong) this sort of array is already only 1D becaue the bits are just the width of single entries (byte, word...).

Best regards
x

 

[mrflibble]

Purely conceptually you do this: https://www.edaboard.com/threads/265739/#post1140682

Note that that purposefully has absolutely nothing to dowith HDL. Still, for an N-dimensional memory that is all powers of 2, I'd do it that way. For anything other than powers of 2 you can do the generate loop. Or do generate for everything, it's a free world.


For a generate example like FvM is refering to, see this: https://www.asic-world.com/verilog/verilog2k2.html

And it even has a memory example, must be your lucky day! And with even more luck, you'll see why for powers of 2 that generate example amounts to much the same thing as doing it like the 5-dimensional example.

 

[FvM]

What is a generate loop? My pdf book does not have any reference to that at all.

It's the precise term used in the IEEE 1364 specification, so I would expect a serious Verilog text book to know about it. Possibly not an "Easy Verilog" book restricting itself to a subset of the language.

A generate loop is essentially a for loop in continuous code used to instantiate or assign objects multiple times. There are other means to achieve the same. Related to your problem, you'll want to map the two-dimensional object, e.g. array of word to a one-dimensional object permitted as module interface.

 

[userx2]

Hello again

Thanks so much for all the help and suggestions. I read through all the links and I can see what is happening.
However, none of the examples explain how to actually pass such an array from one module to another.
In my example, I need to have the businterfate read and write values into the memory array but I then need the parent module(s) to be able to also use and access the values in that memory.

I have now tried to flatten my array to a 1 dimensional bit array.
Needless to say, I am completely stuck as I can't access the ranges in my bit array. This is because I do not really understand the correct syntax.
I read up about the generate loop but I can't make it work for this.
I also can't make the inital begin set my array to values.
Then, I also can't access the correct bits in the bus read / write.

I guess I could write all my code in 1 single module and then just use the array as befor but that would be cheating.

module bus_interface
	#( parameter ADDR_W= 4,	DATA_W = 3, CHANNELS = 12, MEM_SIZE_BITS = CHANNELS * DATA_W )
	(
	output [MEM_SIZE_BITS-1:0] level,
	input [ADDR_W-1:0] address_in,
	inout [DATA_W-1:0] data_io,
	input wr, rd, reset, m_clock
	);
	
	
	reg [DATA_W-1:0] data_latched;
	reg [DATA_W-1:0] data_out;	
	reg [ADDR_W-1:0] address_latched;
		
	reg [MEM_SIZE_BITS-1:0] level;
	
	reg wr_reg;
	reg rd_reg;


	assign data_io = (!rd && !reset) ? data_out : {DATA_W{'bz}};

	reg [ADDR_W-1:0] i;
	
//**DOES NOt WORK

	initial begin
		for (i=0; i<CHANNELS; i=i+1)
			begin
			level[((i+1)*DATA_W)-1: (i*DATA_W)] = {DATA_W{'b0}};
			end
		end
			
//**Likewise this does not work either"
	genvar i;
	
	generate 
		for (i=0; i<CHANNELS; i=i+1)
			begin
			level[((i+1)*DATA_W)-1: (i*DATA_W)] = {DATA_W{'b0}};
			end
	endgenerate




	always @(posedge m_clock)
		begin
		wr_reg <= wr;
		rd_reg <= rd;
		end

	
	always @(posedge m_clock)
		begin
		if (!wr) 
			begin
			data_latched <= data_io;
			address_latched <= address_in;
			end
		else
		if (!wr_reg)
			begin			
//**DOES NOT WORK EITHER
			level[((address_latched+1)*DATA_W)-1: (address_latched*DATA_W)] <= data_latched;
			end

    	end
    
endmodule

Best regards
X

 

[FvM]

Doesn't work isn't really a clear problem report.

Verilog users will recognize that you are trying a vector part-select syntax, that isn't supported by the language.

The Verilog specification says:

There are two types of part-selects, a constant part-select and an indexed part-select.

You are trying a third kind of variable part-select.

Please review your Verilog text book or previous edaboard discussions about indexed part-select and you'll see how it's supposed to work.

 

[userx2]

Doesn't work isn't really a clear problem report.

Verilog users will recognize that you are trying a vector part-select syntax, that isn't supported by the language.

The Verilog specification says:

You are trying a third kind of variable part-select.

Please review your Verilog text book or previous edaboard discussions about indexed part-select and you'll see how it's supposed to work.

Hello

Ok, my mistake. I was hoping that my mistakes are so obvious that you can all just see that it can't work

I should have stated that the generate loop results in a 'syntax error near ='
The intial begin method states that 'i is not a constant' and so does the level[....] = .....

The examples make it look easy but as soon as one tried to do anything like that, it get really difficult and there is no quick help or way out.

For refernce books, I only have (as of today) the pdf Verilog HDL Quick reference guide by Sutherland and I also have another one from some FPGA Express.

None of them explain this stuff in enough detail so I can actually get it to work.
The usual tutorials are also of very little help.

Maybe there is a better book like Verilog for dummies? :!:

I have tried it like this as well with the same result.

level[((i+1)*DATA_W)-1 -: DATA_W] = {DATA_W{'b0}};

Regards
X

 

[j_andr]

it's not clear to me what you try to achieve,
[why you can't just pass to upper level module
selected by address location in level memory]
but may be the example below will help:

module bus_interface
   #( parameter ADDR_W = 4, DATA_W = 3, CHANNELS = 12, 
                MEM_SIZE_BITS = CHANNELS * DATA_W )
 (
     output reg [MEM_SIZE_BITS-1:0] level,
     input      [DATA_W-1:0]        data_in,
     input      [ADDR_W-1:0]        address_in,
     input                          wr, clk
 );

/* assume address_in = 0 selects bits 2-0 in 'level',
                     = 1         bits 5-3
                     = 2         bits 8-6 etc */


 initial  level = {MEM_SIZE_BITS{1'b0}};

 integer i;    
    
  always @(posedge clk)
    if (!wr)
      for ( i=0;i<DATA_W; i=i+1 )
           level[i + address_in * DATA_W ] <= data_in[i];
    
endmodule

j.a

 

[FvM]

I should have stated that the generate loop results in a 'syntax error near ='
The intial begin method states that 'i is not a constant' and so does the level[....] = .....

Yes, besides the wrong part select syntax the expression in the generate loop is incorrect. Continuous assignments are opened with the keyword assign.

I don't want to comment the lamentation about bad text books. I'm sure there are better ones. At least the Verilog specification describes these things clearly.

Generally speaking, the problem is that you are inventing complex structures but don't manage to translate it into correct Verilog code. There are two options:
- learn the correct Verilog syntax fitting the problem description
- translate the problem into more basic algorithms

E.g. if part select doesn't work for you, use variable bit select instead, accessing source and destination bit by bit.