UART: setting baudrate, stopbits, and parity on runtime

[geith]

added UART functions for additional runtime configuration an TX buffer flushing (only implemented for cc2538)

[haukepetersen]

I don't like this:

a) baudrate configuration: There are not many use cases, where one would like to re-configure this settings often. If this is needed, one can always re-call uart_init again -> cleaner interface
b) parity/stop: so far we did not include this into the interface, as these are used very rarely. If really needed, I would prefer to configure them using the board configuration in periph_conf.h
c) what is the use case for the flush function? IMHO the uart_send function should be self-contained, i.e. making sure it won't override any ongoing transmission when called and/or making sure the given byte(s) are send out before returning -> no need for flush -> cleaner interface

[geith]

I have a specific use case for this API extension: a Modbus RTU master with a RS-485 transceiver.

I need different parity setting (depending on the slave devices) and the flush function is essential too. Because the bus works in half-duplex mode, I need to change the transceiver direction manually (using a GPIO pin). For this, I need to guarantee, that the last byte has been sent to the bus before switching the direction.

I agree, that the baudrate configuration is a code duplication; my intention was to separate the baudrate reconfiguration from dangerous actions (setting the callback pointer) and other initialization stuff.
But for my use case, I could live with the uart_init() function too.

Implementing this extension only in cpu/board specific code would not allow writing portable applications needing this functions (e.g. my Modbus master).

[alignan]

I have implemented something alike for a RS-485/Modbus RTU project as well, so I understand the requirement, but I'm reluctant on triggering a full API change... I would separate these features into a separate uart-extended.c file or alike in the boards/cpu, at least until there is criteria to have this extended for other platforms as well. @haukepetersen?

Please separate the flash implementation from the UART proposed changes, it would be great to have in a different PR 😄

[geith]

I think, implementing this as an extended API within a seperate module would be the best solution (e.g. MODULE_UART_EXT). It could be done within the same file (uart.h/.c) or in a seperate one (as suggested uart-extended.h/.c). But it should be a general API for other platforms too.
@haukepetersen: What is yout opinion about that?

So, I will change my code (and remove the flash code; sorry, was the wrong branch...).

[miri64]

Please suffix typedef'ed typenames with _t

[haukepetersen]

@geith: Good to know your use case! In general I completely agree, that we should supply this functionality independent of a platform.

To clarify:

• we need to be able to re-configure the parity usage during runtime.
• we need to re-configure the number of stop bits during runtime.
• do we also need to re-configure the baudrate during runtime?
• we need means to make sure any ongoing transfert (write) is finished.
  Does this fit to your requirements?

As a solution, I suggest we do the following:

• the uart_init stays as-is, it configures the uart to the given baudrate in the default mode (8N1)
• IMHO flush is not needed, as uart_write() is synchronous, as it blocks until all the bytes given to that function are actually send out. We might check our existing implementation and the API doc for this again, though.
• about the re-configuration: let's add this function to the uart interface:

typedef enum {
    UART_NONE,
    UART_ODD,
    UART_EVEN
} uart_parity_t;

int uart_mode(uint8_t databits, uint8_t stopbits, uart_parity_t parity);

Now this allows for (runtime) re-configuration of the parameters of interest, while still keeping the interface slim. Also, existing drivers do not need to be touched. What do you think?

[geith]

@haukepetersen: Your suggested API function would be okay for me, but in my opinion, the uart_flush() is still be needed.

The function uart_write() waits, until the date have been written into the TX FIFO buffer (e.g. 16 bytes on the cc2538, but may differ on other chips). But when the program execution continues, the UART is still transmitting to the wire. As an example: 16 bytes * (1 start bit + 8 data bits + 1 stop bit) = 160 bits, that makes about 67 ms at 2400 baud.

This behavior is absolutely okay for most use cases, but I need to synchronize my program with the moment when the TX buffer is empty again; if I would switch the transceiver direction immediately after uart_write() returns, the 16 bytes from the buffer would be lost on my half-duplex RS-485. On Linux, the termios library provides a similar function tcdrain() (if you prefer, I could change the name of my function to uart_drain()).

So, would you be okay with an API extension following your suggestion plus the uart_flush()?

[haukepetersen]

The question is, which constraints we want to put on the uart_write() function. I mean in general, there is no problem in re-factoring/enforcing the write functions to actually block until the last given byte was actually shifted out. But performance wise, it would lead to some unnecessary waiting, especially on platforms that support TX FIFOs.

