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

New platform independent UART interface #21723

Merged
merged 33 commits into from
Mar 22, 2024
Merged

New platform independent UART interface #21723

merged 33 commits into from
Mar 22, 2024

Conversation

katzfey
Copy link
Contributor

@katzfey katzfey commented Jun 15, 2023

Solved Problem

There are multiple copies of UART configuration and use code that are not platform independent. The Qurt platform, in particular, is quite different in terms of UART usage. This is a proposal for a new platform independent UART API that hides all of the implementation details in platform specific files and provides a generic API for all platforms. The GPS, RC, and UART based ESC drivers will important beneficiaries of this new API.

@katzfey katzfey requested a review from dagar June 15, 2023 18:56
src/drivers/gps/gps.cpp Outdated Show resolved Hide resolved
@PetervdPerk-NXP
Copy link
Member

Its seems that on all non-Qurt targets the flash usage roughly increases by 2KB
Bloaty output px4-fmuv2
+0.2% +2.10Ki TOTAL

@dagar you might want to update bloaty on the CI since on many boards it gets an integer overflow.

PX4 pretty much assumes it's using POSIX based UART and uses termios to change settings.
Wouldn't it be better to add a shim to platforms/qurt to implement the POSIX uart + termios for Qurt?

@dagar
Copy link
Member

dagar commented Jun 19, 2023

@dagar you might want to update bloaty on the CI since on many boards it gets an integer overflow.

Thanks @PetervdPerk-NXP, I'm going to update the containers to 22.04 + latest tools soon.

PX4 pretty much assumes it's using POSIX based UART and uses termios to change settings.
Wouldn't it be better to add a shim to platforms/qurt to implement the POSIX uart + termios for Qurt?

That was one idea I proposed initially, but after some discussion with @katzfey and my own pain with subtle differences in POSIX between NuttX/Linux/MacOS I actually thought this could be a nice opportunity to simplify/optimize the vast majority of our serial usage with an API that's easier to use, harder to misuse.

  • vast majority of serial usage is "raw" mode with that wall of termios + cfsetispeed configuration duplicated across most drivers
  • we don't actually use most of POSIX terminal I/O past initial config where we disable everything to get to raw mode
  • there are subtle differences between NuttX/Linux/MacOS with O_NONBLOCK, cfsetspeed, VTIME, VMIN, etc
  • opportunity to optimize the typical usage pattern where we want at least a certain number of bytes with minimal latency (NuttX doesn't actually implement VMIN/VTIME)
  • simpler setup and cleanup (RAII, no direct file descriptors, etc)
  • possibly a bit easier for future ports to Zephyr or even crazier ideas like a native windows build (not a major consideration)

The immediate goal in this PR was to get to an acceptable MVP to unblock QuRT progress and leave room to continue some of the other ideas in the future.

Happy to discuss further before we fully commit to anything.

src/drivers/gps/gps.cpp Outdated Show resolved Hide resolved
@SalimTerryLi
Copy link
Contributor

I just come up with some other functionality requirements related to UART:

  • Single-Wire mode
  • Inverted mode
  • WorkQueue integration? (mostly about px4::serial_port_to_wq() so that per-bus thread/task can be achieved)
  • Possibility to do non-blocking IO with FIONREAD & FIONSPACE support instead of fire-until-timeout? (not sure if any driver requires that)

Just gave up reading the mavlink module for its size XD Not sure what requirement is needed for those complex, software throttling enabled modules.

The temporary increased code size will be reduced once drivers switching to new API for sure... Just noticed so many redundant occurrences...

Copy link
Contributor

@dirksavage88 dirksavage88 left a comment

Choose a reason for hiding this comment

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

@dagar is it necessary to split the driver into the common and nuttx (and POSIX?) layer or can it all be lumped at the px4 common layer? Serial.cpp needs to be linked with SerialImp.cpp to resolve linking errors it seems.

platforms/common/CMakeLists.txt Outdated Show resolved Hide resolved
@katzfey
Copy link
Contributor Author

katzfey commented Mar 6, 2024

@SalimTerryLi The idea with this PR is to get started with GPS only. So the new API has enough functionality for only what GPS needs and GPS is updated to use the new API. This is actually a pretty standard configuration so it should be usable by most other drivers. But as more drivers get updated to use the new API any missing additional functionality or settings that they need will need to be added to the Serial driver at that time. This will especially be true for the RC drivers as they have a bunch of oddball requirements.

@dagar dagar changed the title WIP: New platform independent UART interface New platform independent UART interface Mar 6, 2024

int ret = poll(fds, sizeof(fds) / sizeof(fds[0]), remaining_time);

if (ret > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Once even a single byte is available (but < character_count) isn't this just going to spin constantly?
Was there a problem with the sleep that was here originally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't mean to change anything. Let me take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put them back in

@dagar dagar merged commit 69028f3 into main Mar 22, 2024
92 checks passed
@dagar dagar deleted the wip_new_uart_api branch March 22, 2024 01:00
@PetervdPerk-NXP
Copy link
Member

PetervdPerk-NXP commented Mar 22, 2024

Oh this went in?
I'm still missing the routines for TX/RX swap, TX/RX invert and Single-wire mode. Any roadmap to get this in? Kinda sucks to have 2 living UART subsystems.

@katzfey
Copy link
Contributor Author

katzfey commented Mar 22, 2024

@PetervdPerk-NXP Yes, those will be coming in as well. The idea was to add enough functionality for the drivers being ported over. Initially this is just GPS where those other features are not used. But today I am working on PR 22917 which requires flush, Tx / Rx swap, and single wire. So those will be added in that PR. Next on the list is UART distance sensor and RC Library which should bring in all the rest of the needed UART functions.

Peize-Liu pushed a commit to Peize-Liu/PX4-Autopilot that referenced this pull request Mar 24, 2024
@julianoes
Copy link
Contributor

This unfortunately breaks Septentrino GPS with the warnings:

ERROR [SerialImpl] readAtLeast: Buffer not big enough to hold desired amount of read data

@julianoes
Copy link
Contributor

Fix in #22936.

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

Successfully merging this pull request may close these issues.

None yet

7 participants