betaflight / betaflight

Open Source Flight Controller Firmware
GNU General Public License v3.0
8.06k stars 2.87k forks source link

Configuration of LPUART is broken. #13567

Closed hydra closed 1 month ago

hydra commented 1 month ago

Describe the bug

It's not possible to configure LPUART pins using the cli resource command.

There are two issues.

  1. when configuring the LPUART TX pin, the SERIAL_TX pins gets configured as well.
  2. an error is given when attempting to configure the RX pin.

To Reproduce

  1. image

  2. image

Expected behavior

working LPUART pin configuration. e.g. resource LPUART_RX 1 B07.

Support ID

N/A

Flight controller

SPRacingH7NEO

Other components

No response

How are the different components wired up (including port information)

No response

Add any other context about the problem that you think might be relevant here

Notes from discord:

I'm wondering if the first issue is due to the calculation of the amount of serial ports and/or having wrong/same index values for LPUART as SERIAL pins.
[9:05 AM]hydra:
#define SOFTSERIAL_PORT_IDENTIFIER_TO_INDEX(x) ((x) - SERIAL_PORT_SOFTSERIAL1)

#if defined(USE_LPUART1)

#define SERIAL_PORT_IDENTIFIER_TO_INDEX(x) \
    (((x) <= SERIAL_PORT_USART_MAX) ? ((x) - SERIAL_PORT_USART1) : ((x) - SERIAL_PORT_LPUART1) + LPUARTDEV_1)
#define SERIAL_PORT_IDENTIFIER_TO_UARTDEV(x) \
    (((x) <= SERIAL_PORT_USART_MAX) ? ((x) - SERIAL_PORT_USART1 + UARTDEV_1) : ((x) - SERIAL_PORT_LPUART1) + LPUARTDEV_1)

#else

#define SERIAL_PORT_IDENTIFIER_TO_INDEX(x) ((x) - SERIAL_PORT_USART1)
#define SERIAL_PORT_IDENTIFIER_TO_UARTDEV(x) ((x) - SERIAL_PORT_USART1 + UARTDEV_1)

#endif

let's see...
[9:07 AM]hydra: likely, the cli command for resource is indexing into the uarts, not the lpuarts.
[9:08 AM]hydra:
    DEFA( OWNER_SERIAL_TX,     PG_SERIAL_PIN_CONFIG, serialPinConfig_t, ioTagTx[0], SERIAL_PORT_MAX_INDEX - SERIAL_LPUART_MAX_INDEX ),
    DEFA( OWNER_SERIAL_RX,     PG_SERIAL_PIN_CONFIG, serialPinConfig_t, ioTagRx[0], SERIAL_PORT_MAX_INDEX - SERIAL_LPUART_MAX_INDEX ),
    DEFA( OWNER_LPUART_TX,     PG_SERIAL_PIN_CONFIG, serialPinConfig_t, ioTagTx[SERIAL_PORT_MAX_INDEX], SERIAL_LPUART_MAX_INDEX ),
    DEFA( OWNER_LPUART_RX,     PG_SERIAL_PIN_CONFIG, serialPinConfig_t, ioTagRx[SERIAL_PORT_MAX_INDEX], SERIAL_LPUART_MAX_INDEX ),
[9:09 AM]hydra: or perhaps SERIAL_PORT_MAX_INDEX is not right.
[9:10 AM]hydra:
#define SERIAL_PORT_MAX_INDEX 11
#define SERIAL_LPUART_MAX_INDEX 1
[9:12 AM]hydra: or perhaps the space allocated to the ioTagTx/Rx struct members is insufficient...
typedef struct serialPinConfig_s {
    ioTag_t ioTagTx[SERIAL_PORT_MAX_INDEX];
    ioTag_t ioTagRx[SERIAL_PORT_MAX_INDEX];
    ioTag_t ioTagInverter[SERIAL_PORT_MAX_INDEX];
} serialPinConfig_t;
[9:13 AM]hydra: feels to me, like storing LPUARTS in their own structure, is perhaps the right way to go about it, instead of tacking them on the end of the normal serial ports.
hydra commented 1 month ago

Note, both issues are still present on master with https://github.com/betaflight/betaflight/pull/13565 applied.

hydra commented 1 month ago

there's also this:

    "SOFTSERIAL_TX",
    "SOFTSERIAL_RX",
    [OWNER_LPUART_TX] = "LPUART_TX",
    [OWNER_LPUART_RX] = "LPUART_RX",
};

which doesn't match any of the previous lines of code. from 90841282b03339c7361db306d280f4bdd96de1e4 / #13306

hydra commented 1 month ago

I note that changing this:

    "SOFTSERIAL_TX",
    "SOFTSERIAL_RX",
    [OWNER_LPUART_TX] = "LPUART_TX",
    [OWNER_LPUART_RX] = "LPUART_RX",
};

to this:

    "SOFTSERIAL_TX",
    "SOFTSERIAL_RX",
    "LPUART_TX",
    "LPUART_RX",
};

fixes the second issue.

# resource LPUART_RX 1 B07

Resource is set to B07

@ledvinap / @haslinghuis your suggestion here https://github.com/betaflight/betaflight/pull/13306#discussion_r1502901863 didn't do what you expected.

hydra commented 1 month ago

With #13565 and #13568 applied the assignment of serial/lpuarts is still coupled, as follows:

# resource LPUART_RX 1 B07

Resource is set to B07

# resource
resource SERIAL_TX 1 A09
resource SERIAL_RX 1 A10
resource LPUART_TX 1 A10
resource LPUART_RX 1 B07
# ^^^ note duplication of assignment on A10.

# resource LPUART_TX 1 NONE
Resource is freed

# resource
resource SERIAL_TX 1 A09
resource SERIAL_TX 2 D05
resource SERIAL_TX 3 D08
resource SERIAL_TX 4 B09
resource SERIAL_TX 8 E01
# ... <-- NO SERIAL_RX 1!
resource SERIAL_RX 2 D06
resource LPUART_RX 1 B07

# resource SERIAL_RX 1 A10

NOTE: A10 already assigned to LPUART_TX 1.

Resource is set to A10

# resource
resource SERIAL_TX 1 A09
resource SERIAL_RX 1 A10
resource LPUART_TX 1 A10
resource LPUART_RX 1 B07
# ^^^ note both SERIAL_RX and LPUART_TX were configured!
ledvinap commented 1 month ago

there's also this:

    "SOFTSERIAL_TX",
    "SOFTSERIAL_RX",
    [OWNER_LPUART_TX] = "LPUART_TX",
    [OWNER_LPUART_RX] = "LPUART_RX",
};

which doesn't match any of the previous lines of code. from 9084128 / #13306

This part is IMO correct. Item order match and gcc is quite strict to prevent mismatch.

hydra commented 1 month ago

This part is IMO correct. Item order match and gcc is quite strict to prevent mismatch.

Can you explain why making the single change in https://github.com/betaflight/betaflight/issues/13567#issuecomment-2068693404 fixes the 'Invalid resource name LPUART_RX' issue?

ledvinap commented 1 month ago

@hydra : I don't know. But at least in master, [OWNER_LPUART_TX] = "LPUART_TX", ends up exactly in the same place as "LPUART_TX" Remove it if you want, all entries shall be prefixed anyway

hydra commented 1 month ago

@hydra : I don't know. But at least in master, [OWNER_LPUART_TX] = "LPUART_TX", ends up exactly in the same place as "LPUART_TX" Remove it if you want, all entries shall be prefixed anyway

compiler bug perhaps?

ledvinap commented 1 month ago

@hydra : I started deep cut into serial mess, so I'll look into this later.