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

Bugfix/issue 1867 Bluetooth notification error fix #1868

Merged
merged 3 commits into from
Oct 23, 2023

Conversation

JulianKast
Copy link
Contributor

@JulianKast JulianKast commented Sep 14, 2023

Fixes #1867

This PR is [ready] for review.

Risk

This PR makes [no] API changes.

Testing Plan

  • I have verified that I have not introduced new warnings in this PR (or explain why below)
  • I have run the unit tests with this PR
  • I have tested this PR against Core and verified behavior (if applicable, if not applicable, explain why below).
  • I have tested Android

Unit Tests

n/a

Core Tests

Test 1

  1. Install SDL app to phone
  2. Connect to tdk via Bluetooth
  3. Observe SDL app connection
  4. Disconnect from tdk by clicking on the BT connection and clicking disconnect on the phone.
    Repeat steps 2-4 up to 30 times
    Observe:
    Bluetooth notification does not appear for too many apps using Bluetooth.

Test 2

  1. Install SDL app to phone
  2. Connect to TDK
  3. Disconnect Bluetooth from TDK, but leave Bluetooth turned on.
  4. Install and run this app https://github.com/shiniwat/consumespp and let it consume all available SPP resources
  5. Connect to TDK
    Observe:
    Bluetooth notification appears for too many apps using Bluetooth

Core version / branch / commit hash / module tested against: Sync 3
HMI name / version / branch / commit hash / module tested against: Sync 3

Summary

Add a check to see if RouterService is in the Foreground before sending the spp notification.

Changelog

Bug Fixes
  • Fixes an issue where the spp notification was being sent to the users in error.

CLA

Base automatically changed from bugfix/issue_1852_update_gradle to develop October 19, 2023 15:00
Copy link
Member

@joeygrover joeygrover left a comment

Choose a reason for hiding this comment

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

Small change requested for thread safety but otherwise solution works well.

Comment on lines 3912 to 3916
// Check first to see if the RouterService is in the Foreground
// This is to prevent the notification appearing in error
if (!this.isForeground) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

This variable should actually be accessed with this lock. It will ensure that the correct state is returned here.

Suggested change
// Check first to see if the RouterService is in the Foreground
// This is to prevent the notification appearing in error
if (!this.isForeground) {
return;
}
synchronized (FOREGROUND_NOTIFICATION_LOCK) {
// Check first to see if the RouterService is in the Foreground
// This is to prevent the notification appearing in error
if (!this.isForeground) {
return;
}
}

@joeygrover joeygrover merged commit 6be63e9 into develop Oct 23, 2023
@joeygrover joeygrover deleted the bugfix/issue_1867_bt_notification_error branch October 23, 2023 20:34
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