api: uart: Add configure functions for UART.

[Mierunski]

Allow UART configuration to be altered during runtime.

This PR is follow up to #10820 it adds configuration part of it.

[Mierunski]

Things that haven't been decided yet.

[Mierunski]

There is ongoing discussion if flow control, parity, stop, and data bits should be kept as bitmasks or enums.
@galak @tbursztyka
I personally prefer using 4 values with enums here. Configuration will be kept inside driver, and this structure will only exist locally when it is used to extract current configuration, change it, then write it back.

[galak]

I'm for avoid bitfields for such things since I think it makes the code harder to get right. I say go with u8_t's.

[tbursztyka]

4 values as 8bits instead of a bitfield in 32bits is same size so go for the enum.

[qianfan-Zhao]

I prefer enum

[Mierunski]

Changed to u8_t and enums

[pfalcon]

Btw, so far I see discussion of bitmasks vs byte-sized fields. Did we consider using true C bitfields, like u32_t parity : 3?

That would save the bits and allow address @erwango's comment below https://github.com/zephyrproject-rtos/zephyr/pull/11185/files#r231818904 (w/o increasing size).

[tbursztyka]

@pfalcon we forbid long ago usage of C bitfields in public APIs, for instance i²c (afaik) used to have those for its config and it was changed to bitmasks.

[pfalcon]

we forbid long ago usage of C bitfields in public APIs

Really? LOL! Because we should do the opposite - forbid their use in referring to external structures (and we do that a lot using bitfields), and be absolutely free to use them in our APIs with our compilers. (Or with "their" compilers, as long as we intend to keep "userspace and kernelspace is built together" paradigm.)

[tbursztyka]

That was the issue as far as I remember, app could be built by a different compiler than common zephyr one. This discussion was given almost 3 years ago.

[Mierunski]

It was decided that C bitfields are forbidden and they will not be used in this API. I will stick with u8_t and enums.

[galak]

Does rx_timeout have any particular meaning (might be held over from other PR)?

[pfalcon]

Actually, +1 to what @galak says - let's leave rx_timeout to the other PR/part of the API.

[Mierunski]

Right, removed rx_timeout

[tbursztyka]

what is the use case for that one?

[galak]

I wonder if you need get_cfg right now to be able to only modify one param, for example if I want to set just the baudrate, we kinda have a read/modify/write with how the set_cfg api is defined.

Wondering if we should have:
set_baudrate()
set_parity()
etc.

[pfalcon]

Wondering if we should have:
set_baudrate()

I believe this again was discussed, and @tbursztyka suggest to follow how it's done for other comm. interfaces (?, didn't check that myself), and have common structure for all params.

We also should consider a usecase of userspace syscalls (because UART config API of course should be exported to userspace in one or another form). From that PoV, we always should save on # of syscalls. (Can define convenience inline wrappers on top of that).

[tbursztyka]

I don't think we need atomic cfg modifier. Just apply a new config via set_cfg as it is done in other buses. It's not like uart cfg has a lot of fields.

I would rename set_cfg to configure btw.

[pfalcon]

atomic cfg modifier

I'm not sure I understand context for "atomic" here. The question is still whether we need to provide "get cfg" operation to users, and that seems to be useful. After all, how one would implement POSIX stty for example?

[qianfan-Zhao]

@pfalcon And what is yours idea about 'u32_t flags' or enum?

[pfalcon]

@qianfan-Zhao: That's orthogonal imho. Posted a "new" suggestion here: #11185 (comment) .

[pfalcon]

I would rename set_cfg to configure btw.

Ok, my 2 cents on this. Prior art: spi.h doesn't have configure(), while i2c has i2c_configure(). Thus, for consistency, proposal:

->configure()
->get_config()

IMHO looks not bad.

[tbursztyka]

looks better indeed.
(on spi the reason why there is no configure() is that config is always given to each calls, driver reconfigures if only it's a different one. I2c could have been doing the same actually.)

[Mierunski]

changed to configure() and get_config()

[codecov-io]

Codecov Report

Merging #11185 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #11185   +/-   ##
=======================================
  Coverage   48.37%   48.37%           
=======================================
  Files         265      265           
  Lines       42188    42188           
  Branches    10137    10137           
=======================================
  Hits        20408    20408           
  Misses      17703    17703           
  Partials     4077     4077

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7d1ce6...89f153d. Read the comment docs.

[erwango]

@qianfan-Zhao , you might be interested

[erwango]

Can we add a vendor, or optional?

[tbursztyka]

As in other APIs we try not to expose any vendor specific stuff, the problem is that code using this API would not be guaranteed to behave the same (or work at all) depending on such configuration.

[Mierunski]

I'm against adding vendor config, if more options are needed they should be explicitly added as new configuration option

[Mierunski]

@galak @pfalcon @nordic-krch Any comments on this? Other discussions seem to be resolved

[pfalcon]

Let's leave US-timezone folks time to catch up/reply. I'd be ready to +1 this tomorrow.

P.S. There's a proposed PR process: https://docs.google.com/document/d/1dSLSTckOgCeBh8MrU4AgpjkR6hQ0IikPnreAOO_wWGY/edit

[qianfan-Zhao]

how about make the most commonly used has the value zero?

STOP_BITS_1 and DATA_BITS_8 is the most common used

[pfalcon]

how about make the most commonly used has the value zero?

But why?

[qianfan-Zhao]

en, memset(&cfg, 0, sizeof(cfg)) can set a default mode.

[qianfan-Zhao]

struct uart_config cfg = {
.baud = 115200,
};
something like this can work fine

[jakub-uC]

@qianfan-Zhao I am not sure if this is good approach...
You can create a macro for setting default values, you will have more control about what is happening.

[qianfan-Zhao]

yeah, maybe this is not a good way, but like most things linux did, 0 can hold a default value in a structure.

[galak]

I think trying to have the default be 0 would be good, since I don't know if there is anyway to enforce the use of a macro for default value.

[Mierunski]

Thats a bad idea in my opinion, i.e. if we have current-speed = <9600>; in dts then default value of speed is taken from dts or arbitrary value like 115200? I was thinking that we are moving in direction of putting initial setting into dts, so using default values would complicate that

[pfalcon]

I am not sure if this is good approach...

I'm not sure either. Recently we had an argument with @jukkar regarding numeric values for some symbolic constants, where I advocated position of using "well known" numeric values form well-known system like Linux, which in turn uses the values from a standard like iBCS - all to be able to printf("%d") (also p/x in gdb) that value and see that widely known (in narrow circles) numeric value and not be confused when jumping from system to system.

With the lack of arguments like that, seeing stuff like:

enum uart_config_data_bits {
BITS_8,
BITS_5,
BITS_6,
...
};

- leaves one one impression: it's erratic. Again, printing raw value/seeing it in debugger will only get you confused, as it's not in logical order.

So, sorry, but you should always set all settings explicitly, and always using symbolic value, not relying on any numeric value it has. That's actually argument to ban zero value, and EINVAL on it.

[tbursztyka]

I'd prefer explicit settings too

[galak]

Not sure if there is any value in doing:

UART_CFG_DATA_BITS_5 = 5,

[Mierunski]

That would make it differ from parity and stop bits, I don't see how this approach could be used for stop bits for example

[pfalcon]

Minor, but why is this put on top of existing copyright, instead of on bottom?

[Mierunski]

Just following approach in other API .h files

[pfalcon]

Weird, never paid attention. Definitely not how Linux has it, e.g. https://elixir.bootlin.com/linux/latest/source/ipc/sem.c (though not too consistent there too, e.g. 2010 and 2016 swapped in this random example.)

[pfalcon]

Extra pass thru docstrings, suggestions for improvements.

[pfalcon]

Any change to fit "enum" in the description, e.g.:

Parity bit, use enum @ref uart_config_parity

Oh, and it's probably "parity setting", not "parity bit".

[pfalcon]

Please specify that "<0" means error, and only then describe particular ones, giving clear hint that there can be more. E.g. something like "<0 error, including: -ENOTSUP ...". Can also look thru other docstrings with similar formulations for ideas.

[Mierunski]

I wanted to use ENOSYS if function is not implemented, but got some problems, see here #11207

[pfalcon]

I wanted to use ENOSYS if function is not implemented, but got some problems, see here

Please use whatever other APIs use in this case, and that's ENOTSUP. If you'd like to change it, it should be done consistently across API, and that's a separate task (and quite a task, with a usual RFC phase, etc.)

[Mierunski]

Changed docstrings @retval 0 If successful, negative errno code otherwise.

[pfalcon]

@Mierunski, ping, any news on docstrings updates?

[nordic-krch]

usually, I see following scheme:

if (/*possible error condition*/) {
    return ERR;
}

/*execute valid case*/

but it's rather not used in that file.

[Mierunski]

@galak ping

[nashif]

nit, we have a mix of API function naming conventions, sometime verb at the end, sometime in the middle, i have no preference, but uart_config_get reads better. We should try and standarize this for new APIs

[pfalcon]

but uart_config_get reads better.

Well, that's hardly. And within 2 functions added here, there's a consistency: first verb goes, then optional object (noun). (configure is a verb). It's inconsistent with the older API, yeah.

We should try and standarize this for new APIs

We should try to standardize, or should stick for consistency to whatever semi-random and not standardized way it was done before?

[Mierunski]

Changed to uart_config_get

[nashif]

@pfalcon

Well, that's hardly.

Obviously that is very subjective. Maybe because I read from right to left.

This is how we have thing in the kernel API and in some drivers.

[pfalcon]

For those who may miss it, first implementation of these APIs went for review: #11577

[ranbochen]

@qianfan-Zhao
The current STM32 driver still does not support LINE_CTRL, can you merge your version to support LINE_CTRL?

zephyr/drivers/serial/uart_stm32.c

Lines 324 to 327 in 794d382

	/* Driver currently doesn't support line ctrl */
	if (UART_CFG_FLOW_CTRL_NONE != cfg->flow_ctrl) {
	return -ENOTSUP;
	}