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

Fix USART length setting for odd/even parity setups. #562

Closed
wants to merge 1 commit into from

Conversation

cajt
Copy link
Contributor

@cajt cajt commented Feb 18, 2021

The prior code set the length to incorrectly if odd or even parity was used.
The fix works fine on the stm32f042. There is also an example that uses an USART with parity, I also checked the output on a scope.

The #ifdef is copied from "setWordLength", but i couldn't test it with a STM32 that doesn't have USART_CR1_M1.

Add an example that uses a USART with parity.
@salkinium
Copy link
Member

Even worse, setWordLength() (always available) was confused with setLastBitClockPulse (USART only for SPI mode).

}
{{ peripheral }}->CR1 = flags;
{{ peripheral }}->CR1 = ({{ peripheral }}->CR1 & ~(USART_CR1_PCE | USART_CR1_PS)) | static_cast<uint32_t>(parity);
setWordLength(parity == Parity::Enabled ? WordLength::Bit9 : WordLength::Bit8);
Copy link
Member

@salkinium salkinium Feb 18, 2021

Choose a reason for hiding this comment

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

This isn't a good API, since technically you can have parity on 7-bit data (like ASCII) and then you get a 8-bit word. I would prefer to remove this line here. What do you think?

Update: and add the word length to the constructor?

@chris-durand
Copy link
Member

chris-durand commented Feb 18, 2021

Update: and add the word length to the constructor?

@salkinium Do you mean the initialize() method? The Hal class only has static members and no user-defined constructor. Then I would agree. setParity() is not even public, so we are free to change its behaviour.

I would also add documentation on setWordLength() to clarify whether the word length there includes the parity bit or not. If we add the parameter to the initialization function we should be consistent with that.

@salkinium
Copy link
Member

salkinium commented Feb 19, 2021

Sorry to hijack this PR, however, the UART driver was triggering my OCD and I had to clean it up. I also made each buffer individually configurable, so that you don't need to generate the Receiver buffer if you only every transmit something (like 90% of the logger use cases).

I can test 9-bit transfers with parity for my AMNB network between a bunch of G071 and a F411.

@salkinium salkinium force-pushed the fix-stm32-uart-parity branch 3 times, most recently from 989d41f to eb4a9d7 Compare February 19, 2021 22:19
@salkinium
Copy link
Member

I'm closing this is favor of #564.

Thanks for reminding me to refactor this terrible code!

@salkinium salkinium closed this Feb 19, 2021
@thestumbler
Copy link

What was the final decision on the data length definition? My application requires even parity, and what would normally be called 8 data bits, which means as noted above, there are a total of 9 bits between the start and stop bits. It seems I have to put data bits of 9 to make it work, but that seems contrary to what I believe is the industry standard definition.

@salkinium
Copy link
Member

Data length includes the parity bit because we followed the ST mechanism. Example usage

@thestumbler
Copy link

I was surprised by this, but looking at a few F1 and F4 family chips, I see how one can interpret it this way. They've blurred the distinction between data bit size as specified in the M-bit with parity enable PCE-bit. But if you look at the table 146 (I'm looking at the F405 manual, RM0090) they do note the true bit size in the text remarks.

In my experience, this is very unusual. I've never seen the parity bit included in the M-bit(s) data size field of line control registers before. I spot-checked a few other MCU data sheets, like RP2040, SAM-E53, ATmega328, and their underlying way of handling parity is in-line with my understanding of industry standards ... that is, the existence or not of a parity bit has no effect on the data character word size. Looking at data sheets of individual chips, this convention seems to go all the way back to the first UART chip, the 1971 Western Digital 1402A. Later, the 1981 National Semiconductor 8250 and successors like the 16450, etc., continued this convention.

Furthermore, the kind of information someone gets to describe the characteristics of an asynchronous serial link is usually expressed like 115200 8N1. I'd guess this convention is even less formally documented than the description of the M-bits field (maybe it comes from modems?). But in my experience, parity of N means there simply will not be a parity bit in the frame, and doesn't change the character size. Likewise, all the others, odd, even, mark, and space, while adding one extra bit to the frame, don't change the character size.

Regardless of how a particular UART implements this mechanism, I would suggest that presenting the traditional representation to the user in the API calls is a preferable approach. I realize, however, at this point, it could be a breaking change for MODM users, and I have no opinion on how to deal with that problem.

@salkinium
Copy link
Member

I realize, however, at this point, it could be a breaking change for MODM users, and I have no opinion on how to deal with that problem.

Oh, that's easy: We have a detailed changelog for that purpose, were we simply make breaking changes every ones elses problem, bwarharharhar! (we try to be sensible though).

I'm happy to merge a change of it, I don't like the current way either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants