STM32 Serial driver status

[LuchianMihai]

Recently I've started to learn the internals of NuttX. I'm currently using nucleo f429zi board.
While looking at lower half stm32 serial driver I've noticed a few this that does not make sense, like:

• interrupt enable mask (ie) struct member as uint16_t shared for two 32 bit registers.
• up_txint guarded by SERIAL_HAVE_RXDMA_OPS define & up_rxint guarded by SERIAL_HAVE_TXDMA_OPS define, which I find odd, shouldn't it be reversed?
• up_dma_setup enables interrupts, while up_dma_rxint (which from docs should enable interrupts) does nothing. This may be due to limitation of my first point.
• up_*usartint functions are bit ackward to use, as stated above, I interact with two register using only one 16 bit variable.

I would like to try my hand at some refactorization, but I know nothing about the current status of this driver.

Would some changes be welcomed? If yes, which ones?

[acassis]

Hi @LuchianMihai welcome to NuttX!!!

Question: How did you discover about NuttX? Could you please give us details?

Thank you for bring these things to our attention. In the past many of those structures were limited to 16-bit, but later were extend to 32-bit to remove some limitations (Xiaomi did it for USERLEDS for example).

About the serial reversed maybe @davids5 and @raiden00pl could confirm, because there contributed a lot of code to this area.

I think re-factorization is always welcome, but keep in mind that you need to check if it will not break other boards. Since STM32 involves STM32 many families. Also many modifications should be replicated to STM32F7/H7 etc, that are in a separated arch directory.

[LuchianMihai]

Hello, Not really much to tell, I'm an embedded software engineer, linux user for the past 5 years. Got bored using FreeRTOS, started looking for something else, with more OS in it, found Zephyr, used it for a few months, a few personal projects. Really nice project, bit more organized than NuttX and in the same time way more chaotic, somehow I love their choices, both in Kconfig and DTS, but It's a bit to much mental overhead during the configuration part, so I've started searching again and found NuttX. For the moment I think it is just what I was looking for.

[LuchianMihai]

Won't be to many changes, maybe it's more of an question at first than an refactoring initiative, but if I am correct, then I can help by changing what is agreed.

[acassis]

@LuchianMihai excellent I think you will get bit by NuttX.

Are you subscribed to the NuttX mailing list? https://nuttx.apache.org/community/

Many discussions happen there, if you can, please post this question there because some users don't look here too often as in their emails

[raiden00pl]

Hi @LuchianMihai, improvements to stm32 are welcome, but please sync ALL stm32 families with your changes. The support for stm32 families in NuttX is a little mess, but they are all based on arch/stm32 port. This means that all these architectures should be modified:

• arch/stm32f7
• arch/stm32h7
• arch/stm32l4
• arch/stm32l5
• arch/stm32u5
• arch/stm32wb
• arch/stm32wl5
• arch/stm32f0l0g0

up_txint guarded by SERIAL_HAVE_RXDMA_OPS define & up_rxint guarded by SERIAL_HAVE_TXDMA_OPS define, which I find odd, shouldn't it be reversed?

I think this is correct. You have to consider all possible cases for DMA:

1. no RX DMA, no TX DMA
2. no RX DMA, TX DMA enabled
3. RX DMA enabled, no TX DMA
4. RX DMA enabled, TX DMA enabled

The case you are writing about applies to 2 and 3, where DMA is enabled only for one direction. So up_txint is required when:

1. no dma at all
2. no TX DMA

up_*usartint functions are bit ackward to use, as stated above, I interact with two register using only one 16 bit variable.

I agree this priv->ie looks weird. This is probably the case where something is implemented in a strange way, but it works, so it's safer not to touch it and no one had the initiative to fix it :)

[acassis]

@raiden00pl @LuchianMihai I think in this case it nice to include a comment before up_txint guarded by SERIAL_HAVE_RXDMA_OPS explaining its logic, because other developers could face same situation in the future

[LuchianMihai]

@raiden00pl Oh, got it (the thing about SERIAL_HAVE_RXDMA_OPS / SERIAL_HAVE_TXDMA_OPS), but I've already touched that part with my changes Is there any reason we use reversed logic? Or, my reasoning is that if we have rx dma enabled, use up_dma_rx, else use up_rx, same for tx dma. This logic works on my branch and I think it's easier to follow. Now there is another issue, I've forked nuttx, you can check my work progress there, but what is the procedure for an pull request?

L.E.: I've completed my changes (described in this issue). For now I've only modified stm32 directory. You can find my changes here https://github.com/LuchianMihai/nuttx/tree/stm32_serial_improvements. Please have a look over them, if they seem ok, I can also open an pull request. I intentionally have not ported my changes to other stm32 families, as it will be more work during review phase, and more changes to omit things. If we get to the point that the review is approved, I'll start porting throughout stm32 families.