-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Add resource for LPUART #13306
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
Add resource for LPUART #13306
Conversation
Do you want to test this code? You can flash it directly from Betaflight Configurator:
WARNING: It may be unstable. Use only for testing! |
27ebbd2
to
e39279f
Compare
I think it would definitely be good to categorise LPUART separately in RESOURCES. I would approve this PR if it is technically the right way to do it. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within a week. |
Are we sure no more need for more LPUART ports, maybe give room for more than 1 new port ie.
I think an update of documentation are important. |
Our current targets do not have a second LPUART port. If needed it can be extended. |
yes, of course, but then also documentation etc etc, but no big issue .-) |
src/main/cli/cli.c
Outdated
DEFA( OWNER_SERIAL_TX, PG_SERIAL_PIN_CONFIG, serialPinConfig_t, ioTagTx[0], SERIAL_PORT_MAX_INDEX ), | ||
DEFA( OWNER_SERIAL_RX, PG_SERIAL_PIN_CONFIG, serialPinConfig_t, ioTagRx[0], SERIAL_PORT_MAX_INDEX ), | ||
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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it better to define serialPinConfig_t { ioTagTx[SERIAL_PORT_MAX_INDEX + SERIAL_LPUART_MAX_INDEX];
... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subtracting SERIAL_LPUART_MAX_INDEX (1) from SERIAL_PORT_MAX_INDEX (11) to have 10 UARTs and 1 LPUART.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you must change both, or it will break.
But a lot of hardwired problems will surface if you change it, so keeping change minimal may be better
#endif | ||
#ifdef USE_LPUART1 | ||
DEFA( OWNER_LPUART_TX, PG_SERIAL_PIN_CONFIG, serialPinConfig_t, ioTagTx[SERIAL_PORT_MAX_INDEX], SERIAL_LPUART_MAX_INDEX ), | ||
DEFA( OWNER_LPUART_TX, PG_SERIAL_PIN_CONFIG, serialPinConfig_t, ioTagRx[SERIAL_PORT_MAX_INDEX], SERIAL_LPUART_MAX_INDEX ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no inverter for LPUART?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes was not sure - reverted
src/main/drivers/resource.c
Outdated
@@ -113,4 +113,6 @@ const char * const ownerNames[OWNER_TOTAL_COUNT] = { | |||
"RX_SPI_EXPRESSLRS_BUSY", | |||
"SOFTSERIAL_TX", | |||
"SOFTSERIAL_RX", | |||
"LPUART_TX", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(maybe refactor all lines into designated initializers:
"LPUART_TX", | |
[OWNER_LPUART_TX] = "LPUART_TX", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - but further refactoring should be done in 4.6 as we are in RC phase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ledvinap please approve if you are okay with the changes. We can refactor remaining items after release.
74f2341
to
bf082ff
Compare
if this is am improvement not a bug fix it shouldn't be in 4.5. I use the lpuarts on my race builds on the airbot Fenix aios for the my elrs so I can use the provided plugs for my radio gear ( means I can have spare rx's ready to go ) it currently performs perfectly fine so this close to release would rather leave as is. I'd be a lot more comfortable with @DieHertz taking a look as he was behind the LPuart implementation, but he's still moving around a lot I thinks |
This PR fixes inconsistency between softserial and lpuart. Agree further improvements could be made in next version. Don't mind if this PR skips release as scope is limited. Rebased on master (for testing). |
bf082ff
to
086788a
Compare
@haslinghuis |
@ctzsnooze You would need a board with LPUART |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve on the basis that the only way to get wide testing is to get this into master. Seems that only G4 boards have LPUART. I don't have any of those.
LPUART is not an usual U[S]ART port: https://www.st.com/resource/en/application_note/an4635-minimization-of-power-consumption-using-lpuart-for-stm32-microcontrollers-stmicroelectronics.pdf
Never had to use softserial but in 4.4 it used UART11 and UART12 before as now we need UART 11 to define LPUART1.
In config [repo] we use LPUART pins to define a serial port with LPUART1.
Currently in CLI we need to use SERIAL 11 to configure LPUART port which is [treated] different from a normal U[S]ART.
This PR follows configuration in config.h and allows CLI to use
resource LPUART
instead but uses same UART constructs.This PR prevents configuring UART 11 to be used as LPUART while there is no LPUART resource available.
In documentation we have
There is an issue when assigning UART 11 on KAKUTEH7MINI we get - as there is no LPUART here: