Skip to content
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

modbus: Respect CONFIG_UART_USE_RUNTIME_CONFIGURE flag #68858

Merged
merged 1 commit into from
Apr 10, 2024
Merged

modbus: Respect CONFIG_UART_USE_RUNTIME_CONFIGURE flag #68858

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion subsys/modbus/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ config MODBUS_SERIAL
default y
depends on SERIAL && SERIAL_HAS_DRIVER
depends on DT_HAS_ZEPHYR_MODBUS_SERIAL_ENABLED
select UART_USE_RUNTIME_CONFIGURE
carlescufi marked this conversation as resolved.
Show resolved Hide resolved
imply UART_USE_RUNTIME_CONFIGURE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, I would remove this select/imply entirely :) The default value of UART_USE_RUNTIME_CONFIGURE is y, so unless the user explicitly adds CONFIG_UART_USE_RUNTIME_CONFIGURE=n to their .prj somewhere, it will be selected. This also means that no current users of the modbus_serial.c have explicitly disabled runtime as it produces this Kconfig error to do so:

warning: UART_USE_RUNTIME_CONFIGURE (defined at drivers/serial/Kconfig:51) was assigned the value
'n' but got the value 'y'. See
http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_UART_USE_RUNTIME_CONFIGURE and/or look up
UART_USE_RUNTIME_CONFIGURE in the menuconfig/guiconfig interface. The Application Development
Primer, Setting Configuration Values, and Kconfig - Tips and Best Practices sections of the manual
might be helpful too.

If it is possible to use this driver without UART_USE_RUNTIME_CONFIGURE as it is after this PR, it shall not be selected in Kconfig. select is explicitly used only when the dependency is "small" but obligatory :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. The first version of this PR removed the select line entirely, but Johann refused it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jfischer-no can we agree on removing the select not being a breaking change? or have I missed something? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

No.

help
Enable Modbus over serial line support.

Expand Down
94 changes: 54 additions & 40 deletions subsys/modbus/modbus_serial.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,58 @@ static int configure_gpio(struct modbus_context *ctx)
return 0;
}

static inline int configure_uart(struct modbus_context *ctx,
struct modbus_iface_param *param)
{
struct modbus_serial_config *cfg = ctx->cfg;
struct uart_config uart_cfg = {
.baudrate = param->serial.baud,
.flow_ctrl = UART_CFG_FLOW_CTRL_NONE,
};

if (ctx->mode == MODBUS_MODE_ASCII) {
uart_cfg.data_bits = UART_CFG_DATA_BITS_7;
} else {
uart_cfg.data_bits = UART_CFG_DATA_BITS_8;
}

switch (param->serial.parity) {
case UART_CFG_PARITY_ODD:
case UART_CFG_PARITY_EVEN:
uart_cfg.parity = param->serial.parity;
uart_cfg.stop_bits = UART_CFG_STOP_BITS_1;
break;
case UART_CFG_PARITY_NONE:
/* Use of no parity requires 2 stop bits */
uart_cfg.parity = param->serial.parity;
uart_cfg.stop_bits = UART_CFG_STOP_BITS_2;
break;
default:
return -EINVAL;
}

if (ctx->client) {
/* Allow custom stop bit settings only in client mode */
switch (param->serial.stop_bits_client) {
case UART_CFG_STOP_BITS_0_5:
case UART_CFG_STOP_BITS_1:
case UART_CFG_STOP_BITS_1_5:
case UART_CFG_STOP_BITS_2:
uart_cfg.stop_bits = param->serial.stop_bits_client;
break;
default:
return -EINVAL;
}
}

if (uart_configure(cfg->dev, &uart_cfg) != 0) {
LOG_ERR("Failed to configure UART");
return -EINVAL;
}

return 0;
}

void modbus_serial_rx_disable(struct modbus_context *ctx)
{
modbus_serial_rx_off(ctx);
Expand Down Expand Up @@ -505,7 +557,6 @@ int modbus_serial_init(struct modbus_context *ctx,
struct modbus_serial_config *cfg = ctx->cfg;
const uint32_t if_delay_max = 3500000;
const uint32_t numof_bits = 11;
struct uart_config uart_cfg;

switch (param.mode) {
case MODBUS_MODE_RTU:
Expand All @@ -521,49 +572,12 @@ int modbus_serial_init(struct modbus_context *ctx,
return -ENODEV;
}

uart_cfg.baudrate = param.serial.baud,
uart_cfg.flow_ctrl = UART_CFG_FLOW_CTRL_NONE;

if (ctx->mode == MODBUS_MODE_ASCII) {
uart_cfg.data_bits = UART_CFG_DATA_BITS_7;
} else {
uart_cfg.data_bits = UART_CFG_DATA_BITS_8;
}

switch (param.serial.parity) {
case UART_CFG_PARITY_ODD:
case UART_CFG_PARITY_EVEN:
uart_cfg.parity = param.serial.parity;
uart_cfg.stop_bits = UART_CFG_STOP_BITS_1;
break;
case UART_CFG_PARITY_NONE:
/* Use of no parity requires 2 stop bits */
uart_cfg.parity = param.serial.parity;
uart_cfg.stop_bits = UART_CFG_STOP_BITS_2;
break;
default:
return -EINVAL;
}

if (ctx->client) {
/* Allow custom stop bit settings only in client mode */
switch (param.serial.stop_bits_client) {
case UART_CFG_STOP_BITS_0_5:
case UART_CFG_STOP_BITS_1:
case UART_CFG_STOP_BITS_1_5:
case UART_CFG_STOP_BITS_2:
uart_cfg.stop_bits = param.serial.stop_bits_client;
break;
default:
if (IS_ENABLED(CONFIG_UART_USE_RUNTIME_CONFIGURE)) {
if (configure_uart(ctx, &param) != 0) {
return -EINVAL;
}
}

if (uart_configure(cfg->dev, &uart_cfg) != 0) {
LOG_ERR("Failed to configure UART");
return -EINVAL;
}

if (param.serial.baud <= 38400) {
cfg->rtu_timeout = (numof_bits * if_delay_max) /
param.serial.baud;
Expand Down
Loading