Critical review of bmp_pkg

[JimLewis]

To avoid conflict with similar packages, you might want to name T_PICFILE something more specific to the package, such as BmpFileType type BmpFileType is file of character;

I use a more verbose naming than the older T_ and R_. I think at this point, we all grew up typing and that verbose names will help a larger community read your code. Sure the people who like the shorter naming will be temporarily annoyed, but they will understand what BmpFileType and BmpRecType are. Then when I create an object of these types, it is just BmpFile and BmpRec. I have to be honest, this is the first time I saw R_ used for a record type - and I have been around a while.

Concatenation is a simple way to join array values. Rather than "+", I suggest that you use concatenation. Also, the name value is used as a VHDL attribute. While it is not a reserved word, it is something that I suggest that you treat as if it were a reserved word. I also suggest that you use the ") is" at the start of a line by itself to separate your procedure interface from the locally declared variables.

   procedure read_word(
      file f         : T_PICFILE;
      variable result: out T_WORD
   ) is
      variable c1,c2 : T_BYTE;
   begin
      read_byte(f,c1);
      read_byte(f,c2);
      result := c2 & c1 ; 
   end procedure read_word;

You don't need an internal variable to slice something. You can use the slice directly in a call. This works here in particular since you used subtypes of std_logic_vector (which is recommended) rather than creating new types. This is your write_word using slicing:

   procedure write_word(
      file f         : T_PICFILE;
      variable A : in  T_WORD
    ) is
   begin
      write_byte(f, A(07 downto 00));
      write_byte(f, A(15 downto 08));
   end write_word;

In a similar fashion, your read_word could have been done as:

   procedure read_word(
      file f         : T_PICFILE;
      variable result: out T_WORD
    ) is
   begin
      read_byte(f, result(07 downto 00));
      read_byte(f, result(15 downto 08));
   end read_word;

[JimLewis]

VHDL aliases work for many things, such as you can replace the PutPixel procedure with:

alias PutPixel is write_pix[T_PICFILE, T_3BYTE];

[Louadria]

Thank you for the comments. However, I got the bmp_pack as a library from contributor Limor Yonatani, so I did not write this code myself. I only adapted the logic for reading in pixels to use multiple data streams.