Rs485 APIs support #80305

2345lug · 2024-10-23T12:42:32Z

My work is based on this branch: #40907, but it differs somewhat from it. I have changed the units of measurement for assertion and deassertion times. I have also implemented control of the DE pin (which is specified in the DTS) for stm32_uart in interrupt-driven mode.

github-actions · 2024-10-23T12:43:10Z

Hello @2345lug, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

2345lug · 2024-11-12T12:14:03Z

Hello! @dleach02 I need some advice regarding the RS485 API:
Is it acceptable to control GPIO in this manner for boards whose drivers do not allow direct pin control?
Is it a good idea to use the CTS pin for DE enable in drivers that allow control of this pin (as implemented by colleagues in the NXP driver)?
Thank you in advance!

drivers/serial/uart_stm32.c

erwango · 2024-11-12T15:25:02Z

drivers/serial/uart_stm32.c

+#ifdef CONFIG_SERIAL_SUPPORT_RS485
+	if (config->de_enable) {
+		gpio_pin_configure_dt(&config->de_pin, GPIO_ACTIVE_LOW | GPIO_OUTPUT_ACTIVE);
+		gpio_pin_set(config->de_pin.port, config->de_pin.pin, config->de_invert);
+		k_timer_init(&data->rs485_timer, rs485_de_time_expire_callback, NULL);
+		k_timer_user_data_set(&data->rs485_timer, (void *)config);
+	}


I wonder why using a GPIO DE bit instead of using the built in DE pin functionality driver by HW (and already implemented, by the way).
At the minimum the PR should explain why introducing this alternate software control method, why one would use it and make it optional (if ever there is a really good reason to have it in the driver)

Thank you very much for your response! If I understand correctly, not all STM32 controllers have hardware support for RS485 (e.g., F0, F1, F4 don't have it), and therefore it is not possible to control the DE pin via hardware on these models. I agree that the API should support hardware control (if the controller supports it), and this needs to be implemented.

My solution proposes an alternative method for controlling the pin for RS485 and a unified API, which, as I understand it, is currently missing in Zephyr. RS485 can be used not only for Modbus, after all. As far as I can tell, people often have the need to use RS485 for other purposes, for example (#32733).

I would appreciate any advice or guidance to successfully integrate this solution.

I agree that the API should support hardware control (if the controller supports it), and this needs to be implemented.

My point is that hw control is already implemented in this driver. See

zephyr/drivers/serial/uart_stm32.c

Lines 2028 to 2043 in 5f418f5

#if HAS_DRIVER_ENABLE

if (config->de_enable) {

if (!IS_UART_DRIVER_ENABLE_INSTANCE(usart)) {

LOG_ERR("%s does not support driver enable", dev->name);

return -EINVAL;

}

uart_stm32_set_driver_enable(dev, true);

LL_USART_SetDEAssertionTime(usart, config->de_assert_time);

LL_USART_SetDEDeassertionTime(usart, config->de_deassert_time);

if (config->de_invert) {

LL_USART_SetDESignalPolarity(usart, LL_USART_DE_POLARITY_LOW);

}

}

#endif

Hence your implementation should take it into account and ensure it could be used by default over SW control if present.

I agree that the API should support hardware control (if the controller supports it), and this needs to be implemented.

My point is that hw control is already implemented in this driver. See

zephyr/drivers/serial/uart_stm32.c

Lines 2028 to 2043 in 5f418f5

#if HAS_DRIVER_ENABLE

if (config->de_enable) {

if (!IS_UART_DRIVER_ENABLE_INSTANCE(usart)) {

LOG_ERR("%s does not support driver enable", dev->name);

return -EINVAL;

}

uart_stm32_set_driver_enable(dev, true);

LL_USART_SetDEAssertionTime(usart, config->de_assert_time);

LL_USART_SetDEDeassertionTime(usart, config->de_deassert_time);

if (config->de_invert) {

LL_USART_SetDESignalPolarity(usart, LL_USART_DE_POLARITY_LOW);

}

}

#endif

Hence your implementation should take it into account and ensure it could be used by default over SW control if present.

Thank you for answer! I agree that this implementation exists, and I will undoubtedly change the approach in my implementation to ensure that the existing HW solution takes priority over SW automatically.

_test_plan_partial.json

_test_plan_partial.json removed from checked-in files.

erwango · 2025-01-20T09:51:07Z

drivers/serial/uart_stm32.c

+		if (clock_control_get_rate(data->clock, (clock_control_subsys_t)&config->pclken[1],
 					   &clock_rate) < 0) {
 			LOG_ERR("Failed call clock_control_get_rate(pclken[1])");
 			return;
 		}
 	} else {
-		if (clock_control_get_rate(data->clock,
-					   (clock_control_subsys_t)&config->pclken[0],
+		if (clock_control_get_rate(data->clock, (clock_control_subsys_t)&config->pclken[0],


Please don't mix format changes and functional changes.
Make a dedicated commit if for format changes

And I'd actually suggest to drop the format changes as the don't add anything.
Note that if you rebase on top of main, you'll see that they are not suggested anymore

erwango · 2025-01-20T09:53:19Z

drivers/serial/uart_stm32.c


 static int uart_stm32_fifo_fill_visitor(const struct device *dev, const void *tx_data, int size,
 					fifo_fill_fn fill_fn)
 {
 	const struct uart_stm32_config *config = dev->config;
 	USART_TypeDef *usart = config->usart;
-	int num_tx = 0U;
+	uint8_t num_tx = 0U;


Not linked to RS485. Please make it a dedicated commit.

erwango · 2025-01-20T09:56:15Z

drivers/serial/uart_stm32.c

+	if (config->de_enable) {
+		gpio_pin_set(config->de_pin.port, config->de_pin.pin, config->de_invert);
+	}
+


As mentioned, DE pins configurations (using alternate functions) are already available and RS485 support should not imply use of GPIO.
If you don't want to make use of built-in RS485 support (for which actually, I don't see specific interest), this should be protected under a dedicated flag as a SW implementation.

Thank you very much for the advice! I just don’t understand one point. My implementation is intended for cases where the device does not have hardware support for RS485. In the case of STM, this is already checked by "HAS_DRIVER_ENABLE." Maybe it’s not what I think?

erwango · 2025-01-20T09:58:56Z

drivers/serial/uart_stm32.c

+	if (config->de_enable) {
+		if (config->de_deassert_time_us) {
+			k_timer_start(&data->rs485_timer, K_USEC(config->de_deassert_time_us),
+				      K_NO_WAIT);
+		} else {
+			gpio_pin_set(config->de_pin.port, config->de_pin.pin, !config->de_invert);
+		}


See above comment for dedicated flag as a SW implementation.

@erwango Good afternoon! I think I've taken the current feedback into account. Could you please advise how this idea could be further improved or optimized? Thanks in advance!

erwango · 2025-04-14T11:33:25Z

dts/bindings/serial/uart-rs485-controller.yaml

+  rs485-de-gpios:
+    type: phandle-array
+    description: Output for contactor and relay 1


I'm raising this from STM32 perspective, but I'd be surprised we're the only vendor supporting RS485 HW Support:
It should be easy for user to configure and select use of built-in, RS485 HW support, vs RS485 SW support.
CONFIG_SERIAL_SUPPORT_RS485 isn't the correct way to make this configuration as:
1-Even when disabled HW RS485 is available and supported
2- you're selecting it by default (while IMO the built-in HW support should be the default)

I'd advise exploring the possibility to perform the HW/SW RS485 support selection through the use of rs485-de-gpios property. Being available, SW is used, otherwise user can rely on HW support

Hello! RS‑485 DE (Driver Enable) management is now based on the de_enable and de_pin fields:

If de_enable is false, no DE control is performed.

If de_enable is true and de_pin is NULL, the hardware method is used
(DE control via driver enable functions).

If de_enable is true and de_pin is not NULL, the software method is used
(GPIO‑based DE control).

erwango · 2025-04-14T11:34:44Z

drivers/serial/uart_stm32.c


-static void uart_stm32_poll_out_visitor(const struct device *dev, uint16_t out, poll_out_fn set_fn)
+static void uart_stm32_poll_out_visitor(const struct device *dev, void *out, poll_out_fn set_fn)


As already mentioned, please limit your changes to RS485 feature.
Other changes should at minimum go in another commit or better a dedicated PR.

Add support for configuring and enabling RS485 signaling by use software method of pin control. Signed-off-by: Sergey Grigorovich <[email protected]> Signed-off-by: Nachiketa Kumar <[email protected]> Signed-off-by: Antonio Tessarolo <[email protected]>

2345lug requested review from mmahadevan108 and dleach02 as code owners October 23, 2024 12:42

zephyrbot added area: Devicetree area: Samples Samples area: UART Universal Asynchronous Receiver-Transmitter platform: NXP Drivers NXP Semiconductors, drivers labels Oct 23, 2024

zephyrbot added the platform: STM32 ST Micro STM32 label Oct 23, 2024

zephyrbot requested review from danieldegrasse, dbaluta, dcpleung, decsny, erwango, FRASTM, gautierg-st, GeorgeCGV, kartben, manuargue, marwaiehm-st, mathieuchopstm and nashif October 23, 2024 12:43

zephyrbot assigned dcpleung Oct 23, 2024

2345lug force-pushed the rs485_api branch 6 times, most recently from 17b5001 to d2080d2 Compare October 28, 2024 06:07

erwango requested changes Nov 12, 2024

View reviewed changes

zephyrbot added the area: CAN label Nov 22, 2024

2345lug force-pushed the rs485_api branch from 3e05d2a to d2080d2 Compare November 22, 2024 11:41

henrikbrixandersen previously requested changes Nov 29, 2024

View reviewed changes

_test_plan_partial.json Outdated Show resolved Hide resolved

2345lug force-pushed the rs485_api branch 3 times, most recently from a74573e to 01c4dba Compare December 10, 2024 09:23

erwango requested changes Jan 20, 2025

View reviewed changes

2345lug force-pushed the rs485_api branch from ab11f30 to f7120f1 Compare February 23, 2025 21:33

2345lug force-pushed the rs485_api branch 3 times, most recently from 1e68707 to f5477f2 Compare April 3, 2025 12:14

2345lug force-pushed the rs485_api branch 2 times, most recently from c7ec712 to bc56492 Compare April 11, 2025 13:30

erwango reviewed Apr 14, 2025

View reviewed changes

2345lug force-pushed the rs485_api branch 2 times, most recently from dcc2cb5 to fa54683 Compare April 21, 2025 12:56

github-actions bot added platform: Infineon Infineon Technologies AG platform: Intel ADSP Intel Audio platforms area: DMA Direct Memory Access platform: Silabs Silicon Labs labels Apr 21, 2025

2345lug force-pushed the rs485_api branch from fa54683 to 93c9f16 Compare April 25, 2025 10:17

2345lug requested a review from erwango April 25, 2025 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rs485 APIs support #80305

Rs485 APIs support #80305

2345lug commented Oct 23, 2024

github-actions bot commented Oct 23, 2024

2345lug commented Nov 12, 2024

erwango Nov 12, 2024

2345lug Nov 15, 2024

erwango Nov 15, 2024

2345lug Dec 9, 2024

erwango Jan 20, 2025

erwango Jan 20, 2025

erwango Jan 20, 2025

erwango Jan 20, 2025

2345lug Feb 23, 2025

erwango Jan 20, 2025

2345lug Apr 14, 2025

erwango Apr 14, 2025

2345lug Apr 21, 2025

erwango Apr 14, 2025

	#if HAS_DRIVER_ENABLE
	if (config->de_enable) {
	if (!IS_UART_DRIVER_ENABLE_INSTANCE(usart)) {
	LOG_ERR("%s does not support driver enable", dev->name);
	return -EINVAL;
	}

	uart_stm32_set_driver_enable(dev, true);
	LL_USART_SetDEAssertionTime(usart, config->de_assert_time);
	LL_USART_SetDEDeassertionTime(usart, config->de_deassert_time);

	if (config->de_invert) {
	LL_USART_SetDESignalPolarity(usart, LL_USART_DE_POLARITY_LOW);
	}
	}
	#endif


		static void uart_stm32_poll_out_visitor(const struct device *dev, uint16_t out, poll_out_fn set_fn)
		static void uart_stm32_poll_out_visitor(const struct device dev, void out, poll_out_fn set_fn)

Rs485 APIs support #80305

Are you sure you want to change the base?

Rs485 APIs support #80305

Conversation

2345lug commented Oct 23, 2024

github-actions bot commented Oct 23, 2024

2345lug commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment