Why does this array indexing not work in verilog?

[userx2]

Hello
I am trying to get a bus interface working. I have a statement in the code below that is

level[address] = data;

I have address latched = 3'b001 and I can szee the correct value is there.

However, the above does not work when I simulate it. It always produces 0 in the level[1] (or any other).

It immediately works correctly if I code it like this
level[1] = data_in;

Can siomeone please let me know how to get it to work correctly? It may have something to do with how address or level is declared?

More complete sample code below:
-------------------------------------------------------------------

---

Code Verilog - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
module test(address_in, data_io, wr, rd, reset );
    
    
    input [2:0] address_in;
    inout [2:0] data_io;
    input wr, rd, reset;
    
    reg [2:0] data_in;
    reg [2:0] data_out; 
    reg [2:0] address;
    reg [2:0] level[2:0];
 
    reg [3:0] state;
    reg [3:0] last_state;
 
    reg wr_flag;
    reg rd_flag;
    reg last_wr_state;
    reg last_rd_state;
    reg [2:0] rd_state;
        
    wire n_rd;
    assign data_io = (!rd && !reset) ? data_out : 3'bz;
    
    clock master_clock(m_clock);  //instantiate clock //cl = output
    
    
    always @ (posedge m_clock)
        begin
        if (wr != last_wr_state)    //some edge on write line
            begin
            last_wr_state = wr;     //positive edge, latch address and data
            if (wr) 
                begin
                data_in = data_io;
                address = address_in;
                wr_flag = 1;
                end
            end
        else
            begin
            if (wr_flag)
                begin
                wr_flag = 1'b0;
 
 *****THIS FAILS HERE
                                     level[address] = data_in;   //data and address latched earlier
 
