search

PX4 / PX4-Autopilot

PX4 Autopilot Software
https://px4.io
BSD 3-Clause "New" or "Revised" License
8.14k stars 13.35k forks source link

DSM overwrites SBUS data #18249

Open ken-voly opened 2 years ago

ken-voly commented 2 years ago

Describe the bug DSM overwrites SBUS data in the global frame buffer, causing the SBUS frame to be unsuccessfully decoded.

Both DSM and SBUS use the same global buffer to store their work between function calls. Each iteration of the IO main loop, sbus_input() is called, followed by dsm_port_input(). Because it takes more than one iteration of the main loop to read an entire SBUS packet, DSM can end up overwriting the SBUS data in the global buffer. This leads to the packet being unsuccessfully decoded. In the worst case, DSM may be able to overwrite with data that results in a valid SBUS packet containing garbage data.

To Reproduce Steps to reproduce the behavior:

  1. Clone this branch, build px4_fmu-5_default, and upload to a Pixhawk 4.
  2. Connect to the IO debug port at 115200 baud rate.
  3. Send an SBUS packet to the flight controller. For example, this Python bytearray - bytearray(b'\x0f\xac\xf0\x9e\xf7\xc0\x07>V\xb0\x02|\xac`\x05\xf8\xc0\x07>\xf0\x81\x0f|\x00\x00')
  4. DSM will overwrite the SBUS packet, leading to a decode error.

Expected behavior DSM should not be able to overwrite the SBUS data and the SBUS packet should be decoded successfully.

When DSM is commented out, it behaves correctly.

Log Files and Screenshots Screenshot from 2021-09-15 13-54-52

dagar commented 2 years ago

Based on other issues I've seen I'm currently wondering if we should stop scanning between all these protocols (by default) and instead simply have the user explicitly configure which to use.

Thoughts?

JacobCrabill commented 2 years ago

That's basically what we've done internally, except at build time, because that makes sense for our use case. For general purpose use, having it be configurable would likely be the best bet. Then you can get the safety of only 1 protocol, but keep the flexibility of having multiple options.

ken-voly commented 2 years ago

Yeah. I think making it configurable using a parameter would be the best approach. A register could be added to the IO to indicate which method the IO should use, if any.

dagar commented 2 years ago

To be clear, we can probably still fix the buffer reuse bug.

ken-voly commented 2 years ago

I agree. There's too much potential for bugs by using a shared global buffer.