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

RFC draft - patch to add IrDA SIR mode support to the usart code. #466

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tim-seoss
Copy link
Contributor

A few queries about this:

  • Should it be behind a feature flag?
  • Should IrdaMode be replaced with SpecialMode or similar, and support all of IrDA, LIN and Smartcard?

Any comments welcome!

n.b. I'm seeing some weird behaviour in transmit mode (missing or truncated start bit pulse) - which I'm currently debugging - might be a clock setup issue outside of the serial driver code.

@tim-seoss tim-seoss marked this pull request as draft March 15, 2022 15:35
@burrbull
Copy link
Contributor

  • Should it be behind a feature flag?

In release mode it should be optimized out if not used. So I think No.

@tim-seoss
Copy link
Contributor Author

Re f405 tests failing - the docs say that register should be present, so maybe an SVD error? Is that likely?

@burrbull
Copy link
Contributor

Re f405 tests failing - the docs say that register should be present, so maybe an SVD error? Is that likely?

No. It present in usart1.
There is bigger problem. It is because of:

use crate::pac::uart4 as uart_base;

@tim-seoss
Copy link
Contributor Author

tim-seoss commented Mar 15, 2022

Re f405 tests failing - the docs say that register should be present, so maybe an SVD error? Is that likely?

No. It present in usart1. There is bigger problem. It is because of:

use crate::pac::uart4 as uart_base;

OK, thanks, so this needs to be conditionally not compiled for uarts (as opposed for usarts) [edit: this was incorrect - see below - the SVDs were missing the IrDA fields for UARTs - see https://github.com/stm32-rs/stm32-rs/pull/713 ], I'll take a look at that once I've worked out what's going on with the mystery of the missing start bits.

@burrbull
Copy link
Contributor

OK, thanks, so this needs to be conditionally not compiled for uarts (as opposed for usarts), I'll take a look at that once I've worked out what's going on with the mystery of the missing start bits.

I don't think it will be easy with current implementation.
Looks like it is time to split implementations of USARTs and UARTs.

@burrbull
Copy link
Contributor

I think I found some workaround witch don't requires total rework.

Instead of adding this to new just create other constructor function (for example irda) with advanced restriction:

USART: Instance + core::ops::Deref<Target = crate::pac::usart1::RegisterBlock>

Something like ab28a0a

@burrbull
Copy link
Contributor

Also in this case you don't need IrdaMode::None so you can optimize it.

@tim-seoss
Copy link
Contributor Author

tim-seoss commented Mar 18, 2022

The plot thickens...

According to RM0090 30.5 USART mode configuration (page 1007 on rev 19), IrDA mode is supported on UART peripherals as well USART peripherals.

30.6.7 Guard time and prescaler register (USART_GTPR) (p. 1017) suggests that this register is unavailable on UARTs, which would (I think) make IrDA low power mode non-functional on UARTs (but there's no mention of that elsewhere in the documentation). I have an f446 here, so out of curiosity, I'll do some experimenting with low power mode to see if the register is really not there...

n.b. The GTPR register is also used for Smartcard mode - which is not present on UART peripherals.

@tim-seoss
Copy link
Contributor Author

I noticed that the GTPR register is included in the UART4 peripheral of the stm32f413, but is missing on the other models which include UART4: https://stm32-rs.github.io/stm32-rs/stm32f/stm32f4/UART4_0x40004C00.html

I only have an stm32f446 here, so I tried writing to the GTPR register to see if it would have any effect. It works exactly as expected (writing to the PCS bits of the location where the GTPR reg "should" be does change the transmit pulse length in IrDA low-power mode, exactly as it does on USART peripherals on the 446, and on the 411), so I suspect that this omission is just an SVD bug, and I'll submit a PR to stm32-rs.

As to why the register is missing from the SVDs... The GTPR register is used in IrDA mode, and also in Smartcard mode. The UART peripheral includes IrDA support, but doesn't include Smartcard support, so I think it was accidentally omitted from the SVD because of this (the wording in the reference manual is ambiguous, so maybe this is the original source of the error).

@tim-seoss
Copy link
Contributor Author

tim-seoss commented Apr 2, 2022

PAC issue:
blocked-by: stm32-rs/stm32-rs#711

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

Successfully merging this pull request may close these issues.

None yet

2 participants