modbus: serial: Ignore received data when reception is disabled

[msalau]

A byte received when reception has been disabled
corrupts internal state of the server
(e.g. during transmission of a reply in server mode). The reponse packet is corrupted and its transmission is aborted and the data in the buffer is treated by the server as a new incoming packet. Since the buffer is corrupted CRC doesn't match and the following log message is printed:

modbus_serial: Calculated CRC does not match received CRC

This condition happens when uart_irq_rx_ready() returns true if there is a new byte in the receive FIFO even with disabled RX interrupt.

The issue has been discovered on a nucleo_u083rc board with a RS485 transceiver with the RI signal floating (a pull-down gives more stable reproduction). The pull-down ensures that RI is low during transmission which is seen as byte 0 with a framing error by the receiver. The byte is received by the MCU and corrupts the response. Similar effect can be achieved by not disabling the receiver during transmission (i.e. nRE is driven by the MCU and is fixed low).

The fix discards any data received when reception has been disabled.

[msalau]

I think the pull-request should fix #42366

[msalau]

Tested the change with RE=0

Responses are sent correctly and no warnings are present in the log.

[msalau]

Hello @jfischer-no
Thank you for feedback.

It seems I found a proper way to drain the RX FIFO before enabling reception.
I've added a function to drain the RX FIFO. It is used in RX handler when reception is disabled and in the TX complete handler right before enabling the RX interrupt.

Tested the change in 2 modes:

• uart_irq_rx_ready() == true when FIFO is not empty but RX IRQ is disabled (e.g. stm32)
• uart_irq_rx_ready() == false when RX IRQ is disabled (e.g. silabs, renesas ra)

Both modes were tested using an stm32 board. For the second one uart_stm32_irq_rx_ready() has been patched in the following way:

static int uart_stm32_irq_rx_ready(const struct device *dev)
{
	const struct uart_stm32_config *config = dev->config;
	/*
	 * On stm32 F4X, F1X, and F2X, the RXNE flag is affected (cleared) by
	 * the uart_err_check function call (on errors flags clearing)
	 */
-	return LL_USART_IsActiveFlag_RXNE(config->usart);
+	return LL_USART_IsActiveFlag_RXNE(config->usart) && LL_USART_IsEnabledIT_RXNE_RXFNE(config->usart);
}

[Copilot]

Pull Request Overview

This PR fixes a bug where stale UART data corrupts the response packet when reception is disabled. Key changes include adding a function to drain the RX FIFO, updating RX enabling/disabling logic with atomic state tracking, and updating internal definitions to support the new state.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
subsys/modbus/modbus_serial.c	Added a FIFO drain function and updated RX on/off and TX handlers
subsys/modbus/modbus_internal.h	Defined MODBUS_STATE_RX_ENABLED to track reception state

[jfischer-no]

Looks like a multiple review failure. A human ask review by trashpilot, trashpilot made a trash suggestion, and the humans blindly followed it.

[kartben]

Looks like a multiple review failure. A human ask review by trashpilot, trashpilot made a trash suggestion, and the humans blindly followed it.

And in what world is that calling for using such language?
Also, please do state clearly what is your request for change (e.g. going back to commit xyz). Throwing a random -1 is not helping.
https://docs.zephyrproject.org/latest/contribute/reviewer_expectations.html#:~:text=Reviewers%20shall%20be%20clear%20about%20what%20changes%20they%20are%20requesting%20when%20the%20%E2%80%9CRequest%20Changes%E2%80%9D%20option%20is%20used

[jfischer-no]

Looks like a multiple review failure. A human ask review by trashpilot, trashpilot made a trash suggestion, and the humans blindly followed it.

And in what world is that calling for using such language?

That's fine in any world that values concise and honest communication. You seem to have a problem with this, please work on it.

Also, please do state clearly what is your request for change (e.g. going back to commit xyz). Throwing a random -1 is not helping. https://docs.zephyrproject.org/latest/contribute/reviewer_expectations.html#:~:text=Reviewers%20shall%20be%20clear%20about%20what%20changes%20they%20are%20requesting%20when%20the%20%E2%80%9CRequest%20Changes%E2%80%9D%20option%20is%20used

It is a line change between my reviews, which I am pretty sure is very easy to figure out. Also, before you blame me further, note that it was your actions that caused me to request changes.

[kartben]

Looks like a multiple review failure. A human ask review by trashpilot, trashpilot made a trash suggestion, and the humans blindly followed it.

And in what world is that calling for using such language?

I suggested that maybe the comment was valid, @msalau, who is in a position to actually test things, thought so too, how does that make either of us "humans blindly following the suggestions"?
To be perfectly clear, I would have more than likely made the exact same suggestion without Copilot -- so yeah sh** happens and sometimes reviewers make wrong suggestions, just like sometimes reviewers might miss things that others catch. That's the whole point of the review process, I think.

[kartben]

Also, please do state clearly what is your request for change (e.g. going back to commit xyz). Throwing a random -1 is not helping. https://docs.zephyrproject.org/latest/contribute/reviewer_expectations.html#:~:text=Reviewers%20shall%20be%20clear%20about%20what%20changes%20they%20are%20requesting%20when%20the%20%E2%80%9CRequest%20Changes%E2%80%9D%20option%20is%20used

It is a line change between my reviews, which I am pretty sure is very easy to figure out. Also, before you blame me further, note that it was your actions that caused me to request changes.

I guess I was simply hoping you would comment on the why it is wrong, since it's apparently so obvious to you the change is "trash" and yet it is not necessarily the case for at least 2 of the people involved in this PR who could maybe learn something for the future (plus anyone else stumbling open this PR in the future).

[msalau]

Hello @jfischer-no

I don't think that the change is unreasonable. Unfortunately I see discrepancies between documentation and actual behavior regularly and tend to be over-cautious in such cases.

The change decreases performance by making one additional syscall to confirm that FIFO is empty. I thought this is not a big deal.

I'll revert the change.

Thanks and regards
Maksim

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[jfischer-no]

I don't think that the change is unreasonable. Unfortunately I see discrepancies between documentation and actual behavior regularly and tend to be over-cautious in such cases.

It clearly is, no doubts. It does not matter what capabilities specific UART controller/driver has, the change was incorrect, not overcautious.