Skip to content

Fix thread safety, races, and correctness issues across drivers#639

Draft
HTRamsey wants to merge 1 commit intomik3y:masterfrom
HTRamsey:fix/thread-safety-and-correctness
Draft

Fix thread safety, races, and correctness issues across drivers#639
HTRamsey wants to merge 1 commit intomik3y:masterfrom
HTRamsey:fix/thread-safety-and-correctness

Conversation

@HTRamsey
Copy link
Copy Markdown
Contributor

Summary

Targeted fixes for thread safety, race conditions, error handling, and correctness issues across all drivers and utilities. No new APIs, no architectural changes.

Thread safety (volatile)

  • Drivers: DTR/RTS flags, mReadRequest, DEBUG, breakConfig, mStatus, mDeviceType, latency timer fields accessed across threads without visibility guarantees
  • SerialInputOutputManager: DEBUG, mReadTimeout, mWriteTimeout, mThreadPriority, mReadBuffer, mWriteBuffer
  • XonXoffFilter: xon field

Correctness fixes

  • FtdiSerialDriver: Assign endpoints by direction + bulk type instead of hardcoded index — prevents silent read/write swap if device enumerates endpoints in unexpected order
  • Ch34xSerialDriver: getStatus() validates exact return length, not just >= 0 — prevents returning stale/zeroed data on partial transfer
  • Cp21xxSerialDriver: Port number validation before mIsRestrictedPort assignment
  • ProlificSerialDriver: interrupt() before join() on status thread — prevents indefinite hang if thread is blocked on USB transfer
  • ProbeTable: Boolean.TRUE.equals(o) instead of (boolean)o — prevents NPE if probe method returns null
  • ChromeCcdSerialDriver: Use actual interface count instead of hardcoded 3
  • CdcAcmSerialDriver: Guard against double-releasing same interface; null check on control endpoint count
  • CommonUsbSerialPort: Null check on mReadRequest in readAsync() before queueing; getSerial() throws if port not open

Error handling

  • Replace silent catch(Exception ignored){} with Log.w in all driver close paths — makes USB debugging possible
  • testConnection error message includes return code (rc=)

Resource management

  • UsbRequest.close() on Android O+ (was only cancelling, not closing)

Other

  • static final on TAG and constant fields that were incorrectly instance-scoped
  • LinkedListArrayList for read queue (better random-access performance)
  • Fix "occured" typo in log message

Test plan

  • Verify existing instrumented tests pass on connected devices
  • Smoke test open/close/read/write on FTDI, Prolific, CP21xx, CH34x devices
  • Verify Log.w messages appear in logcat during abnormal disconnect

- volatile on dtr/rts in Ch34x, Cp21xx, CdcAcm, Ftdi drivers
- volatile on mReadRequest, mStartuplatch in CommonUsbSerialPort/SIOM
- volatile on mStopReadStatusThread, mReadStatusException in Prolific
- fix readAsync TOCTOU: local capture + try-catch on re-queue during close
- fix Prolific mReadStatusException double-read race
- fix Prolific sign-extended version byte parsing
- fix CdcAcm double releaseInterface in single-interface mode
- add CdcAcm endpoint count guard before getEndpoint(0)
- fix ChromeCcd hard-coded port count, use getInterfaceCount()
- fix SIOM stop() to handle STARTING state
- fix SIOM start() InterruptedException to clean up threads
- fix baud rate typos in Ftdi and Prolific (to -> too)
- make TAG static final in CdcAcm, GsmModem, Prolific, ChromeCcd
- make Ch34x DEFAULT_BAUD_RATE static final
- make GsmModem initGsmModem() void (unused return)
- add driver class name to UsbSerialProber exception
@kai-morich
Copy link
Copy Markdown
Collaborator

looks AI generated. Any real issues you want to fix?

@HTRamsey
Copy link
Copy Markdown
Contributor Author

Believe things like usage of RESET_PURGE_TX & RESET_HXN_TX_PIPE are backwards right? But yeah all the extra debugging stuff was AI added to help me resolve a couple issues, can remove all the debugging stuff if you want

@kai-morich
Copy link
Copy Markdown
Collaborator

yes, PURGE constants look reversed, which also confused me when I took over the library, but DeviceTest.purgeHwBuffers() successfully tests the functionality so I kept as is.
All else looks like cosmetics, or did I miss something important?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants