a dodgy uart, without the r

top.v

`default_nettype none

module top(input CLK, output PIN_24);
	wire tx_buf_empty;
	reg tx_data_ready;
	reg [7:0] tx_data;

	uart_tx #(
		.BAUD_DIVISOR(16000000 / 115200)
	) uart_tx_inst (
		.i_clk(CLK),
		.o_tx(PIN_24),

		.o_tx_buf_empty(tx_buf_empty),
		.i_tx_data_ready(tx_data_ready),
		.i_tx_data(tx_data)
	);

	reg [3:0] ctr = 0;

	always @(*) begin
		if (tx_buf_empty) begin
			if (ctr == 10) begin
				tx_data <= " ";
			end else begin
				tx_data <= "0" + ctr;
			end
			tx_data_ready <= 1;
		end else begin
			tx_data <= 'h00;
			tx_data_ready <= 0;
		end
	end

	always @(posedge CLK) begin
		if (tx_buf_empty) begin
			if (ctr == 10) begin
				ctr <= 0;
			end else begin
				ctr <= ctr + 1;
			end
		end
	end
endmodule

uart_tx.v

`default_nettype none

module uart_tx
# (
	parameter BAUD_DIVISOR = 0,
	parameter N_START_BITS = 1,
	parameter N_DATA_BITS = 8,
	parameter N_STOP_BITS = 1
) (
	input i_clk,
	output o_tx,

	output o_tx_buf_empty,
	input i_tx_data_ready,
	input [N_DATA_BITS-1:0] i_tx_data
);
	localparam SHREG_WIDTH = N_START_BITS + N_DATA_BITS + N_STOP_BITS;
	localparam START_BITS = {N_START_BITS{1'b0}};
	localparam STOP_BITS = {N_STOP_BITS{1'b1}};

	localparam STATE_IDLE = 0;
	localparam STATE_TX = 1;
	reg state = STATE_IDLE;
	reg next_state;

	reg tx;
	assign o_tx = tx;

	reg tx_buf_empty = 1;
	reg [N_DATA_BITS-1:0] tx_buf;

	assign o_tx_buf_empty = tx_buf_empty;

	reg [SHREG_WIDTH-1:0] shreg;
	reg [3:0] num_bits_sent;
	reg [10:0] ctr;

	always @(*) begin
		if (state == STATE_IDLE) begin
			tx <= 1'b1;

			if (!tx_buf_empty) begin
				next_state <= STATE_TX;
			end else begin
				next_state <= STATE_IDLE;
			end
		end else if (state == STATE_TX) begin
			tx <= shreg[0];
			if ((num_bits_sent == SHREG_WIDTH - 1) & (ctr == BAUD_DIVISOR)) begin
				next_state <= STATE_IDLE;
			end else begin
				next_state <= STATE_TX;
			end
		end
	end

	always @(posedge i_clk) begin
		if (i_tx_data_ready & tx_buf_empty) begin
			tx_buf_empty <= 0;
			tx_buf <= i_tx_data;
		end

		if (state == STATE_IDLE) begin
			if (!tx_buf_empty) begin
				tx_buf_empty <= 1;

				shreg <= {STOP_BITS, tx_buf, START_BITS};
				num_bits_sent <= 0;
				ctr <= 0;
			end
		end else if (state == STATE_TX) begin
			if (ctr == BAUD_DIVISOR) begin
				shreg <= {1'b0, shreg[SHREG_WIDTH-1:1]};
				num_bits_sent <= num_bits_sent + 1;
				ctr <= 0;
			end else begin
				ctr <= ctr + 1;
			end
		end

		state <= next_state;
	end
endmodule

Damn can you really not do inline comments on a gist? I will try to format this in a sensible way. All of these are for uart_tx.v

Line 14: input i_tx_data_ready,

• Minor nit: lots of people will find this naming confusing. A flow handshake like this has customarily has one or more of the following signals:
  • *_valid, signalling to the consumer that the producer has data available
  • *_ready, signalling to the producer that the consumer will accept data presented on this cycle

With transfer taking place when both are high. So the naming here is just a little unconventional, and could cause confusion.

Line 21-22:

	localparam STATE_IDLE = 0;
	localparam STATE_TX = 1;

• Nit: this is really a boolean. Instead consider giving state a more descriptive name

Line 23: reg state = STATE_IDLE;

• Initialised registers can lead to portability issues. Some FPGAs only support initialising to 0, and ASIC tools won't know what to do with this. Consider a (synchronous) reset. This goes for all the initialised registers in the design

Line 40: tx <= 1'b1;

• Nonblocking assignments are not generally used in always @ (*) blocks. They are nowhere near as dangerous as blocking assignments in clocked blocks, but can increase simulation time for large designs. Consider using blocking assignments here

Line 49: if ((num_bits_sent == SHREG_WIDTH - 1) & (ctr == BAUD_DIVISOR)) begin

• This will never become true for BAUD_DIVISOR >= 2 ** 11. Consider sizing ctr with e.g. $clog2(BAUD_DIVISOR + 1)
• Similar issue between num_bits_sent and SHREG_WIDTH
• Nit: people often mix up precedence of & and == (although your code is fine). Consider && here

Line 67: shreg <= {STOP_BITS, tx_buf, START_BITS};

• As far as I can tell, data is only loaded into the shift register when transitioning from the IDLE state to the TX state
• This means you enter the IDLE state for one cycle after each byte.
• This loses a tiny (probably negligible) amount of TX performance. More importantly...
• Consider whether the transition to IDLE is strictly necessary, and whether the design could be made cleaner or more orthogonal without it

Line 72: if (ctr == BAUD_DIVISOR) begin

• There are multiple instances of this comparison ctr == BAUD_DIVISOR, one of them in the block where ctr is updated. It requires a little more context to read
• Done this way, all 11 bits of ctr are an input to the logic used in these two blocks. You may want to consider generating a single, registered clk_en signal from the free-running counter, and using this single bit to enable the downstream logic

Thank you so much!!! Really appreciate it and will try and incorporate later :)