search

ARMmbed / mbed-os

Arm Mbed OS is a platform operating system designed for the internet of things
https://mbed.com
Other
4.67k stars 2.98k forks source link

STM32s Serial does not properly handle parity bits. #4189

Closed Sissors closed 7 years ago

Sissors commented 7 years ago

Description

Bug

Target STM32s, haven't checked if all suffer from it.

Toolchain: All

Expected behavior Enabling parity bit adds a parity bit to the transmission.

Actual behavior Enabling parity bit replaces a bit for the parity bit in the transmission. If you look at the serial format code, https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32F0/serial_api.c#L300, you see 9-bit transmission is only enabled if the format is 9 data bits. However according to the reference manual, the number of data bits is including the parity bit: So if you have it set on 8-bits with parity, it will send 7 data bits and the parity. So if you want 8 data bits and parity, you need to set it on 9-bit data. If you call the mbed serial format function with 9 data bits and parity, it indeed correctly sends 8 data bits and parity. But obviously you should be able to set the format to 8 data bits and have a proper output format.

Kudos to: https://gathering.tweakers.net/forum/list_message/50904807#50904807 for immediatly knowing what the issue was.

Steps to reproduce Send a serial packet with parity enabled and check with logic analyzer/serial terminal on PC.

fmanno commented 7 years ago

I agree with @Sissors that the way of specifying the configuration is confusing. Serial applications usually specify configuration in the format <data_length - parity - stop_bits> e.g. 8-N-1, 7-E-2, etc. I.e. the parity bit is excluded from the data_bits count. However from the reference manuals of both STM32F0 and STM32F4 It says that when parity is enabled, it replaces the MSB of the data_length. Thus implying that with parity disabled there's actually up to 9 bits usable length for the serial data transmissions. In those cases always reserving one bit for the parity in the format say 8-N-1 doesn't seem efficient as some applications may benefit from the extra bit, who knows.

Having said that, it's my interpretation that mbedOS aims mainly at portability between different chips and boards and keeping that in mind I've been looking at the MAX32625 and the SAMG55 user manuals and in both cases the data_length excludes the parity bit (i.e. they stick to the 8-N-1 style configuration). So I believe that the serial_api.c implementation for the STM32 family should also follow that way of specifying the format or users might get a nasty surprise when running their app in an STM32 platform if they used another one before.

I've created Pull request 4216 with a change that addresses this.

0xc0170 commented 7 years ago

cc @bcostm @adustm @LMESTM @jeromecoutant

LMESTM commented 7 years ago

@fmanno - thank you for the PR - this looks good to me