patternMinor
Adding two binary-coded-decimal numbers
Viewed 0 times
numbersdecimaladdingtwobinarycoded
Problem
I want to make some VHDL code open source. But before I do, I want to make sure that it is as readable as possible. Things to improve upon could for example be naming and the use of comments.
The code is adding two binary-coded-decimal numbers. The algorithm behind it will come in the documentation. Note that this isn't the default way to add BCD numbers.
```
library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;
entity BCD_adder is
generic(
--BCD_ADD_DEC_SIZE is the amount of decimal digits. e.g. BCD_ADD_DEC_SIZE = 4 means the highest representable integer is 9999
BCD_ADD_DEC_SIZE : integer := 3
);
port(
--input and output ports
BCD_add_a_i, BCD_add_b_i : in STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_sum_o : out STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_clk_i, BCD_add_cin_i : in STD_LOGIC;
BCD_add_cout_o : out STD_LOGIC
);
end entity;
architecture behavioral of BCD_adder is
signal BCD_a, BCD_b : unsigned(4*BCD_ADD_DEC_SIZE downto 0);
signal BCD_sum : STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
signal BCD_cout : STD_LOGIC;
--increment6: a function to increment a 4 bit STD_LOGIC_VECTOR by 6.
function increment6 (number : STD_LOGIC_VECTOR(3 downto 0)) return STD_LOGIC_VECTOR is
begin
return ((number(3) or number(2) or number(1)) & ((number(2) or number(1)) nand (number(2) nand number(1))) & not(number(1)) & number(0));
end function;
--decrement6: a function to decrement a 4 bit STD_LOGIC_VECTOR by 6.
function decrement6 (number : STD_LOGIC_VECTOR(3 downto 0)) return STD_LOGIC_VECTOR is
begin
return ((number(3) and number(2) and number(1)) & (number(2) xor number(1)) & not(number(1)) & number(0));
end function;
begin
process(BCD_add_clk_i)
--BCD_SIZE is the amount of binary digits that are needed for the BCD number. Each decimal digit is 4 bits, so 4*BCD_ADD_DEC_SIZE.
The code is adding two binary-coded-decimal numbers. The algorithm behind it will come in the documentation. Note that this isn't the default way to add BCD numbers.
```
library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;
entity BCD_adder is
generic(
--BCD_ADD_DEC_SIZE is the amount of decimal digits. e.g. BCD_ADD_DEC_SIZE = 4 means the highest representable integer is 9999
BCD_ADD_DEC_SIZE : integer := 3
);
port(
--input and output ports
BCD_add_a_i, BCD_add_b_i : in STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_sum_o : out STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_clk_i, BCD_add_cin_i : in STD_LOGIC;
BCD_add_cout_o : out STD_LOGIC
);
end entity;
architecture behavioral of BCD_adder is
signal BCD_a, BCD_b : unsigned(4*BCD_ADD_DEC_SIZE downto 0);
signal BCD_sum : STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
signal BCD_cout : STD_LOGIC;
--increment6: a function to increment a 4 bit STD_LOGIC_VECTOR by 6.
function increment6 (number : STD_LOGIC_VECTOR(3 downto 0)) return STD_LOGIC_VECTOR is
begin
return ((number(3) or number(2) or number(1)) & ((number(2) or number(1)) nand (number(2) nand number(1))) & not(number(1)) & number(0));
end function;
--decrement6: a function to decrement a 4 bit STD_LOGIC_VECTOR by 6.
function decrement6 (number : STD_LOGIC_VECTOR(3 downto 0)) return STD_LOGIC_VECTOR is
begin
return ((number(3) and number(2) and number(1)) & (number(2) xor number(1)) & not(number(1)) & number(0));
end function;
begin
process(BCD_add_clk_i)
--BCD_SIZE is the amount of binary digits that are needed for the BCD number. Each decimal digit is 4 bits, so 4*BCD_ADD_DEC_SIZE.
Solution
Overall, its not too bad, but I do have some suggestions for readability. What I like to do is add white-space to make elements line up with each other. For example, for this line:
I'd do something like this:
I think it makes large blocks of text like that a smidge easier to read.
For the increment/decrement functions, I'd split up the big or/and/nand statements with variables. It ultimately doesn't affect much synthesis-wise, but again makes a bit easier to read. I'd also be consistent on whether you use the '&' operator or 'and' operator, rather than switching throughout the statement.
For this statement:
I'd find a way to make that a little less ugly, but I'm not strictly sure how. It might have to be one of those necessary evil type of things, but cleaning that up would be really nice. Maybe do the SLV/unsigned typecasting within the increment/decrement functions.
I'm not too fond of the sorta-Hungarian notation thing for ports and signals, but that's more a personal thing than a readability thing.
BCD_add_a_i, BCD_add_b_i : in STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_sum_o : out STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_clk_i, BCD_add_cin_i : in STD_LOGIC;
BCD_add_cout_o : out STD_LOGICI'd do something like this:
BCD_add_a_i : in STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_b_i : in STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_sum_o : out STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_clk_i : in STD_LOGIC;
BCD_add_cin_i : in STD_LOGIC;
BCD_add_cout_o : out STD_LOGICI think it makes large blocks of text like that a smidge easier to read.
For the increment/decrement functions, I'd split up the big or/and/nand statements with variables. It ultimately doesn't affect much synthesis-wise, but again makes a bit easier to read. I'd also be consistent on whether you use the '&' operator or 'and' operator, rather than switching throughout the statement.
For this statement:
b(4*i+3 downto 4*i) := unsigned(increment6(STD_LOGIC_VECTOR(b(4*i+3 downto 4*i))));I'd find a way to make that a little less ugly, but I'm not strictly sure how. It might have to be one of those necessary evil type of things, but cleaning that up would be really nice. Maybe do the SLV/unsigned typecasting within the increment/decrement functions.
I'm not too fond of the sorta-Hungarian notation thing for ports and signals, but that's more a personal thing than a readability thing.
Code Snippets
BCD_add_a_i, BCD_add_b_i : in STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_sum_o : out STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_clk_i, BCD_add_cin_i : in STD_LOGIC;
BCD_add_cout_o : out STD_LOGICBCD_add_a_i : in STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_b_i : in STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_sum_o : out STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_clk_i : in STD_LOGIC;
BCD_add_cin_i : in STD_LOGIC;
BCD_add_cout_o : out STD_LOGICb(4*i+3 downto 4*i) := unsigned(increment6(STD_LOGIC_VECTOR(b(4*i+3 downto 4*i))));Context
StackExchange Code Review Q#139839, answer score: 3
Revisions (0)
No revisions yet.