Skip to content

Merge develop into main#86

Merged
matt2005 merged 4 commits intomainfrom
develop
Mar 15, 2026
Merged

Merge develop into main#86
matt2005 merged 4 commits intomainfrom
develop

Conversation

@matt2005
Copy link

@matt2005 matt2005 commented Mar 15, 2026

Summary

Merge latest changes from develop into main.

Notes

  • Source branch: develop
  • Target branch: main
  • Requested promotion PR from origin/develop to origin/main.

matt2005 added 2 commits March 13, 2026 23:58
@matt2005
fix: USB AOAP recovery, TLS bridge guard, SSL decrypt drain, diagnost…
ccfcb03 
…ics (#84)

* Fix SSL decrypt handling and migrate cert install to /etc/aasdk

- handle SSL WANT_READ/WANT_WRITE as partial frame in Cryptor decrypt\n- add richer SSL diagnostics in SSLWrapper/Cryptor\n- move cert/key install path from /etc/openauto to /etc/aasdk\n- add debian postinst migration and permission/ownership fixups

* fix: USB AOAP recovery, TLS bridge, SSL decrypt drain, diagnostics

- AOAPDevice: detect LIBUSB_ERROR_BUSY on claimInterface and attempt
  releaseInterface + retry before throwing to handle stale ownership
  after abrupt transport teardown

- MessageInStream: conditionally inject ENCAPSULATED_SSL prefix (MessageId 3)
  only when the incoming payload looks like a TLS record (content-type
  0x14-0x17, version byte 0x03); plain payloads such as version responses
  are no longer misclassified; promote frame/payload logs to info level

- Cryptor: simplify SSL decrypt drain loop to pure while(true) with
  fixed 2048-byte read chunks; remove overhead/expected-bytes heuristics
  that caused premature loop exit; demote WANT_READ/WANT_WRITE to debug

- ControlServiceChannel: add info-level logs for sendVersionRequest
  (logs major/minor) and sendHandshake (logs payload size); promote
  incoming MessageId log to info for runtime visibility

- USBTransport: add diagnostic logs for doSend submission, sendComplete,
  sendError, enqueueReceive, receiveComplete and receiveError including
  endpoint address, byte count and error code/native code
@matt2005
Add runtime-toggleable cryptor/message tracing and docs (#85)
f8a8651
@matt2005 matt2005 self-assigned this Mar 15, 2026
@matt2005 matt2005 requested a review from Copilot March 15, 2026 00:30
Copilot AI reviewed Mar 15, 2026
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR promotes the current develop branch state into main, bringing in USB transport resiliency/logging, expanded runtime tracing for encrypted message flow, and packaging changes around TLS certificate/key installation.

Changes:

  • Add USB interface claim retry logic and additional USB transport send/receive logging.
  • Add runtime trace toggles and richer diagnostics around message framing and SSL/Cryptor read/decrypt paths.
  • Move installed headunit TLS cert/key path to /etc/aasdk with Debian postinst migration and updated docs.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
src/USB/AOAPDevice.cpp Adds a busy-interface recovery path when claiming the AOAP interface.
src/Transport/USBTransport.cpp Adds detailed endpoint-level logging around send/receive operations.
src/Transport/SSLWrapper.cpp Adds stringification + logging for SSL error diagnostics in getError().
src/Messenger/MessageInStream.cpp Adds runtime-configurable tracing, extra frame/payload logs, and a cryptor-inactive encrypted pass-through path.
src/Messenger/Cryptor.cpp Adds runtime tracing toggles and revises decrypt/read error handling + cert/key search paths.
src/Channel/Control/ControlServiceChannel.cpp Adds extra info logs for version request/handshake and message id processing.
debian/postinst Migrates cert/key from legacy location and applies ownership/permissions post-install.
QUICK_REFERENCE.md Documents new runtime tracing environment variables and expected log tags.
CMakeLists.txt Changes install destination/permissions for headunit.crt and headunit.key to /etc/aasdk.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@matt2005
Copy link
Author

matt2005 commented Mar 15, 2026

Copilot review triage for this PR:

  1. Security permissions/grouping (debian/postinst, CMakeLists.txt)
  • pi group fallback and 640 on headunit.key were intentional for Raspberry Pi appliance deployments where the runtime process is non-root and must read the key.
  • Agreed this is broader than ideal on multi-user systems.
  • Follow-up plan: introduce a dedicated service group (e.g. aasdk), default key perms to 0600, and only relax when explicitly configured for non-root runtime compatibility.
  1. High-frequency info logs (src/Transport/USBTransport.cpp, src/Messenger/MessageInStream.cpp)
  • Agree with Copilot: current info volume is too high for normal runtime and can impact throughput/noise.
  • Follow-up plan: demote per-frame/per-transfer logs to debug and keep detailed logs behind runtime trace toggles.
  1. shouldTraceMessage() static counter race (src/Messenger/MessageInStream.cpp)
  • Agree with Copilot: function-local static counter should be atomic or instance-scoped to avoid cross-instance race potential.
  • Follow-up plan: switch to atomic counter, matching approach used in Cryptor tracing.
  1. SSLWrapper::getError() severity and error-queue draining (src/Transport/SSLWrapper.cpp)
  • Agree with Copilot: logging WANT_READ/WANT_WRITE as error is misleading, and draining ERR_get_error() should be restricted to fatal paths.
  • Follow-up plan: lower expected-flow logging level and only drain/emit OpenSSL queue when escalating hard failures.
  1. Magic number MessageId(3) (src/Messenger/MessageInStream.cpp)
  • Agree with Copilot: replace with named control enum constant to make intent explicit and robust to enum changes.
  1. Test coverage for cryptor_->isActive() encrypted pass-through branch (src/Messenger/MessageInStream.cpp)
  • Agree with Copilot: add/adjust tests for active/inactive encrypted paths and TLS-like pass-through behavior.

Thanks for the review; I’ll keep this thread as the explicit rationale and track the follow-ups accordingly.

@matt2005 matt2005 mentioned this pull request Mar 15, 2026
Merged
@matt2005
Copy link
Author

matt2005 commented Mar 15, 2026

Follow-up after merging #87 into develop:

  • Replied on each Copilot review comment that was addressed by Address Copilot review follow-ups for runtime trace and SSL handling #87.
  • The merged develop branch now includes the actioned fixes for:
    • hot-path log level reductions
    • dedicated-group / key permission hardening
    • atomic trace counter
    • SSLWrapper::getError() fatal-vs-nonfatal handling
    • OpenSSL error queue draining only on fatal paths
    • replacement of the hard-coded MessageId(3) with MESSAGE_ENCAPSULATED_SSL
  • The remaining unaddressed Copilot point is the request for additional unit-test coverage around the cryptor_->isActive() encrypted pass-through branch.

I could not programmatically toggle GitHub threads to resolved with the tools available here, but the addressed threads now have explicit reply markers and are ready to be resolved in the PR UI.

@matt2005
Copy link
Author

matt2005 commented Mar 15, 2026

add tests and merged to develop

@matt2005 matt2005 merged commit a264826 into main Mar 15, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

@matt2005 matt2005

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@matt2005