My first FPGA code

[lawrence11]

Does anybody know why this doesnt work, I took all day to see if I can debug this, it seems I am missing something, there seems to be something after my first always block.
Its a simple spi buffer that stores in RAM bits and resets not to overflow.
My goal is to compile, wich I cant. And then maybe make a test bench, and mess around in modelsim.

This is my code, how off am I in my understanding of verilog?

EDIT: Ok, this cpi_rcv.v has no warning, the other warning are just some stuff about top level. I will solve that on my own.

Can anybody generally comment on my comments and add some philosophy?

I must say this about verilog, I feel like I dont know how to start and am randomly doing things. I chose to learn verilog but I think I was more a VHDL kind of person.

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

Code: [Select]

module  spi_rcv (some_magnitude,some_string); // The way I imagine it These bits should be available for another block with more complex stuff.
                                                                        // I am trying to stay with the youtube lecture of Kirk Weedman and stay organized                                                                                 
 

                        input wire spi_clk; 
input wire spi_mosi;
input wire ram_de; // ram data eneable
reg [7:0] i; // some counter, for bit shift, will this be optimized away?
             //I  have visions of a snowplow, plowing away ram bits in sequence, there is no counter in this vision.
reg some_magnitude [7:0]; // lets make this interresting, Once ram_de is pressed there is a special 8 bytes in the beginning that is processed differently
reg some_string [87:0]  // there can be many strings later


