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.

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

uart_configure does not return -ENOTSUP for stm32 uart with 9 bit data length. #31799

Closed
zisisadamos opened this issue Jan 29, 2021 · 13 comments · Fixed by #32204
Closed

uart_configure does not return -ENOTSUP for stm32 uart with 9 bit data length. #31799

zisisadamos opened this issue Jan 29, 2021 · 13 comments · Fixed by #32204
Assignees
Labels
area: UART Universal Asynchronous Receiver-Transmitter bug The issue is a bug, or the PR is fixing a bug platform: STM32 ST Micro STM32 priority: low Low impact/importance bug

Comments

@zisisadamos
Copy link
Contributor

Describe the bug
When we use uart_configure on STM32 for a device with data length of 9 bits we dont get -ENOTSUP

To Reproduce
Steps to reproduce the behavior:

  1. Create a 9bit configuration for uart device
  2. use that device as an argument for uart_configure
  3. Bug. Seems like device is initialized and supports 9bit.

Expected behavior
Return -ENOTSUP

Impact
Not that critical. Lack of 9bit support is more critical.

Environment (please complete the following information):

  • OS: Linux
  • Toolchain Zephyr SDK
  • Commit 95712bd
@zisisadamos zisisadamos added the bug The issue is a bug, or the PR is fixing a bug label Jan 29, 2021
@nashif nashif added priority: low Low impact/importance bug platform: STM32 ST Micro STM32 labels Jan 30, 2021
@tagunil
Copy link
Collaborator

tagunil commented Jan 30, 2021

Actually the driver itself supports configuring UART to 9 bit mode. Looks like it is the API that is missing.

@erwango
Copy link
Member

erwango commented Feb 1, 2021

@zisisadamos I'm a bit confused with this issue. And the fix provided didn't seem correct.
Can you elaborate a bit on the issue you're facing?

@erwango erwango added the area: UART Universal Asynchronous Receiver-Transmitter label Feb 1, 2021
@zisisadamos
Copy link
Contributor Author

Definately. So I am trying to do 9bit data transfers(excluding parity and stop bits) with zephyr. Which at least for stm32 driver doesn't seem to be supported(all transfer functions i found on the driver are for 8 bits). So I assumed that when i try to configure serial for 9bit data width it should return -ENOTSUP. Which it doesnt.

@erwango
Copy link
Member

erwango commented Feb 1, 2021

ok, got it. Then I think UART_CFG_DATA_BITS_9 shouldbe purely removed for the API. @dcpleung do you confirm ?

@zisisadamos
Copy link
Contributor Author

zisisadamos commented Feb 1, 2021

I think we can do 9bit as well I was thinking of implementing it. Just wanted to do that fix for now so I get familiar with contribution flow. 9bit is only impossible for poll api of zephyr because the specify the input type and dont use pointer(poll_out specifically).

@dcpleung
Copy link
Member

dcpleung commented Feb 1, 2021

ok, got it. Then I think UART_CFG_DATA_BITS_9 shouldbe purely removed for the API. @dcpleung do you confirm ?

I think it would be better to extend the API to support 9 bit transfers, for example, using uint16_t instead of uint8_t for buffer type. So I think adding new API functions to do uint16_t when some kconfig is enabled would be the way to go.

@zisisadamos
Copy link
Contributor Author

ok, got it. Then I think UART_CFG_DATA_BITS_9 shouldbe purely removed for the API. @dcpleung do you confirm ?

I think it would be better to extend the API to support 9 bit transfers, for example, using uint16_t instead of uint8_t for buffer type. So I think adding new API functions to do uint16_t when some kconfig is enabled would be the way to go.

I was thinking about casting the pointer according to configuration. That would only require poll_write to change.

@dcpleung
Copy link
Member

dcpleung commented Feb 1, 2021

The send/receive functions all takes 8-bit character or 8-bit byte array so 9-bit data cannot be sent through them. That's why I think we will need a new set of API which takes uint16_t or uint16_t *. New APIs also avoid the issue with changing current stable API. Also avoid using uint16_t if the app only ever needs 8-bit to save a few bytes for buffers.

@zisisadamos
Copy link
Contributor Author

The send/receive functions all takes 8-bit character or 8-bit byte array so 9-bit data cannot be sent through them. That's why I think we will need a new set of API which takes uint16_t or uint16_t *. New APIs also avoid the issue with changing current stable API. Also avoid using uint16_t if the app only ever needs 8-bit to save a few bytes for buffers.

All of them except uart_poll_out accept pointers. So if driver does cast uint8_t* to uint16_t* based on configuration why do we need to change APIs? We can only change uart_poll_out to accept a pointer instead of an actual value. Then driver can handle it internally.

@dcpleung
Copy link
Member

dcpleung commented Feb 1, 2021

Conceptually, the array represents a series of datum to send or receive. In 8-bit mode, it's just an array of 8-bit characters. When the driver arbitrarily type casts the input, it becomes less clear on what is expected from the app. Does one datum occupy 2 bytes? Or do you pack multiple 9-bit data continuous into the array? I simply wish the API to be straight-forward, so app developers do not have to have in-depth knowledge of the drivers.

@erwango
Copy link
Member

erwango commented Feb 3, 2021

@zisisadamos @dcpleung I propose we create a dedicated UART API issue for the 9 bit support.
#31798 could be pursued but requires rework.

zisisadamos added a commit to zisisadamos/zephyr that referenced this issue Feb 6, 2021
…TSUP.

Fixes driver incorrectly indicating that drive
r supports 7 and 9 bit modes.

Fixes zephyrproject-rtos#31799

Signed-off-by: Zisis Adamos <[email protected]>
@easternblack
Copy link

I think configuring PARITY bits in UART has similar issue.
UART only work with NONE parity, but not with ODD or EVEN.
I tested on stm32l433cb, and /include/stm32l4xx_ll_usart.h include EVEN and ODD defines.
I also experienced data bit issue explained above, and now have doubt with PARITY bit configure has same problem.

@erwango
Copy link
Member

erwango commented Feb 9, 2021

@easternblack Don't hesitate to raise a dedicated issue so we don't lose the point.

erwango added a commit to erwango/zephyr that referenced this issue Feb 11, 2021
STM32 uart driver doesn't support 9bits transactions in any case,
so remove case were it was declared as supported.

Fixes zephyrproject-rtos#31799

Signed-off-by: Erwan Gouriou <[email protected]>
nashif pushed a commit that referenced this issue Feb 14, 2021
STM32 uart driver doesn't support 9bits transactions in any case,
so remove case were it was declared as supported.

Fixes #31799

Signed-off-by: Erwan Gouriou <[email protected]>
github-actions bot pushed a commit that referenced this issue Feb 14, 2021
STM32 uart driver doesn't support 9bits transactions in any case,
so remove case were it was declared as supported.

Fixes #31799

Signed-off-by: Erwan Gouriou <[email protected]>
nashif pushed a commit that referenced this issue Feb 19, 2021
STM32 uart driver doesn't support 9bits transactions in any case,
so remove case were it was declared as supported.

Fixes #31799

Signed-off-by: Erwan Gouriou <[email protected]>
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 bug The issue is a bug, or the PR is fixing a bug platform: STM32 ST Micro STM32 priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants