Skip to content

api: uart: Add configure functions for UART. #11185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 15, 2018
Merged

api: uart: Add configure functions for UART. #11185

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 115 additions & 0 deletions include/uart.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/*
* Copyright (c) 2018 Nordic Semiconductor ASA
Copy link
Contributor

@pfalcon pfalcon Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@Mierunski Mierunski Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just following approach in other API .h files

Copy link
Contributor

@pfalcon pfalcon Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

* Copyright (c) 2015 Wind River Systems, Inc.
*
* SPDX-License-Identifier: Apache-2.0
Expand Down Expand Up @@ -64,6 +65,62 @@ extern "C" {
*/
#define UART_ERROR_BREAK (1 << 3)

/**
* @brief UART controller configuration structure
*
* @param baudrate Baudrate setting in bps
* @param parity Parity bit, use @ref uart_config_parity
Copy link
Contributor

@pfalcon pfalcon Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

* @param stop_bits Stop bits, use @ref uart_config_stop_bits
* @param data_bits Data bits, use @ref uart_config_data_bits
* @param flow_ctrl Flow control setting, use @ref uart_config_flow_control
*/
struct uart_config {
u32_t baudrate;
u8_t parity;
u8_t stop_bits;
u8_t data_bits;
u8_t flow_ctrl;
};
Copy link
Member

@erwango erwango Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a vendor, or optional?

Copy link
Contributor

@tbursztyka tbursztyka Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Mierunski Mierunski Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@Mierunski Mierunski Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pfalcon pfalcon Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/** @brief Parity modes */
enum uart_config_parity {
UART_CFG_PARITY_NONE,
UART_CFG_PARITY_ODD,
UART_CFG_PARITY_EVEN,
UART_CFG_PARITY_MARK,
UART_CFG_PARITY_SPACE,
};

/** @brief Number of stop bits. */
enum uart_config_stop_bits {
UART_CFG_STOP_BITS_0_5,
UART_CFG_STOP_BITS_1,
UART_CFG_STOP_BITS_1_5,
UART_CFG_STOP_BITS_2,
};

/** @brief Number of data bits. */
enum uart_config_data_bits {
UART_CFG_DATA_BITS_5,
Copy link
Contributor

@galak galak Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if there is any value in doing:

UART_CFG_DATA_BITS_5 = 5,

Copy link
Author

@Mierunski Mierunski Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

UART_CFG_DATA_BITS_6,
UART_CFG_DATA_BITS_7,
UART_CFG_DATA_BITS_8,
Copy link
Contributor

@qianfan-Zhao qianfan-Zhao Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

STOP_BITS_1 and DATA_BITS_8 is the most common used

Copy link
Contributor

@pfalcon pfalcon Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

But why?

Copy link
Contributor

@qianfan-Zhao qianfan-Zhao Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@qianfan-Zhao qianfan-Zhao Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jakub-uC jakub-uC Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@qianfan-Zhao qianfan-Zhao Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@galak galak Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Mierunski Mierunski Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pfalcon pfalcon Nov 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@tbursztyka tbursztyka Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer explicit settings too

};

/**
* @brief Hardware flow control options.
*
* With flow control set to none, any operations related to flow control
* signals can be managed by user with uart_line_ctrl functions.
* In other cases, flow control is managed by hardware/driver.
*/
enum uart_config_flow_control {
UART_CFG_FLOW_CTRL_NONE,
UART_CFG_FLOW_CTRL_RTS_CTS,
UART_CFG_FLOW_CTRL_DTR_DSR,
};


/**
* @typedef uart_irq_callback_user_data_t
* @brief Define the application callback function signature for
Expand Down Expand Up @@ -125,6 +182,10 @@ struct uart_driver_api {
/** Console I/O function */
int (*err_check)(struct device *dev);

/** UART configuration functions */
int (*configure)(struct device *dev, const struct uart_config *cfg);
int (*config_get)(struct device *dev, struct uart_config *cfg);

#ifdef CONFIG_UART_INTERRUPT_DRIVEN

/** Interrupt driven FIFO fill function */
Expand Down Expand Up @@ -257,6 +318,60 @@ static inline unsigned char _impl_uart_poll_out(struct device *dev,
return api->poll_out(dev, out_char);
}

/**
* @brief Set UART configuration.
*
* Sets UART configuration using data from *cfg.
*
* @param dev UART device structure.
* @param cfg UART configuration structure.
*
*
* @retval -ENOTSUP If configuration is not supported by device.
Copy link
Contributor

@pfalcon pfalcon Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Mierunski Mierunski Nov 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pfalcon pfalcon Nov 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Author

@Mierunski Mierunski Nov 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* or driver does not support setting configuration in runtime.
* @retval 0 If successful, negative errno code otherwise.
*/
__syscall int uart_configure(struct device *dev, const struct uart_config *cfg);

static inline int _impl_uart_configure(struct device *dev,
const struct uart_config *cfg)
{
const struct uart_driver_api *api =
(const struct uart_driver_api *)dev->driver_api;

if (api->configure) {
Copy link
Contributor

@nordic-krch nordic-krch Nov 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually, I see following scheme:

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

/*execute valid case*/

but it's rather not used in that file.

return api->configure(dev, cfg);
}

return -ENOTSUP;
}

/**
* @brief Get UART configuration.
*
* Stores current UART configuration to *cfg, can be used to retrieve initial
* configuration after device was initialized using data from DTS.
*
* @param dev UART device structure.
* @param cfg UART configuration structure.
*
* @retval -ENOTSUP If driver does not support getting current configuration.
* @retval 0 If successful, negative errno code otherwise.
*/
__syscall int uart_config_get(struct device *dev, struct uart_config *cfg);

static inline int _impl_uart_config_get(struct device *dev,
struct uart_config *cfg)
{
const struct uart_driver_api *api =
(const struct uart_driver_api *)dev->driver_api;

if (api->config_get) {
return api->config_get(dev, cfg);
}

return -ENOTSUP;
}

#ifdef CONFIG_UART_INTERRUPT_DRIVEN

Expand Down