betaflight / betaflight

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

Fix index and ability to configure LPUART_RX due to typo #13565

Closed haslinghuis closed 1 month ago

haslinghuis commented 1 month ago

Some history:

Tested on MAMBAG4AIO

resource SERIAL_TX 1 A09
resource SERIAL_TX 2 B03
resource SERIAL_TX 4 C10
resource SERIAL_RX 1 A10
resource SERIAL_RX 2 B04
resource SERIAL_RX 3 B09
resource SERIAL_RX 4 C11
resource LPUART_TX 1 B11
resource LPUART_RX 1 B10

# serial
serial 20 1 115200 57600 0 115200
serial 0 0 115200 57600 0 115200
serial 1 0 115200 57600 0 115200
serial 2 0 115200 57600 0 115200
serial 3 0 115200 57600 0 115200
serial 40 0 115200 57600 0 115200
github-actions[bot] commented 1 month ago

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

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

hydra commented 1 month ago

I'd just like to say that while being accurate, the PR title leaves a lot out. Perhaps, 'Fix ability to configure LPUART_RX due to typo'. The description is a bit lacking too, some information about the original report and the source would have been nice too. When searching through commits, history and using git blame it really helps to have good commit messages and PR titles.

Here's my orginal report that led to this PR, posted in discord. https://discord.com/channels/868013470023548938/924169278926770207/1231700278089089117

image

ledvinap commented 1 month ago

Actually, the code is still wrong. SERIAL_PORT_MAX_INDEX is count of uarts + lpuarts. So it should be

    DEFA( OWNER_LPUART_TX,     PG_SERIAL_PIN_CONFIG, serialPinConfig_t, ioTagTx[SERIAL_PORT_MAX_INDEX - SERIAL_LPUART_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], SERIAL_LPUART_MAX_INDEX ),
#endif

And _MAX_INDEX is totally misleading with zero-based C indexing / when mixing resource id with C arrays

haslinghuis commented 1 month ago

@ledvinap @hydra

hydra commented 1 month ago

@ledvinap @hydra

I gave this a test on the SPRacingH7NEO target I'm working on and was able to remap UART 1 RX/TX pins and assign/de-assign/swap LPUART 1 RX and TX pins ok.

suggest merging this as a quick-fix until someone has more time to cleanup the indexing further with regards to the other changes in #13569

hydra commented 1 month ago

I would just like to mention that it sure would be nice to have some unit tests to check for expected behavior.

ledvinap commented 1 month ago

@hydra : That would mean unittesting different target #defines .. not impossible, bue there is very little scaffolding in place now