[sw]: neorv32_uart.c: use common function implementation and align to uart channel via macro

[akaeba]

Hi Stephan,

in the file neorv32_uart.c are for uart0 and uart1 the same mechanics defined, see f.e. neorv32_uart0_disable and neorv32_uart1_disable. The common definition of the register layout in neorv32_uart_t would allow to define a prototype for void neorv32_uart0_disable(void) in the way ```void neorv32_uart_disable(neorv32_uart_t *self) and redefine to original function name space in the neorv32_uart.h. The header would look in the following way:

// prototypes
// https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html
void neorv32_uart_disable(neorv32_uart_t *self);
// UART0 
#define neorv32_uart0_disable(void) neorv32_uart_disable(&NEORV32_UART0)
//UART1
#define neorv32_uart1_disable(void) neorv32_uart_disable(&NEORV32_UART1)

Issued Function could be:

• neorv32_uart0_disable
• neorv32_uart0_enable
• neorv32_uart0_putc
• neorv32_uart0_tx_busy
• neorv32_uart0_getc
• neorv32_uart0_getc_safe
• neorv32_uart0_char_received
• neorv32_uart0_char_received_get
• neorv32_uart0_puts
• neorv32_uart0_printf
• neorv32_uart0_scan

Same for UART1.

For neorv32_uart0_setup i have doubts about the textmode.

What do dou think? If you like i can prepare a PR.

BR,
Andreas

[stnolting]

Hey Andreas!

I know there is so much redundancy in the UART drivers... a relic from the early days of this project... 😅

I like the idea of cleaning up this mess, but I do not understand your proposal. Do you want to add a new argument to all UART functions that is used to pass the actual HW-access struct?? And then uses macros to provide backwards compatibility?

[akaeba]

Hi Stephan,

Do you want to add a new argument to all UART functions that is used to pass the actual HW-access struct??

yes, then you can distinguish between UART0 and UART1 with the given Basepointer

And then uses macros to provide backwards compatibility?

excatly :)

BR,
Andreas

[stnolting]

Thanks for clearing. Sounds good to me!

If you like i can prepare a PR.

That would be great! Looking forward to it 😉

[akaeba]

Perfect, stay tuned to my PR :)

[akaeba]

Hi Stephan, see here the start of the changes.

main...akaeba:neorv32:common_neorv32_uart_functions

At the moment i stumble a little with the functions:

• neorv32_uart0_setup
• neorv32_uart1_setup

The issue to distinguish between UART0_SIM_MODE and UART1_SIM_MODE

BR,
Andreas

[stnolting]

Looks good so far! 👍

The issue to distinguish between UART0_SIM_MODE and UART1_SIM_MODE

Simulation mode is not the default case, so it should be OK to add some program overhead here to check for the actual UART module being setup:

// setup UART0
// ...
uint32_t sim_mode = 0;
#ifdef UART0_SIM_MODE
#warning UART0_SIM_MODE (primary UART) enabled! Sending all UART0.TX data to text.io simulation output instead of real UART0 transmitter. Use this for simulations only!
  if ((uint32_t(&self) == NEORV32_UART0_BASE) then {
    sim_mode = 1 << UART_CTRL_SIM_MODE;
  }
#endif
// ...

// setup UART1
// ...
uint32_t sim_mode = 0;
#ifdef UART1_SIM_MODE
#warning UART1_SIM_MODE (secondary UART) enabled! Sending all UART1.TX data to text.io simulation output instead of real UART1 transmitter. Use this for simulations only!
  if ((uint32_t(&self) == NEORV32_UART1_BASE) then {
    sim_mode = 1 << UART_CTRL_SIM_MODE;
  }
#endif
// ...

By the way, why do name the hardware handle self? I know this comes from some self-referencing c++ class, but I never really got into object oriented programming... 😅

[emb4fun]

Hello,

if you look at how this was done in the ST library, then the definitions for the NEORV32 UART would look like this:

#define __IO	volatile

typedef struct
{
   __IO uint32_t CTRL;
   __IO uint32_t DATA;
} UART_TypeDef;

#define UART0_BASE	0xFFFFFFA0
#define UART1_BASE 	0xFFFFFFD0

#define UART0          	((UART_TypeDef *) UART0_BASE)
#define UART1          	((UART_TypeDef *) UART1_BASE)

And the prototypes could look like:

void neorv32_setup (UART_TypeDef* UARTx, uint32_t baudrate, uint8_t parity, uint8_t flow_con);

And for the compatibility:

#define neorv32_uart0_setup(_a,_b,_c) neorv32_setup(UART0,_a,_b,_c)

And a call with the new setup for UART0 will look like:

neorv32_setup(UART0, 19200, PARITY_NONE, FLOW_CONTROL_NONE);

And at the same time, the old way will work too:

neorv32_uart0_setup(19200, PARITY_NONE, FLOW_CONTROL_NONE);

Best regards,
Michael

[akaeba]

Hi Michael,

thanks for providing all the comments. The point with the different register definition is the backward compatibility. There would be a lot of change inside the User programs as well in the framework itself. Therefore i dont know.

@stnolting: What do you think?

[akaeba]

@emb4fun: semicolons are removed main...akaeba:neorv32:common_neorv32_uart_functions

[emb4fun]

Hello Andreas,

The point with the different register definition is the backward compatibility. There would be a lot of change inside the User programs as well in the framework itself. Therefore i dont know.

I don't believe that this will result in changes in existing programs.
I'll take a look and will test it here too with your branch and some small changes.

Best regards,
Michael

[stnolting]

@emb4fun

#define __IO	volatile

typedef struct
{
   __IO uint32_t CTRL;
   __IO uint32_t DATA;
} UART_TypeDef;

#define UART0_BASE	0xFFFFFFA0
#define UART1_BASE 	0xFFFFFFD0

#define UART0          	((UART_TypeDef *) UART0_BASE)
#define UART1          	((UART_TypeDef *) UART1_BASE)

Basically, this is current state, right?

void neorv32_setup (UART_TypeDef* UARTx, uint32_t baudrate, uint8_t parity, uint8_t flow_con);

This looks good to me.

@akaeba

@stnolting: What do you think?

I'm not sure. From a functional point of view both of your proposals look identical... 😉
How about you start a PR and then we can further elaborate on this?

[akaeba]

PR done, let's elaborate after merge.

[akaeba]

@emb4fun : let us work together on this PR.

As next I introduce Stephans recommendation to move the c macros to the legacy.h function.