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

AP_HAL_ESP32: check UART thread ownership #29003

Merged

Conversation

srmainwaring
Copy link
Contributor

@srmainwaring srmainwaring commented Jan 4, 2025

Add checks that UART driver functions are called on the thread the port is initialised on, and handle a change of thread ownership.

Motivation

  • Fix an issue where calling UARTDriver::_begin with a baudrate of zero (for example when changing the thread owning the UART while initialising the AP_MSP_Telem_Backend) causes a core panic due to an integer division by zero exception.
  • The implementation follows the equivalent AP_HAL_ChibiOS UART driver.

Testing

  • Configure, build and flash esp32s3empty.
  • Set SERIAL3_BAUD 115.
  • Set SERIAL3_PROTOCOL 32.
  • Without this change there is a core panic on the MSP thread.
  • With this change a MSP GPS such as the Mateksys M8Q will work (set GPS1_TYPE = 19).

Copy link
Contributor

@tpwrules tpwrules left a comment

Choose a reason for hiding this comment

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

Looks right and was able to test that the crash is fixed though I don't have an MSP GPS.

This is another strange area of the code I'd like to fix, but for now ESP32 should better match ChibiOS.

@tpwrules tpwrules merged commit 44fdd0b into ArduPilot:master Jan 5, 2025
8 checks passed
Copy link
Collaborator

@davidbuzz davidbuzz left a comment

Choose a reason for hiding this comment

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

reviewed ok too

@srmainwaring srmainwaring deleted the prs/pr-esp32-uart-thread-owner branch January 6, 2025 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants