Skip to content

Conversation

@GeorgeCGV
Copy link
Contributor

Adds 'err_check' implementation for STM32 serial driver.

@GeorgeCGV GeorgeCGV requested a review from erwango as a code owner February 7, 2019 21:29
@galak galak added area: UART Universal Asynchronous Receiver-Transmitter platform: STM32 ST Micro STM32 labels Feb 7, 2019
@codecov-io
Copy link

codecov-io commented Feb 7, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #13152   +/-   ##
=======================================
  Coverage   51.96%   51.96%           
=======================================
  Files         309      309           
  Lines       45576    45576           
  Branches    10554    10554           
=======================================
  Hits        23683    23683           
  Misses      17083    17083           
  Partials     4810     4810

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 5a56ee5...c4b8d63. Read the comment docs.

@GeorgeCGV GeorgeCGV changed the title drivers: serial: stm32: add err_check [WIP] drivers: serial: stm32: add err_check Feb 8, 2019
@GeorgeCGV GeorgeCGV force-pushed the driver_serial_stm32_error_chk branch 2 times, most recently from a131338 to 82a1001 Compare February 8, 2019 12:03
@GeorgeCGV GeorgeCGV changed the title [WIP] drivers: serial: stm32: add err_check drivers: serial: stm32: add err_check Feb 8, 2019
@GeorgeCGV
Copy link
Contributor Author

GeorgeCGV commented Feb 8, 2019

Regarding this PR. It checks for erros consequentially and sets specific bits specified by the uart_rx_stop_reason. This contradicts will all other uart api implementations of the error check function.

Majority of implementations check for error sequentially but with one mistake...
They treat uart_rx_stop_reason values as corresponding bits.

e.g. gecko

	int err = 0;

	if (flags & USART_IF_RXOF) {
		err |= UART_ERROR_OVERRUN;
	}

	if (flags & USART_IF_PERR) {
		err |= UART_ERROR_PARITY;
	}

	if (flags & USART_IF_FERR) {
		err |= UART_ERROR_FRAMING;
	}

but because uart_rx_stop_reason values are simple integers the error won't be accumulative.
At the end we will have the last error reported and all flags cleared.

This looks like a mistake.. As either uart_rx_stop_reason values shall represent bits (e.g. UART_BREAK = 1 << 1, UART_ERROR_OVERRUN = 1 << 2) or err_check shall return as soon as it detects one of the flags and clears it, but this will affect the user as err_check would have to be called multiple times until all errors are cleared.

@GeorgeCGV GeorgeCGV force-pushed the driver_serial_stm32_error_chk branch 3 times, most recently from 2db8371 to 5c3d2d4 Compare February 8, 2019 19:01
@GeorgeCGV
Copy link
Contributor Author

GeorgeCGV commented Feb 8, 2019

Pull request #10820 brakes previously defined UART error bitmasks, which are now within uart_rx_stop_reason enum as simple integers.

@GeorgeCGV GeorgeCGV force-pushed the driver_serial_stm32_error_chk branch from 5c3d2d4 to 35ccf5b Compare February 8, 2019 21:13
@erwango
Copy link
Member

erwango commented Feb 11, 2019

Pull request #10820 brakes previously defined UART error bitmasks, which are now within uart_rx_stop_reason enum as simple integers.

Raised #13256 for this issue.
Let's see what will be the feedback before going further.

@GeorgeCGV GeorgeCGV force-pushed the driver_serial_stm32_error_chk branch 3 times, most recently from 8e8767d to 788c947 Compare February 14, 2019 17:50
@GeorgeCGV GeorgeCGV force-pushed the driver_serial_stm32_error_chk branch 2 times, most recently from ba5cb1b to 60f9349 Compare March 5, 2019 01:59
@GeorgeCGV GeorgeCGV force-pushed the driver_serial_stm32_error_chk branch from 60f9349 to 919a4d5 Compare March 12, 2019 21:07
Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Minor comment

@GeorgeCGV GeorgeCGV force-pushed the driver_serial_stm32_error_chk branch from 919a4d5 to c4b8d63 Compare March 15, 2019 19:24
@erwango erwango added this to the v1.15.0 milestone Mar 18, 2019
@GeorgeCGV GeorgeCGV force-pushed the driver_serial_stm32_error_chk branch from c4b8d63 to ce5f577 Compare April 10, 2019 11:30
Adds 'err_check' implementation.

Signed-off-by: Georgij Cernysiov <[email protected]>
@galak galak force-pushed the driver_serial_stm32_error_chk branch from ce5f577 to 728a978 Compare April 17, 2019 20:13
@galak galak merged commit c74c131 into zephyrproject-rtos:master Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: UART Universal Asynchronous Receiver-Transmitter platform: STM32 ST Micro STM32

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants