VHDL coding style question

[barry]

And then when you end up with 256 types of SPI modules you start generalizing again ?

It's been my experience that when you try and make something "generalized", you end up with an unwieldy piece of junk where 50% of it's functionality is never used at any one time. If you partition your design properly, again, using my "SPI Input" module example, you can reuse the input module, output module etc. with NO customization, and just customize the upper levels.

- - - Updated - - -

makes it extremely painful to deal with code that is hidden behind all those submodules

The whole point of having a component (sorry, I only speak VHDL) is that you don't have to worry about any of the lower-level stuff-it's already been vetted. I don't need to know anything about the piston rods in my car in order to drive it.

 

[mrflibble]

Well the fact that most people write code like this:

---

Code Verilog - [expand]
1
2
3
a a_i (xyz, abc, nmo, pqr, c, r);
b b_i (c, r, abc, xyz, hij);
add add_i (hij, xyz, add_out);

---

Yeah, code like that makes me want to grab a baseball bat and invite the coder to go ARK ARK ARK like a cute & cuddly baby seal.

makes it extremely painful to deal with code that is hidden behind all those submodules without even named port mappings and names that s**k on top of everything else. Oh and two lines of comments at the top of the file...name of file (duh I know the file name), and their name (like I would ever put my name in a file full of cr*p ;-))

Well, author name is good. Then you know where to go clubbing. XD

As for port maps of doom, we can only hope that with improved synthesizer support for things like SV interfaces and wildcards you can be spared some of this pain in the future. I had a look at it yesterday and looks like Vivado supports synthesis of these constructs, so that's a start.

 

[shaiko]

mrflibble,

given the SPI example,
please explain to what block you will break your code...

 

[mrflibble]

As always ... it depends. Simplest case it will be just one module. If complex then also submodules.

Let's say as simple example the fpga is an SPI slave with a couple of read/write registers. Then I would just have a single spi module which would mainly contain a FSM to keep track of the transaction. The registers would be boring regs right there in the same module, and the reading and writing would be done on the spot in the FSM.

Do you have a very specific example of SPI thingy for which you want to see the component breakdown?

 

[ads-ee]

The whole point of having a component (sorry, I only speak VHDL) is that you don't have to worry about any of the lower-level stuff-it's already been vetted. I don't need to know anything about the piston rods in my car in order to drive it.

Yeah but does this kind of stuff make sense?

---

Code Verilog - [expand]
1
2
3
4
5
6
7
8
9
10
11
module parity (data, parity);
parameter N = 32;
input wire [N-1:0] data;
output wire parity;
integer i;
reg psum = 0;
for (i=0;i<N;i=i+1) begin
assign psum = data[i] ^ psum;
end
assign parity = psum;
endmodule

---

when it would suffice to just do this...

---

Code Verilog - [expand]
1
assign parity = ^data;

---

I've seen this type of stuff done before. And if we keep the piston rod analogy, then do we want the engine defined as all the individual parts, when all you care about is there is an engine and you can drive the car. Who cares if the engine is described as engine.v and all the inner workings are clumped together as one large description.

e.g. we use always @ (negedge tdc) piston[7:0] = firing[7:0] instead of piston_module piston_module_i (.firing(firing[7:0], .piston(piston[7:0]), .tdc(tdc)); in engine.v.

To me having a file full of tiny instantiated modules makes for less readable code and a larger maintenance burden.

 

[shaiko]

Do you have a very specific example of SPI thingy for which you want to see the component breakdown?

Not really.
The reason, I started this post is because I stumbled upon a UART code that has been designed with:
counters, shift registers, timing clocks, io registration, parity checking all in different modules.

It looked elegant at first...but now I doubt that it's worth the debugging effort. Seems like the designer worked with 6 separate screens when he wrote it.
And also, as he used only synchronous logic (which is a good thing) - to communicate between module, some of the control signals have 4 latency cycles(!!!) till they actually change the state of the module's outputs.

 

[ads-ee]

Well most all code you find on the internet is mostly stuff designed by students/hobbyists that have little if any industry background in designing anything that will have to be maintained in a product or multiple products. The stuff that is done by consulting firms (as examples of their superior skills ;-)) and a number of other professional engineering types is typically a cut above the rest and it's pretty apparent just by the coding standards used and the number of comments.

The break everything down to the minimal divisible function seems to be the typical coding style taught by academia and therefore is what is used by the less experienced coders. I never learned VHDL/Verilog in a school environment, but learned it by reading entire books on the subject and reading code by experienced engineers. Personally I have never wrote code using a gazilion small instantiated blocks. That's sort of analogous to designing a board with SMI (small scale integration) components like 374, 244, 04, and etc. Instead of using FPGAs/uCs etc.

 

[barry]

To me having a file full of tiny instantiated modules makes for less readable code and a larger maintenance burden.

I respectfully totally disagree.

I'd rather see an instantiation of INPUT.VHD which has 3 ports: SERIAL IN, PARALLEL OUT, DATA_READY, READ, than a bunch of registers, state machines, etc. The four lines or so instantiating the module is going to be a lot clearer than 100 lines of stuff implementing the INPUT function. This is even more important if you've got multiple instances of the same component.

 

[ads-ee]

I respectfully totally disagree.

I'd rather see an instantiation of INPUT.VHD which has 3 ports: SERIAL IN, PARALLEL OUT, DATA_READY, READ, than a bunch of registers, state machines, etc. The four lines or so instantiating the module is going to be a lot clearer than 100 lines of stuff implementing the INPUT function. This is even more important if you've got multiple instances of the same component.

I think you've misinterpreted what I've said. The keyword in my statement was tiny. If you have a individual shift register module, data ready module, parallel output module, register module, read module, fsm module (with each state decide in it's own module, yes I had to fix a design like that :shock, etc to implement the 100 lines of stuff, then I have a problem with that. To me any module that is 100-300 lines of code without counting comments is fine with me. Going to the extreme of having every possible register, AND gate, parity, etc as instances in anything is overkill and you might as well go back to designing with schematics and 7400 series components.

You're INPUT.VHD makes perfect sense and is what I would typically do.

 

[mrflibble]

I think it boils down to the complexity of what you are choosing to instantiate versus write inline. If it's 100 lines and used several times in other places as well, of course you put it in a separate module. If it is something as trivial as a shift register or an adder, it's probably better to write the code directly. And the trick as always is the judgement call of how to segment the design when things are not so cut and dried. 1 line versus 100 lines is easy. But 3? 10? And how will the design look in the future?

 

[ads-ee]

If it is something as trivial as a shift register or an adder, it's probably better to write the code directly.

Have you ever picked up code written by someone who did this?

---

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
module nreg (in_d, out_d, enable, reset, clk)
parameter N = 8;
input [N-1:0]  in_d;
output [N-1:0] out_d;
input enable;
input reset;
input clk;
 
wire [N-1:0] in_d;
reg [N-1:0] out_d;
wire enable, reset, clk;
 
always @ (posedge clk or posedge reset) begin
    if (reset)
        begin
            out_d <= {N{1'b0}};
        end
    else if (enable)
        begin
            out_d <= in_d;
        end
    end
endmodule

---

then they use it everywhere in their code, WHY?

---

Code Verilog - [expand]
1
2
3
4
5
6
7
8
9
10
11
nreg #(16) a_reg_i (a_signal, a, 1'b1, 1'b0, clk);
nreg #(16) b_reg_i (b_signal, b, 1'b1, 1'b0, clk);
 
// of course this mux is actually in a separate module too. :P
always @*
    begin
        mux = sel ? a : b;
    end
nreg #(16) mux_reg_i (mux, mux_reg, enable, reset, clk);
 
// Note: I've written the code like they did, not following any reasonable coding readability guidelines or 2001 port declarations.

---

I have unfortunately... :thumbsdown:

 

[mrflibble]

Have you ever picked up code written by someone who did this?

In the past I have downloaded & read code from opencores. Does that answer your question? ;-)

 

[TrickyDicky]

Bad code style (seen in a real comment in real released and sold code):

---

Code VHDL - [expand]
1
--I know this file looks a mess, but it's been bodged together over the years, theres never any time to make it good. It really needed making into a single process/state machine, not the 30 or so single register processes it is now!

---

or my favorite:

-- this seems to work, Im now quite sure why though

 

[mrflibble]

At least they were honest. *grin*

Actually this could also be seen as good coding style, and a signal-embedded-in-codebase to other engineers about the prevalent management style. ;-)

 

[shaiko]

That's what I found in the top entity of a design that's responsible of stabilizing a thermal imaging camrera:

--25.12.08 I tried to rewrite the component so it won't use the external SPI clock as a system clock - but ISIM simulation didn't show good results

 

[barry]

I think you've misinterpreted what I've said. The keyword in my statement was tiny. If you have a individual shift register module, data ready module, parallel output module, register module, read module, fsm module (with each state decide in it's own module, yes I had to fix a design like that :shock, etc to implement the 100 lines of stuff, then I have a problem with that. To me any module that is 100-300 lines of code without counting comments is fine with me. Going to the extreme of having every possible register, AND gate, parity, etc as instances in anything is overkill and you might as well go back to designing with schematics and 7400 series components.

You're INPUT.VHD makes perfect sense and is what I would typically do.

Then I guess we totally agree.

 

[shaiko]

TrickyDicky,

What do you think about the subject ?
What are your rules when in comes to splitting your design into small pieces ?

 

[TrickyDicky]

TrickyDicky,

What do you think about the subject ?
What are your rules when in comes to splitting your design into small pieces ?

Whatever is appropriate so that it is easy for someone else to modify