**It works like this instead                   level[1] = data_in;
                end
            else if (!rd)
                begin
                if (rd_state <= 3'd3)
                    begin
                    rd_state = rd_state + 1;  //wait some clock cycles for address to stabilize
                    end
                else if (rd_state == 3'd3)      //after n clocks since neg edge on bus, latch internal data to address
                    begin
                    rd_state = rd_state + 1;                    
                    data_out= level[address_in];
                    end
                else
                    begin
                    data_out= data_out;
                    end
                end
            else
                begin               
                state <= state;
                end
            end
        end
endmodule

---

------------------------------

Best regards
X

 

[stacksmith]

Hi. Try to use the <code> tag (# button) around your code, it makes it much easier to read. In fact, I would have to reformat it and indent it to see what's happening... Perhaps you could do it more easily.

A quick question. you have address = address_in , creating a latch. Then you try to do leve[address]=... which is another latch. In another place you use level[address_in], which is what address is latched from. All this happens during the same clock. Is that what you mean to do? It seems like asking for trouble.

 

[mrflibble]

Do you have a very specific reason for mixing blocking and non-blocking assignments like this? Because your current coding style is just waiting for accidents to happen.

Case in point:

data_out= data_out;
end
end
else
begin	
state <= state;

Any particular reason?

Let me put that another way: pick one particular habit and stick with it for at least the always block du jour. Most of the time you'll want to do non-blocking assignments like "state <= state;" .

That, and please use the code or syntax tags. That makes it easier for us to read. And easier reading translates to an increased inclination to actually read your entire code. Right now I read part of it and after that I went pppppfffffrt and entered lazy monkey mode.

- - - Updated - - -

Also, to stay with the FSM theme of your code, check out the top couple of google hits:

http://www.google.com/search?q=sunburst+design+fsm

And by "check out" I mean "read it and adopt those coding standards". ;-)

 

[userx2]

Hello
Thanks for the replies.
Firstly, here is the complete formatted proper code as I last used it. Sorry, I had no idea that the formatting would get lost.

module bus_interface(address_in, data_io, wr, rd, reset );
	
	parameter ADDRESS_WIDTH= 4;
	parameter DATA_WIDTH = 3;
	parameter CHANNELS = 4;
	
	input [ADDRESS_WIDTH-1:0] address_in;
	inout [DATA_WIDTH-1:0] data_io;
	input wr, rd, reset;
	
	reg [DATA_WIDTH-1:0] data_in;
	reg [DATA_WIDTH-1:0] data_out;	
	reg [DATA_WIDTH-1:0] address;
	reg [DATA_WIDTH-1:0] level[CHANNELS-1:0];

	reg [3:0] state;
	reg [3:0] last_state;

	reg wr_flag;
	reg rd_flag;
	reg last_wr_state;
	reg last_rd_state;
	reg [2:0] rd_state;
	
	wire n_rd;
	assign data_io = (!rd && !reset) ? data_out : 3'bz;
	
	clock master_clock(m_clock);  //instantiate clock //cl = output
	
	
	always @ (posedge m_clock)
		begin
		if (reset)
			begin
			wr_flag = 1'b0;
			rd_flag = 1'b0;
			last_wr_state = wr;
			last_rd_state = rd;
			rd_state = 3'd4;
			level[0] = 3'd0;
			level[1] = 3'd1;
			level[2] = 3'd2;
			level[3] = 3'd3;
			address = 3'd0;
			data_in = 3'd0;
//			data_out = 3'bz;
			state = 3'd0;
			address1 = 1;

			end	
		else if (wr != last_wr_state)    //some edge on write line
			begin
			last_wr_state = wr;		//positive edge, latch address and data
			if (wr) 
				begin
				data_in = data_io;
				address = address_in;
				wr_flag = 1;
				end
			end
		else if (rd != last_rd_state)    //some edge on read line
			begin
			last_rd_state = rd;	
			if (!rd)		//negative edge
				begin
				rd_state = 3'd0;
				end
			end
		else
			begin
			if (wr_flag)
				begin
				wr_flag = 1'b0;
**The problem---->		level[address] = data_in;   //data and address latched earlier
				end
			else if (!rd)
				begin
				if (rd_state <= 3'd3)
					begin
					rd_state = rd_state + 1;  //wait some clock cycles for address to stabilize
					end
				else if (rd_state == 3'd3)		//after n clocks since neg edge on bus, latch internal data to address
					begin
					rd_state = rd_state + 1;					
					data_out= level[address_in];
					end
				else
					begin
					data_out= data_out;
					end
				end
			else
				begin				
	//			data_out = 3'bz;
				state <= state;
				end
			end
//				endcase
		end
endmodule

It is like it is because I have been butchering it by trying 100s of changes to try and somehow get it working.
I had everything as blockign before and then I wsa that I don't need it in this case so when things didn't work, I changed it. The whole code / state machine will probably eventuall work differently.

The important thing is that I can learn / figure out why the leve[address] does not work as I am stuck because of that.

Best regards
X

 

[j_andr]

your error is here:

 if (rd_state <= 3'd3)
   begin
	 rd_state <= rd_state + 1;  
   end
  else if (rd_state == 3'd3)
    begin
      rd_state <= rd_state + 1;		
  	  data_out <= level[address_in];
  	end

what you did means:
if
rd_state LESS or EQUAL /.../
else if
rd_state EQUAL
--------------------------------


what I'd change:

reg wr_flag;
always @(posedge clk)
 begin
   wr_flag <= wr
   if (wr && !wr_flag) //positive edge
     level[address_in] = data_io;
 end

if you do not have good reasons to latch data/address
internally it's wasting of logic to create the extra
registers;

probably all your 'read logic' you can simply substitute
by: [if you don't care power consumption too much]
data_out <= level[address_in];

yes, you will have false output sometimes when data
is not expected, but at required timing correct data
is present;

-------

and -of course - follow mrflibble blocking/no blocking suggestions
j.a

 

[stacksmith]

May I suggest that the array error is the least of the problems here.

The reason we have state machines is to avoid writing code like this.

So try the following:
1) Figure out exactly how many states there are.
2) Write code - try using case statements, for each state, how the next state is figured out.
3) Separately from (2), using case statements, write what is done at each state (except anything having to do with setting states).

Make sure you understand the difference between = and <= as it is absolutely crucial - I'd say 90% of understanding verilog

If you do this, you will find that your code will work well, look pretty, and others will be able to figure it out quickly.

 

[mrflibble]

Indeed. I started out a post pointing out a few errors + matters of style etc. But essentially I was rewriting the entire thing so I deleted that post.

There's lots of info on FSMs out there, but if you only have time/patience for one I suggest you read this one:

http://www.sunburst-design.com/papers/CummingsSNUG2000Boston_FSM.pdf

If you adhere to that style, several of your current coding problems magically go away. ;-)

And yes, definitely numero duo:

2) Write code - try using case statements, for each state, how the next state is figured out.

The current if else else else else fest becomes painful real fast. ;-) Using a "proper" FSM should take care of a lot of that.

Oh and I see

//			data_out = 3'bz;

Commenting it out is a good idea! Throwing it away [DELETE] and never looking back on that accident-waiting-to-happen-code is even better! The way you solved it now with the assign data_io = (!rd && !reset) ? data_out : 3'bz; code is indeed the way to go.

 

[j_andr]

stacksmith & mrflibble - userx2 tries to write a code describing just
a simple memory, or rather a register file, I don't think this exercise is a good one
to practise FSM

j.a

 

[mrflibble]

Possibly. It is however the perfect exercise for writing readable & maintainable code. Be that in the form of a FSM or something else. IMO userx2 would do well to at least read some about it, just in case it coincides with what he intends to make.



Edit: I can't spill.

 

[stacksmith]

It seems to have states, although, the way it's written, I can't even look at it, so who knows...

 

[userx2]

Hello all

Looks like you gurus out there can't see the forest because of the trees
I have simplified the code right down to basics and it has exactly the same problem in that the indexing of level[] does not work during simulation.

Here goes:

module array_test(address_in, data_io, wr);
	input [2:0] address_in;
	input [2:0] data_io;
	input wr;

	
	reg [2:0] level[2:0];
	reg [2:0] address_latched;
	reg [2:0] data_latched;


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

	always @ (posedge m_clock)
		begin
		if (!wr)    //write line = low, latch address and data
			begin
			data_latched <= data_io;
			address_latched <= address_in;
			end
		else 
			begin		              //wr line high, write previous address and data to level[x]
Does not work--->>	level[address_latched] <= data_latched;   //data and address latched earlier
			end
		end
	endmodule

By "does not work", I mean that no values in the level array ever change. They just remain X all the time.
However, address_latched data_latched are operating as expected.

The only way I can get values into level[] is I hardcode a index like level [2] <= data_latched .



Regards
X

 

[stacksmith]

Ah, that's beautiful!
One thing that jumps out immediately is your declaration

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

is usually written as

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

Buses are high-low, arrays low-high. I don't have the docs handy, but it might confuse the simulator.

 

[userx2]

Ah, that's beautiful!
One thing that jumps out immediately is your declaration

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

is usually written as

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

Buses are high-low, arrays low-high. I don't have the docs handy, but it might confuse the simulator.

Just tried that, no difference. :sad:

X

 

[mrflibble]

Thank you very much for clearing up the forest. I can see trees now!

A good candidate would be to check out your address_in signal. You are probably generating that with a testbench? What is the value of that during the testbench?

The reason I ask is you said you do get a result when you hardcode it with level[2] <= data_latched . That would mean that at least at that time data_latched has some useful values. So the main suspect would then be this bit.

address_latched <= address_in;

I am assuming that this portion actually gets hit (read: wr is low), but that seems a reasonable assumption for now. What I mean is: for you to check, for me to assume right now since I don't see the rest of your code.

Anyways, since address_latched most probably contains Xs (or U's, Z's depending on your exact error), with your current code that would mean that address_in is X's, which means that you most probably forgot something one level up. Either you did not initialize it, or you forgot to connect it altogether.

And if you do have a testbench and after this you haven't solved it, could you post the testbench code + screenshot of waveform?

 

[userx2]

Thank you very much for clearing up the forest. I can see trees now!

A good candidate would be to check out your address_in signal. You are probably generating that with a testbench? What is the value of that during the testbench?

The reason I ask is you said you do get a result when you hardcode it with level[2] <= data_latched . That would mean that at least at that time data_latched has some useful values. So the main suspect would then be this bit.

address_latched <= address_in;

I am assuming that this portion actually gets hit (read: wr is low), but that seems a reasonable assumption for now. What I mean is: for you to check, for me to assume right now since I don't see the rest of your code.

Anyways, since address_latched most probably contains Xs (or U's, Z's depending on your exact error), with your current code that would mean that address_in is X's, which means that you most probably forgot something one level up. Either you did not initialize it, or you forgot to connect it altogether.

And if you do have a testbench and after this you haven't solved it, could you post the testbench code + screenshot of waveform?

Hello and thanks for the help.
It all looks above board and the address is indeed the correct value according to the simulator waveform.
I have attached a screenshot.
i am currently not clued up enough to locate or find a test bench file. I am only just starting out with this so it is still a steep learning curve and I have been bogged down with this particular issue.

 

[stacksmith]

Some more advice that will probably not help:

Whenever you have a conditional, each leg of the conditional should assign the same destinations. Otherwise, unknown to you, latches and muxes will be inserted into your circuit.

For instance, one part of your "if" statement assigns data_latched and address_latched. The other side does not. So what happens if the condition is false? data_latched and address_latched cannot be assigned, so either an enable will be generated for the register, or some kind of a latch will be added, with a 2-1 mux in front of your register, so the same data can be clocked in. Generally, that is bad, especially if you are not aware of it.

Same goes for your level[address_latched] construct. Normally it would be a mux and a register. But because you are not explicitly setting it for some clocks, some sort of a circuit - probably another mux and a latch will be added.

You should also clearly understand the difference between latches and flip-flops. Your synchronous logic uses flip-flops. A latch is an asynchronous construct that holds the last value stored. It can mean trouble. Your naming conventions - data_latched etc. actually probably mean data_flopped or data_registered...

 

[userx2]

Some more advice that will probably not help:

Whenever you have a conditional, each leg of the conditional should assign the same destinations. Otherwise, unknown to you, latches and muxes will be inserted into your circuit.

For instance, one part of your "if" statement assigns data_latched and address_latched. The other side does not. So what happens if the condition is false? data_latched and address_latched cannot be assigned, so either an enable will be generated for the register, or some kind of a latch will be added, with a 2-1 mux in front of your register, so the same data can be clocked in. Generally, that is bad, especially if you are not aware of it.

Same goes for your level[address_latched] construct. Normally it would be a mux and a register. But because you are not explicitly setting it for some clocks, some sort of a circuit - probably another mux and a latch will be added.

You should also clearly understand the difference between latches and flip-flops. Your synchronous logic uses flip-flops. A latch is an asynchronous construct that holds the last value stored. It can mean trouble. Your naming conventions - data_latched etc. actually probably mean data_flopped or data_registered...

Taken it on board, thank you. Hasen't helped though. Tried it with same result.
Problem is tha tI don't even know if I am doing something wrong or if it is a compiler issue or a simulator issue.:sad:

Regards
X

 

[j_andr]

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

 

[FvM]

In the simulation, you are latching an address value of 3, but level covers an index range of 0 to 2 only. So data is stored to non-existing memory.

 

[ramkumara]

In the simulation, you are latching an address value of 3, but level covers an index range of 0 to 2 only. So data is stored to non-existing memory.

ya, change the index range, it will work....