Thinking about it now, I think you are right and a flush function would make sense. I think I would prefer a name as uart_txflush() or similar, to underline that this function is only looking at the TX buffer.

[geith]

I agree, from the performance perstpective, uart_write() should remain as it is. The name of uart_flush() has been changed to uart_txflush().

[haukepetersen]

maybe something like Available parity modes?

[geith]

Just Parity modes? The availability depends on hardware and driver implementation.

[haukepetersen]

s/@brief set UART mode/@brief Confiure UART mode/

[haukepetersen]

same here: `s/@brief flush UART TX buffer/@brief Flush UART TX buffer/

[haukepetersen]

indention

[haukepetersen]

maybe better: 'Block until TX buffer is empty and all bytes have been shifted out.'

[haukepetersen]

Could you also add some more detailed documentation for:

• uart_init: Telling that during init the device is always configured in 8N1 mode and to use uart_mode afterwards if changes to this are needed
• uart_write: Underline that the function returns after all bytes have been written to the TX buffer, but that the bytes must not necessarily have been shifted out. Link to uart_txflush for safely syncing on this.
• uart_mode: that implementations MUST make sure that no transfers (e.g. no receive) is going on while the mode is reconfigured

Thanks!

[MrKevinWeiss]

Yes, if you want just this PR dealt with I can work it into the short term schedule. Otherwise, I would like to propose an update to the uart API at some point (maybe next month) if this can wait until then.

[tcschmidt]

I would like to propose an update to the uart API at some point (maybe next month) if this can wait until then.

Agreed, makes sense: let's wait for this.

[haukepetersen]

This PR is surely still very valid. As quick recap on the state of discussion (might be subjective), we are discussing two separate issues:

1. reconfiguring the UART at runtime (i.e. mode)
2. removing the (implicit) 'synchronous' requirement from uart_write() and adding a flush() function

Regarding 1.

An agreement for the need of this functionality is reached, so it is rather the question on 'how' to implement it. Thinking about it, I propose the following code:

typedef enum {
    UART_8N1,
    UART_8E1,
    UART_8O1,
    UART_8N2,
    ...
} uart_mode_t;

int uart_mode(uart_t uart, uart_mode_t mode);

Of course following the existing concept of allowing CPU implementation to override that type definition. This would enable for significantly more efficient UART driver implementations (as register values can be encoded directly in the enum value -> less ugly swich-case blocks).

Now when implementing this new functionality for existing uart drivers, I propose the approach similar to the one taken in #9845, using a temporary feature (e.g. periph_uart_modecfg) for this new functionality. Once all existing UART implementations are updated with this functionality, we can then drop that feature again - while of course all new ports are required to provide this right away.

On top, when we touch existing implementations, it would be IMHO be further beneficial to add a static mode configuration parameter to the uart_config structs, so we allow also for static UART mode config (see #9127).

Regarding 2

Personally, I am still in favor of a synchronous uart_write() for reasons of simplicity. But I also see the arguments given for making write() asynchronous and adding an additional flush() function - so I won't block this approach any longer.

For the naming, I personally would simply call that function uart_flush() however...

So here is my proposal for the next steps:

• move this part to a separate PR
• also use a temporary feature to mark this capability
• implement for existing CPUs...

[yegorich]

@haukepetersen uart_mode_t; can get very long. We have data bits from 5 to 9, five parity modes and two stop bit modes.

[haukepetersen]

rather 3 stop bit modes (0-2) -> so it would be 180 modes altogether. Probably some less, as certain combinations (e.g. 9 data bytes + parity) are not possible, right?!

So I guess my idea was not so great after all... I am trying to think of something that could still enable us to efficiently code the mode() function. How about something in the direction of:

typedef enum {
    UART_DATABITS_3,
    UART_DATABITS_4,
    ...
    UART_DATABITS_9,
} uart_databits_t;

typedef enum {
   UART_PARITY_NONE,
   ...
} uart_parity_t;

typedef enum {
    UART_STOPBITS_0,
    ...
} uart_stopbits_t;

int uart_mode(uart_t dev, uart_databits_t databits, uart_parity_t parity, uart_stopbits_t stopbits);

This would still enable us to encode CPU specific register contents into the enum values (allowing us to apply them directly), while circumventing the 'state explosion' in the enum definitions.

Alternatively even we could even do something like

// condition when defining values for the 3 enums:
// UART_DATABITS_8 := must be 0 for all platforms
// UART_PARITY_NONE := must be 0 for all platforms
// UART_STOPBITS_1 := must be 0 for all platforms

int uart_mode(uart_t dev, unsigned mode);
// usage:
uart_mode(dev, 0); //-> 8N1
uart_mode(dev, (UART_DATABITS_5 | UART_PARITY_EVEN)); // -> 5E1

[MrKevinWeiss]

I don't know if here is the place to start the discussion but I would throw in

typedef enum {
    UART_DATABITS_8,
    UART_DATABITS_5,
    UART_DATABITS_6,
    UART_DATABITS_7,
    UART_DATABITS_9
} uart_databits_t;

typedef enum {
   UART_PARITY_NONE,
   UART_PARITY_EVEN,
   UART_PARITY_ODD,
} uart_parity_t;

typedef enum {
   UART_STOPBITS_0,
   UART_STOPBITS_1,
   UART_STOPBITS_2,
} uart_stopbits_t;

typedef struct {
	uart_databits_t databits:	3;
	uart_parity_t parity:		2;
	uart_stopbits_t stop:		2;
} uart_config;

On that not why not put all args in a struct (so baud, dev, and callback too)? Makes saving the config state easy. I think that is what the STMCUBE HIL tries to do. We can also have nice packing with the bitfields and type safety with the enums. If we make all default values zero then if users don't want non-standard settings they don't need to change the values. Maybe we even have a #ifdef macro for extended API vs basic one so code size doesn't go crazy.

...Maybe a bit much.

[yegorich]

@MrKevinWeiss

typedef enum {
   UART_PARITY_NONE,
   UART_PARITY_EVEN,
   UART_PARITY_ODD,
   UART_PARITY_MARK,
   UART_PARITY_SPACE,
} uart_parity_t;

[haukepetersen]

@MrKevinWeiss that does unfortunately not work, as it would take away the possibility of encoding register bit values in the enums.

Let me elaborate in a small example: take the UART->CONFIG register of a nrf52 CPU. Bit 1-3 are defining the parity mode, with 0x0 meaning no parity and 0x7 configuring them to even parity.

So we can simply do
[EDIT: fixed a small typo in the example code...]

// in the nrf52/include/periph_cpu.h
#define HAVE_UART_PARITY_T
typedef enum {
   UART_PARITY_NONE = 0x00,
   UART_PARITY_EVEN = (0x07 << 1),
   UART_PARITY_X = 0x20, // something that lets us read this as not-supported value
   ...
} uart_parity_t;

// in the uart driver
int uart_mode(..., uart_partity_t parity, ...)
{
    /* check if given parity value is actually supported */
    if (partiy & 0xe0) {
       return UART_NOMODE;
    }
    ...
    NRF_UART->CONFIG = parity;  // no need for shifting, masking, switch-case, etc...
    ...
}

Now as the actual used values for the enum members are highly dependent on the actual CPU, one can not make any assumptions on their structure, so joining them will not work...

On that not why not put all args in a struct (so baud, dev, and callback too)? Makes saving the config state easy. I think that is what the STMCUBE HIL tries to do.

Major reason: efficiency. Minor: consistency.

I have been playing with that approach quite a bit when initially designing the periph interfaces. But that approach proved to be quite inefficient in terms of memory usage and runtime overhead. Just look at the code size overhead of the STM cube library... At least back then, you could easily add ~5K ROM to any standard RIOT example when using that library.

[MrKevinWeiss]

So would the enums be declared in each cpu or in the API (ie. drivers/include/periph/uart.h)?

[MrKevinWeiss]

you could easily add ~5K ROM to any standard RIOT example when using that library

Heh true the STM cube is big and heavy.

possibility of encoding register bit values in the enums

You could make the bitfields big enough to fit the encodings or just not use a bitfield and it would be the same as the enum.

I guess was thinking to add the enums to switches (or if's) only if they don't fit into a exploitable pattern but if the majority will have to be "mapped" then I can see how it isn't worth it.

Should we find a proper place to have this discussion?

[haukepetersen]

I guess was thinking to add the enums to switches (or if's) only if they don't fit into a exploitable pattern but if the majority will have to be "mapped" then I can see how it isn't worth it.

it is worth it :-)

On top, this concept is already heavily used throughout the peripheral drivers, and we should be consistent (which is and has always been an important point in the periph interface's design).

Should we find a proper place to have this discussion?

I think this is the proper place, isn't it?

[haukepetersen]

So would the enums be declared in each cpu or in the API (ie. drivers/include/periph/uart.h)?

There would be a generic definition in drivers/include/perihp/uart.h, which can be overridden by a specific CPU in its CPU/include/periph_cpu.h header.

Take as example the gpio_flank_t definition the perihp/gpio.h interface. This is overridden by specific CPUs, e.g. in cpu/sam0_common/include/periph_cpu_common.h or in cpu/stm32_common/include/periph_cpu_common.h.

[MrKevinWeiss]

Ok, thanks for the info, that clarifies the enum choice. What about the passing several parameters by multiple arguments vs structs?

[haukepetersen]

What about the passing several parameters by multiple arguments vs structs?

Quote "Heh true the STM cube is big and heavy."

The two reasons I don't like it are (i) it is inefficient in both memory and runtime overheads, and (ii) we should stay consistent with the rest of the periph APIs

[haukepetersen]

maybe to give some quick example to underline this:

// say we have the parity that we need to change programatically, the rest is constant
uart_mode_cfg_t cfg = { .databits = UART_DATABITS8, .stopbits = UART_STOPBITS_2, .parity = some_parity };
uart_mode(dev, &cfg);

// vs
uart_mode(dev, UART_DATABITS8, some_parity, UART_STOPBITS_2);

To the best of my knowledge (so when I tried it last), the compiler does not optimize both approaches the same way, so that the first approach takes more instructions and also more stack space.

[yegorich]

@haukepetersen I've cooked a patch defining enums for parity, databits etc. both in drivers/include/periph/uart.h as also for STM32. But before I make an official PR I'd like to clarify some points.

1. I've looked at what Zephyr OS makes of UART configuration. Basically they use the same configuration options, but there are some differences. They also define 0.5 and 1.5 stopbits, that will be used for Smartcard operation. But what is more important their uart.h also defines hardware handshake options. At least options none and RTS/CTS would be relevant for RIOT and supported MCUs. I would include this option into uart_mode() param list.

2. What is the final plan for UART configuration? Should all these parameters go into uart_init() or into uart_mode()? If it is uart_init() then we can only take of the options that deviate from default ones as the UART will be initialized with default values, but if uart_mode() will be allowed at arbitrary time in the user code, then we will have to take care of setting all values, i.e. reverting non-default values to their original state.

What do you think about it?

[haukepetersen]

At least options none and RTS/CTS would be relevant for RIOT and supported MCUs. I would include this option into uart_mode() param list.

Don't like it, I think they should stay in the board/CPU specific code path and be configured statically.

What is the final plan for UART configuration? Should all these parameters go into uart_init() or into uart_mode()? If it is uart_init() then we can only take of the options that deviate from default ones as the UART will be initialized with default values, but if uart_mode() will be allowed at arbitrary time in the user code, then we will have to take care of setting all values, i.e. reverting non-default values to their original state.

I only partly understand this question. Which parameters do you mean exactly?

Concerning the default parameters: from my perspective uart_init() should still configure the UART in default configuration (8N1), and then uart_mode() can be called afterwards to switch to another mode. This way we don't break the existing API.

[yegorich]

@haukepetersen I was talking about 8n1 params only, not the baudrate. But I see your point and will change my upcoming PR accordingly.

[MrKevinWeiss]

I suppose all params can be either added statically in the periph_config or with an extra function (uart_mode).

Can anyone think of practical reasons to need any of the parameters runtime configurable?
Maybe Modbus RS485 could use that?

Moving the setting to a seperate function does help minimal code size if it isn't used. If it had to be implemented in the cpu during init it would always be included.

[MrKevinWeiss]

Hmm I suppose any type of controller like plc's, I have worked on industrial products that had some parity and baud parameters configurable runtime. I think 👍 to the uart_mode added to the api.

[MrKevinWeiss]

Well now that #10743 is merged maybe we can update the cc2538 to fit the uart_mode api!

[MrKevinWeiss]

@geith If you are not planning to update I can take it over?

[geith]

@MrKevinWeiss I'm currently not active on RIOT (have other priorities); so you are welcome to take over that part.

[MrKevinWeiss]

Taken over with #11503. Thanks!