// godtoplevel spi_rcv_inst(.spi_clk(spi_clk),.spi_mosi(spi_mosi),.ram_de(ram_de)); ?? IS THIS REALLY NECESSARY??? SO ANNOYING

         always @(posedge ram_de) begin

         i='b00000000;
     

          end
   


     

         always @(negedge spi_clk) begin // yeah I use negative, because my pin is a neg edge signal, do I GAIN EFFICIENCY in LUTS by doing negedge things with negedge pins??
       

            if (ram_de == 1'b1) begin

        if(i<=7) begin
         some_magnitude[i]=spi_mosi; //  Some document say I should use non blocking assignement, that its ok to do that.
         i=i +1;                    //  should I use blocking assignement for line above?, dont wanna skip a RAM bitlocation             
         end
 
      else if(i>7 & i<=87) begin
some_string[i-8]=spi_mosi;
i=i+1;
end

else if (i>87) begin
i=0;
end

end
end

endmodule
                       

[jhpadjustable]

Try again. Use code tags so that

Code: [Select]

[i] doesn't get eaten.

[lawrence11]

Here is my contribution, you cant say I'm a lazy bastard.

This is the screen grabs I took manually from Kirk Wedman's lectures in pdf format, this goes with the Kirk Weedman 1-10 video lecture.

I bookmarked them in my Ipad1 , goodreader. Is Ipad1 still a thing?

http://www.filehost.pt/ay5u~s

[Yansi]

Again and again and again.

Upload images directly to the forum! Pleeeease.

Your link is either fucked up, or the website is fucked up, or likely both. It is showing just some irrelevant garbage graphs together with some warnings about "no content for my country".  (ノಠ益ಠ)ノ彡┻━┻

[lawrence11]

its 37 megs

Its 101 pages of manual screen grabs over 1month period. with annotated notes.

Its notes that go with the video lecture.

[0culus]

I don't have any specific advice for your verilog, but the one thing that made my experience with hardware description language orders of magnitude easier is the realize that it has more in common with wiring up a circuit on the bench than it does with coding. Of course, you do need to make sure you get the syntax right, but outside of that troubleshooting becomes much clearer when you think if it in terms of a literal circuit you are wiring up.

[lawrence11]

Ok thanks, I have ths image wich helps me alot.

Its also about magining little box subsystems and not make useless connections everywhere.

[lawrence11]

found it! , still have warnings about top level tho. I looked line by line for like 2 hours and never saw it, dumb me. The warning was "always".... something.

here it was, can you see it? I thought there was a problem with using two always block, the warning tricked me and also my poor attention to detail.

                        reg some_magnitude [7:0]; // lets make this interresting, Once ram_de is pressed there is a special 8 bytes in the beginning that is processed differently
         reg some_string [87:0]  // there can be many strings later

Now that spi_rcv.v compiles without warning, Can somebody review my code and answer my philosophical questions?

[ejeffrey]

What is it with verilog that makes people unable to indent properly?

Your code is actually better than a lot of I have seen, but that isn't saying a lot.  Please use consistent and correct indentation!

[lawrence11]

Ok, thanks.

I actually havent coded in a long long time, I have been so busy with the usual: general research on my schematic+ component research+ PCB design + all the nice things  with PCB assembly.

Now I am coming back to coding, it almost feels like vacaction time, but I guess soon enough I will be back at it 

[lawrence11]

What is it with verilog that makes people unable to indent properly?

Your code is actually better than a lot of I have seen, but that isn't saying a lot.  Please use consistent and correct indentation!

Maybe I was more a VHDL type of guy?

Too late now tho... I hear SV is the futur.

I tried to learn "the best language". I dont got enough RAM to learn both VHDL and Verilog.

I know C a bit and now verilog a bit, both are supposed to be complementary to eachother.

[jhpadjustable]

No, reg i probably isn't going to get optimized away completely, though the synthesizer might discover that bit 7 is "stuck at 0" and get rid of it.

Philosophically, prefer synchronous designs. Calculate the next state depending on current inputs and let the clock make it the next current state in its time. All non-blocking assignments that take place in tick x are held until all the other tasks scheduled to run in that tick have completed, so that you don't have to worry about ordering statements to avoid it.

Rather than switch between two registers of 88 and 8 bits, you could create one 96-bit register and bundle it into wires to split it up, like:

Code: [Select]

wire[7:0] some_magnitude;
wire[87:0] some_string;
reg[95:0] allmybits;
assign {some_string, some_magnitude} = allmybits;
The clearer and more straightforward your code is, the more likely that the optimizer will see viable optimizations.

Inverters are basically free. For clarity, it is preferable to keep your signals in the true sense (not inverted) within the design and invert at the pins or the edge of the module where necessary. Where you do use inverted-sense signals, it is strongly recommended for your own sanity that you note it in the name with a prefix or suffix.

[lawrence11]

No, reg i probably isn't going to get optimized away completely, though the synthesizer might discover that bit 7 is "stuck at 0" and get rid of it. Is there a bug in my code? I always fuck these >= relationships up ROFL

Philosophically, prefer synchronous designs. Calculate the next state depending on current inputs and let the clock make it the next current state in its time. All non-blocking assignments that take place in tick x are held until all the other tasks scheduled to run in that tick have completed, so that you don't have to worry about ordering statements to avoid it. so I did it right in my case? This is a fool proof way? I was just imagining "timing issues" in my own mind, so felt like putting in a => instead

Rather than switch between two registers of 88 and 8 bits, you could create one 96-bit register and bundle it into wires to split it up, like:

Code: [Select]

wire[7:0] some_magnitude;
wire[87:0] some_string;
reg[95:0] allmybits;
assign {some_string, some_magnitude} = allmybits;
The clearer and more straightforward your code is, the more likely that the optimizer will see viable optimizations.   Hmm interresting, I will pay attentin to your philosophy

Inverters are basically free. For clarity, it is preferable to keep your signals in the true sense (not inverted) within the design and invert at the pins or the edge of the module where necessary. Where you do use inverted-sense signals, it is strongly recommended for your own sanity that you note it in the name with a prefix or suffix. I have no idea what you just said, give me an example of a bad program and a bad pin selection?, my SPI on the mcu has an inverting hardware bit, I connected the neg to spi_clk like a monkey would put the square in the square, I know the mcu will adapt to whats bets for the FPGA, not the ther way arund, anyways its not that high a frequency.

[asmi]

Never use blocking assignments inside clocked blocks, and non-blocking - in non-clocked (combinatorial) blocks!
SystemVerilog allows to make it more explicit with always_ff and always_comb blocks respectively, and will even issue a warning if the code in always_comb block is synthesized into non-combinatorial logic! This warning saved me many-many times from unintentional latches!

[lawrence11]

Never use blocking assignments inside clocked blocks, and non-blocking - in non-clocked (combinatorial) blocks!
SystemVerilog allows to make it more explicit with always_ff and always_comb blocks respectively, and will even issue a warning if the code in always_comb block is synthesized into non-combinatorial logic! This warning saved me many-many times from unintentional latches!

The issue is not really blocking assignment for a beginner. The issue is that a beginner thinks that there might be a data fuckup since these are not happening one after the other but at the same exact same time and by the inter-relationship of assignements, you anticipate bugs wich are not happening. Because its hard to visualize the data moving, but if you say its safe, and never do that, its good enough for me.

wOW ASMI, YOU SOUND SO SMART, CAN WE BE FRIENDS? I'LL MESSAGE YOU EVERYDAY, I'M BUILDING SOMETHING COOL I'LL ONLY ASK YOU SIMPLE QUESTIONS. I'LL ALWAYS REMEMBER YOU

[lawrence11]

Rather than switch between two registers of 88 and 8 bits, you could create one 96-bit register and bundle it into wires to split it up, like:

Code: [Select]

wire[7:0] some_magnitude;
wire[87:0] some_string;
reg[95:0] allmybits;
assign {some_string, some_magnitude} = allmybits;
The clearer and more straightforward your code is, the more likely that the optimizer will see viable optimizations.

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

I could do that but there always 2 or 3 ways to skin a cat and most the the time its best to jsut do it with what seems natural to you and move on because it will be compiled to the same. I looked serial to RAM verilog code and most use a counter


What if I wanted to add more than just one 88 bit register, like 8 bits deep? 64 bits deep?

reg [87:0] some_strings [7:0];

[hamster_nz]

To help me out I reformatted the indentation... here is he reformatted code:

Code: [Select]

module  spi_rcv (
some_magnitude,
        some_string
);
// The way I imagine it These bits should be available for another block with more complex stuff.
// I am trying to stay with the youtube lecture of Kirk Weedman and stay organized                                                                                 
 
input wire spi_clk;
input wire spi_mosi;
input wire ram_de; // ram data eneable
reg [7:0] i;       // some counter, for bit shift, will this be optimized away?
                   // I  have visions of a snowplow, plowing away ram bits in sequence, there is no counter in this vision.
reg some_magnitude [7:0]; // lets make this interesting, Once ram_de is pressed there is a special 8 bytes in the beginning that is processed differently
reg some_string [87:0]  // there can be many strings later

// godtoplevel spi_rcv_inst(.spi_clk(spi_clk),.spi_mosi(spi_mosi),.ram_de(ram_de)); ?? IS THIS REALLY NECESSARY??? SO ANNOYING

always @(posedge ram_de) begin
  i='b00000000;
end

always @(negedge spi_clk) begin // yeah I use negative, because my pin is a neg edge signal, do I GAIN EFFICIENCY in LUTS by doing negedge things with negedge pins??
  if (ram_de == 1'b1) begin
     if(i<=7) begin
        some_magnitude[i] = spi_mosi; //  Some document say I should use non blocking assignment, that its ok to do that.
        i = i+1;                    //  should I use blocking assignment for line above?, don't wanna skip a RAM bitlocation             
     end else if(i>7 & i<=87) begin
some_string[i-8] = spi_mosi;
i = i+1;
     end  else if (i>87) begin
i = 0;
     end
  end
end
endmodule

At least one issue is that 'i' is updated on the posedge of ram_de, and also on the negedge of spi_clk. This will not work as Flipflops only have one clock signal, and can only be triggered by either a negative or positive edge. It will work in simulation but not in H/W

Adapting this to use shift registers rather than counters is the way to go IMO. Indexing arrays indicates a 1:n MUX, which is expensive for large values of 'n'.

Count the number of cycles that ram_de is asserted, and when you reach 92 move the data from the shift register into the outputs. Also as the benefit that the update is 'atomic' (all bits change at the same time, not one by one).

[jhpadjustable]

Is there a bug in my code? I always fuck these >= relationships up ROFL

No, no bug in your code. Your comment asked whether i would get optimized away. The answer is not completely, but individual bits might.

so I did it right in my case? This is a fool proof way? I was just imagining "timing issues" in my own mind, so felt like putting in a => instead

Surely you meant <=. No, you didn't. asmi spelled out the rule better than I could at the time. Heed that advice, especially if you're worried about keeping your LUTs down.

Inverters are basically free.

As in, inverters will be mapped into the LUTs or IOBs where needed so you don't need to concern yourself too much with them. Your FPGA's user manual should have a drawing of all the steering muxes and other config-driven logic within each logic element.

For clarity, it is preferable to keep your signals in the true sense (not inverted) within the design and invert at the pins or the edge of the module where necessary.

As a general rule, avoid active-low signals in the guts of your design. Anything that should be active-low externally should be inverted in the top file.

Where you do use inverted-sense signals, it is strongly recommended for your own sanity that you note it in the name with a prefix or suffix.

Such as _n, e.g. spibus_memss_n or host_irq_out_n:

Code: [Select]

assign spibus_memss = ~in_spibus_memss_n;
assign out_host_irq_n = ~host_irq;


my SPI on the mcu has an inverting hardware bit, I connected the neg to spi_clk like a monkey would put the square in the square, I know the mcu will adapt to whats bets for the FPGA, not the ther way arund, anyways its not that high a frequency.

SPI is a bit of a special case, since MISO and MOSI are updated on opposite edges of the same clock pulse. You could invert the slave select at the edge of the design as above, or not. When you do larger designs with lots of emable and other signals flying around, consistent use of positive logic will help understanding. For now, do whatever works for you.

As for

Code: [Select]

reg [87:0] some_string [7:0]; fair enough.

[lawrence11]

To help me out I reformatted the indentation... here is he reformatted code:

Code: [Select]

module  spi_rcv (
some_magnitude,
        some_string
);
// The way I imagine it These bits should be available for another block with more complex stuff.
// I am trying to stay with the youtube lecture of Kirk Weedman and stay organized                                                                                 
 
input wire spi_clk;
input wire spi_mosi;
input wire ram_de; // ram data eneable
reg [7:0] i;       // some counter, for bit shift, will this be optimized away?
                   // I  have visions of a snowplow, plowing away ram bits in sequence, there is no counter in this vision.
reg some_magnitude [7:0]; // lets make this interesting, Once ram_de is pressed there is a special 8 bytes in the beginning that is processed differently
reg some_string [87:0]  // there can be many strings later

// godtoplevel spi_rcv_inst(.spi_clk(spi_clk),.spi_mosi(spi_mosi),.ram_de(ram_de)); ?? IS THIS REALLY NECESSARY??? SO ANNOYING

always @(posedge ram_de) begin
  i='b00000000;
end

always @(negedge spi_clk) begin // yeah I use negative, because my pin is a neg edge signal, do I GAIN EFFICIENCY in LUTS by doing negedge things with negedge pins??
  if (ram_de == 1'b1) begin
     if(i<=7) begin
        some_magnitude[i] = spi_mosi; //  Some document say I should use non blocking assignment, that its ok to do that.
        i = i+1;                    //  should I use blocking assignment for line above?, don't wanna skip a RAM bitlocation             
     end else if(i>7 & i<=87) begin
some_string[i-8] = spi_mosi;
i = i+1;
     end  else if (i>87) begin
i = 0;
     end
  end
end
endmodule

At least one issue is that 'i' is updated on the posedge of ram_de, and also on the negedge of spi_clk. This will not work as Flipflops only have one clock signal, and can only be triggered by either a negative or positive edge. It will work in simulation but not in H/W

Adapting this to use shift registers rather than counters is the way to go IMO. Indexing arrays indicates a 1:n MUX, which is expensive for large values of 'n'.

Count the number of cycles that ram_de is asserted, and when you reach 92 move the data from the shift register into the outputs. Also as the benefit that the update is 'atomic' (all bits change at the same time, not one by one).

Dam, I dont know what you mean, but it sounds really good, so a quick solution would be to have the ram_de to be @negedge as well? Basically making this reset a wired OR scenario inside the fpga?

So whats more important, to stick the negedge SPI because spi_clk is connected to a negedge pin on the PCB, or make this a strictly @ posedge thing?

I was also imagining an ever expanding need for parallel connections, but what I was really thinking was : Snowplow!

Umm, can you code it for me? Pretty please? I'm trying to learn, honestly... But doggy eyes

[lawrence11]

So whats more important/better/more efficient, to stick to  the negedge SPI because spi_clk from MCU is connected to a negedge pin on the PCB, or make this a strictly @ posedge thing?

I wanted "efficiency". I saw a quare, I put the square in the square.

[hamster_nz]

Umm, can you code it for me? Pretty please? I'm trying to learn, honestly... But doggy eyes

Doggy eyes! oh no! Must help!

I'm completely away from anywhere I can test this so hopefully the idea comes across. Also Verilog is not my native HDL, and it has been many months.

Most likely bugs

- I shift data into to the shift register assuming it is MSB on the wire first. I am most likely wrong. If this is true, then flip:

Code: [Select]

shift_reg <= {shift_reg[94:0] &  spi_mosi};
to

Code: [Select]

shift_reg <=spi_mosi & shift_reg[95:1]};
You will also need to assign the correct bits to your output.

- There are setup time requirements for ram_de and spi_mosi - the logic in the FPGA will be working on a fractionally delayed signal (because of delays inside the FPGA), so don't expect it will work faster than a few 10s of MHz.

- Simulate to pick up the many "fence post errors" (off-by-one errors) that I haven't thought through.

- Has plenty of extra bits that will generate "not needed" warnings (e.g. bit 7 of 'received_bits')

Code: [Select]

module  spi_rcv (
some_magnitude,
        some_string
);
                                                       
 
input wire spi_clk;
input wire spi_mosi;
input wire ram_de;
reg [7:0] received_bits;

reg shift_reg [95:0];
reg some_magnitude [7:0];
reg some_string [87:0];

always @(negedge spi_clk) begin

  // Are we receiving the last bit?
  if (ram_de == 1'b0) begin  // Double check nobody drops ram_de on the last bit!
     if(received_bits == 95)  begin
      some_magnitude <= shift_reg[94:86];
      some_string    <= shift_reg[94:0] &  spi_mosi};
     end
  end

  // Shift into the shift register, no matter what
  shift_reg <= {shift_reg[94:0] &  spi_mosi}

  // Count the number of valid bits in the shift register
  if (ram_de == 1'b0) begin
     received_bits <= 8'b0;
  end else if (received_bits != 255)  begin
     received_bits  <= received_bits+1;
  end
end
endmodule


[lawrence11]

Wow thanks.

Do you agree with this statement? Snowplow!

[hamster_nz]

Wow thanks.

Do you agree with this statement? Snowplow!

I only hear a whooshing sound as that statement flies by...

Oh I get it, maybe it is more like snow building up on a roof, that then slides off just as you close the door....

[lawrence11]

Ok heres some of my code, I think I was mistakelnly thinking I can just press ram_de, and restart from zero, as long as ram_de was pressed, while not disturbing the rest of the bits that were
further than the bytes I decided to write in that  time period. But this is non-sensical, if I want this to be efficient in luts, I cant have it both ways, the bits must be pushing themelves, thus affected by the last bit. If I wanna place  bit in the 187th bit, I gotta think 187 moves ahead, and If I want to change it, they all have to get flushed either by writing or a reset.

I mean... The ram allocating mechanism is not some little robot on tracks that goes up 1 bitram when you say +1, and deposits the data like in the amazon factory, this is not how it works.

As such, the word "ram_de" will be changed to "spi_reg_reset". The counter is no longer there, is it seems like its use is redundant  from my new POV, the data must be reset, not a counter, my new way of looking at the world .

Here is the present code, as I am writing this, new questions arise in my brain, when I compare this particular example, I am still perplexed as to how I would adapt this "wire mentality"
into my particular case, I must admit I am not too comfortable with the rules of concatenation and if I am doing this right.

Please help me understand WTF I am doing.

Edit: I also notice in his code, he resets the data in the register with a <= block assignement. I dont in my code due to the rule of not having blocking assignements in clocked blocks.

http://referencedesigner.com/tutorials/verilog/verilog_32.php

Code: [Select]

module  spi_rcv (important_byte,some_connection); // Should I seperate/concatenate now or later? Where should boundary be defined?

         input wire spi_clk; 
input wire spi_mosi;
input wire reset_spi_reg; //
//reg [7:0] spi_limit_switch; dont think this is really necessary, if I can reset all I can get known state.
wire [7:0]important_byte; // I seperate and send out all my bits via wires, not another reg, got it.             
wire [n-8:0]some_connection; // this good?
reg [n-1:0]some_string;  //
   parameter n=184;


// godtoplevel spi_rcv_inst(.spi_clk(spi_clk),.spi_mosi(spi_mosi),.ram_de(ram_de)); ?? IS THIS REALLY NECESSARY??? SO ANNOYING


always @(posedge reset_spi_reg) begin

         some_string=0;
assign{some_connection,important_byte}=some_string;
     

         end
   

     

         always @(negedge spi_clk) begin
// yeah I use negative, because my pin is a neg edge signal, do I GAIN EFFICIENCY in LUTS?
//doing negedge things with negedge pins?? because if its bad I'll just use posedge
         

            if (reset_spi_reg == 0) begin //
some_string<={some_connection,important_byte};// Ok, we are using blocking assignement+ a concatenation.
                              //how to adapt this line to my situation?
                             //I read this is bad, so is this an exeption, due to chain nature??

assign {some_connection,important_byte}={spi_mosi, some_string[n-1:1]};// Hmm, again, how to adapt this line
                                                                       // to my situation?

end


                  end



[hamster_nz]

You need to go back to basics, and describe succinctly what you are trying to do. In your current code you don't use spi_mosi at all, so I am not sure it acutally does anything

Your questions in image..

> "Is his[or her] reset technique superior to mine?"

Yes. It is the design pattern that the synthesis tools are expected to see. I am not sure if the 'negedge reset' is correct. Resets are usually level sensitive not edge sensitive - otherwise how can you hold the design in reset, because an edge only happens at a point in time.

> "Do I need to do this?"

Looks weird to me  - I  have never seen the concatenation on the left hand side of the equals. But it only depends on what you want your reset state needs to be.

> "Yeah OK, but I got 2 separate data types..."

Verilog is a loosely typed language, and everything is bits and wires in the H/W, so you only have one datatype. In the HDL are describing how the bits flow in the hardware, What those bits acutally mean (i.e. the datatype) is all in the eyes of the designer. The HDL is pretty 'meh' on everything to do with data types.

The the problem I see is that you don't seem to have a clear idea of the end result you are trying to achieve (which is OK, because you are building up experience and learning as you go)

Here is how I would describe what I assume you are trying to do:

I want to have something that has three inputs:
  - spi_clk  - the clock for data transfer
  - spi_mosi - the serial data line. Is stable during the falling edge of SPI_CLK
  - reset_spi_reg - An active low signal indicating that transfer is in progress.

It is assumed that 'reset_spi_reg' will be lowered while SPI_CLK is high, and then 96 bits will be transferred into the design, each bit transferred on the falling edge of SPI_CLK. The data transfer is completed when the 96th bit is received, and the outputs are updated. Additional bits are ignored until RESET_SPI_REG us raised. If a transfer is less than 96 bits, all data is ignored.

The outputs for the design are:
  - important_byte[7:0] - bits - These are the last 8 bits of the 96-bit transfer. (need to define bit ordering here)
  - some_string[87:0]  - These are the first 88 bits of the 96-bit transfer. (also need to define bit ordering here)

They will be updated all at once, when the 95th bit is clocked into the design.

The 'reset_spi_reg' only controls the internal state of the design - the outputs will remain stable (and not be reset) if this is toggled.

Is any of that wrong? Do you disagree with anything in that spec?

PS. please check the formatting of your code you post. Your indentation is really hard to read.