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

Refactor uart #13585

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Refactor uart #13585

wants to merge 20 commits into from

Conversation

ledvinap
Copy link
Contributor

  • serial - refactor serial resource handling

    • port related resources (tx/rx/inverted,dma)
      • are stored sequentially, with space in place of ports that are not enabled (RESOURCE_UART_COUNT + RESOURCE_LPUART_COUNT + RESOURCE_SOFTSERIAL_COUNT)
      • RESOURCE_UART_OFFSET, RESOURCE_LPUART_OFFSET, RESOURCE_SOFTSERIAL_OFFSET
      • resource entries are pointing into this array (UART, LPUART, SOFTSERIAL)
        • both pins and DMA
        • inverter is supproted only for UART + LPUART (makes sense only for UART, to be removed)
      • softSerialPinConfig is removed, it is no longer necessary
    • serialResourceIndex(identifier) is used universally to get correct index into resource array
    • unified handling of resources where possible
    • serialOwnerTxRx() + serialOwnerIndex() are used for displaying resources correctly to user.
    • serialType(identifier) implemented
      • serialOwnerTxRx / serialOwnerIndex are trivial with it
      • large switch() statemens are greatly simplified
  • serial - merge code duplicated in all UART implementations

    • drivers/serial_uart_hw.c contains merged serialUART code.
      Code did not match exactly. Obvious cases are fixed, more complicated use #ifs
    • pin inversion unified
    • uartOpen is refactored a bit
  • serial - refactor uartDevice

    • use 'compressed' enum from uartDeviceIdx_e. Only enabled ports are in this enum.
    • uartDeviceIdx directly indexes uartDevice (no search necessary, no wasted space)
    • use serialPortIdentifier_e identifier; for uartHardware
    • for DMA remap, define only entries for enabled ports (uartDeviceIdx_e does not exist disabled port)
  • serial - apply changes to inverter
    New implementation is trivial

  • serial - HAL - rxIrq, txIrq replaces by irqn
    There is only one IRQ for serial

  • serial - serial_post.h
    Generated code to normalize target configuration.
    jinja2 template is used to generate header file. Port handling is unified a lot.

    SERIAL_<type><n>_USED 0/1 - always defined, value depends on target configuration
    SERIAL_<type>_MASK - bitmask of used ports or given type. 1 is BIT(0)
    SERIAL_<type>_COUNT - number of enabled ports of given type
    SERIAL_<type>_MAX - + 1, 0 when no port is enabled

  • targets - remove automatically calculated valued from configs
    serial_post.h generated them

  • serial - remove definitions replaced by serial_post.h

  • serial - change LPUART to UART_LP1 in places
    LPUART is mostly handled as another UART port, this change reflects it

  • serial - use ARRAYLEN / ARRAYEND in some places
    replaces constant that may not match actual array size

  • serial - adapt softserial to changes

  • serial - whitespace, comments

  • softserial - fix serialTimerConfigureTimebase

  • serial - suspicious code

  • serial - unittests

  • serial - cleanup

  • serial - simpler port names
    Is this useful ?

- port related resources (tx/rx/inverted,dma)
  - are stored sequentially, with space in place of ports that are not
  enabled (RESOURCE_UART_COUNT + RESOURCE_LPUART_COUNT +
     RESOURCE_SOFTSERIAL_COUNT)
  - RESOURCE_UART_OFFSET, RESOURCE_LPUART_OFFSET, RESOURCE_SOFTSERIAL_OFFSET
  - resource entries are pointing into this array (UART, LPUART, SOFTSERIAL)
    - both pins and DMA
    - inverter is supproted only for UART + LPUART (makes sense only
     for UART, to be removed)
  - softSerialPinConfig is removed, it is no longer necessary
- serialResourceIndex(identifier) is used universally to get correct
   index into resource array
- unified handling of resources where possible
- serialOwnerTxRx() + serialOwnerIndex() are used for displaying resources
   correctly to user.
- serialType(identifier) implemented
  - serialOwnerTxRx / serialOwnerIndex are trivial with it
  - large switch() statemens are greatly simplified
- drivers/serial_uart_hw.c contains merged serialUART code.
  Code did not match exactly. Obvious cases are fixed, more complicated
  use #ifs
- pin inversion unified
- uartOpen is refactored a bit
- use 'compressed' enum from uartDeviceIdx_e. Only enabled ports are
   in this enum.
- uartDeviceIdx directly indexes uartDevice (no search necessary, no
    wasted space)
- use `serialPortIdentifier_e identifier;` for uartHardware
- for DMA remap, define only entries for enabled ports (uartDeviceIdx_e
   does not exist disabled port)
New implementation is trivial
There is only one IRQ for serial
Generated code to normalize target configuration.
jinja2 template is used to generate header file. Port handling is
unified a lot.

SERIAL_<type><n>_USED 0/1 - always defined, value depends on target configuration
SERIAL_<type>_MASK        - bitmask of used ports or given type. <port>1 is BIT(0)
SERIAL_<type>_COUNT       - number of enabled ports of given type
SERIAL_<type>_MAX         - <index of highest used port> + 1, 0 when no port is enabled
LPUART is mostly handled as another UART port, this change reflects it
replaces constant that may not match actual array size
Copy link

Do you want to test this code? You can flash it directly from Betaflight Configurator:

  • Simply put #13585 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

@ledvinap
Copy link
Contributor Author

A bit bigger PR .. but it removes ~930 lines of code (generated serial_post.h is 530 lines)

I can cleanup it a bit or remove more controversial parts.

include serial_post.h, some ports are defined, so normalization will
work fine
#ifdef USE_LPUART1
DEFA( OWNER_LPUART_TX, PG_SERIAL_PIN_CONFIG, serialPinConfig_t, ioTagTx[SERIAL_UART_COUNT], SERIAL_LPUART_COUNT ),
DEFA( OWNER_LPUART_RX, PG_SERIAL_PIN_CONFIG, serialPinConfig_t, ioTagRx[SERIAL_UART_COUNT], SERIAL_LPUART_COUNT ),
#if defined(USE_LPUART)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#if defined(USE_LPUART)
#if defined(USE_LPUART1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

USE_LPUART is intended. Port use is normalized, it will work for any number of ports of given type(VCP is partial exception).

#endif

#if defined(USE_UART) || defined(USE_SOFTSERIAL)
#if defined(USE_UART) || defined(USE_LPUART) || defined(USE_SOFTSERIAL)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#if defined(USE_UART) || defined(USE_LPUART) || defined(USE_SOFTSERIAL)
#if defined(USE_UART) || defined(USE_LPUART1) || defined(USE_SOFTSERIAL)

Copy link
Member

Choose a reason for hiding this comment

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

Only have one LPUART at the moment - but we have ID's 40-49 reserved for LPUART 1-10 so maybe we go with USE_LPUART instead to be consistent.

@SteveCEvans
Copy link
Member

Impressive amount of work. Have you tested DMA operation? This is code we don’t use and so isn’t tested at the moment, but in future and with more DMA channels this may be resurrected.

@ledvinap
Copy link
Contributor Author

@SteveCEvans : DMA should be unchanged (same enums, with possibly different values). I'll double-check it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants