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

feat: Bose Frames Rondo support #134

Open
wants to merge 54 commits into
base: main
Choose a base branch
from

Conversation

NikMel
Copy link
Contributor

@NikMel NikMel commented Jan 30, 2025

This pull request adds support for Bose Frames Rondo.
A few notes:

  • We changed a few things in the project files (e.g., "entitlements") to make the project build locally without a paid dev license
  • This is the first time we have developed anything for iOS (and in Swift, naturally), and also for Soundscape so any advise on how to improve the PR is welcome
  • Bose Frames report "accuracy" with each direction sample, but documentation on what it means is vague, so there's a fairly arbitrary threshold that sends them into calibration mode. This threshold may need adjusting (the constant accuracy_calibration_required_threshold)

NikMel added 30 commits March 6, 2024 08:05
…mplementing UserHeadingProvider, CalibratableDevice and added connection menu to DeviceViewController
…writing sensor config,subscribing to sensor data and decoding. Also flow to register BLE device and a manager to control the device (CalibratableDevice, UserHeadingProvider). Time to test
…but Testflight requires a paid account... Also added a notification about calibration cancelled when disconnecting (if device was currently calibrating)
…ix DeviceViewController and the views in order...
…oved state management for Bose in the DeviceViewController
…EDevice. Notifies delegate instead of requireing subscription to change in the variable. Updated BoseMotionManager and DeviceViewController accordingly. Code cleanup
…mplementing UserHeadingProvider, CalibratableDevice and added connection menu to DeviceViewController
…writing sensor config,subscribing to sensor data and decoding. Also flow to register BLE device and a manager to control the device (CalibratableDevice, UserHeadingProvider). Time to test
NikMel and others added 23 commits April 17, 2024 10:12
…ix DeviceViewController and the views in order...
…oved state management for Bose in the DeviceViewController
…EDevice. Notifies delegate instead of requireing subscription to change in the variable. Updated BoseMotionManager and DeviceViewController accordingly. Code cleanup
#Conflicts:
#	apps/ios/GuideDogs.xcodeproj/project.pbxproj
#	apps/ios/GuideDogs/Assets/Localization/en-GB.lproj/Localizable.strings
#	apps/ios/GuideDogs/Assets/Localization/sv-SE.lproj/Localizable.strings
@RDMurray
Copy link
Contributor

Thanks for doing all this work! Sorry if it takes us a while to get round to merging it.

We can't merge without first reverting the unrelated changes such as the app name and entitlements. I see this is from your main branch which you might want to keep as is. I would suggest creating another branch for the PR to keep it separate.

You could either create a branch off soundscape-community/soundscape main and then cherry-pick the relevant commits, or branch from your main and revert the unwanted changes.

I think the second option would be easier.

@RDMurray
Copy link
Contributor

I did a quick skim of the diff. I'm not going to comment on the added code; you have a lot more expertise in swift than me. There's only a couple of tiny things I noticed:

  • The new files are in the root of the project, rather than under code/. I haven't opened it in xcode yet to see if they are in groups there, but this is probably xcodes fault because groups don't have to match the directory.
  • There is one unrelated change in the en_GB strings.
  • there are a few minor changes to interface builder .storyboard files, I can't tell if these are intensional. If they improve the look of the app it's fine to leave them in, I can't judge because I'm completely blind.

It doesn't modify any other functionality of the app, so apart from these minor issues I don't see any problem with merging it.

1 similar comment
@RDMurray
Copy link
Contributor

I did a quick skim of the diff. I'm not going to comment on the added code; you have a lot more expertise in swift than me. There's only a couple of tiny things I noticed:

  • The new files are in the root of the project, rather than under code/. I haven't opened it in xcode yet to see if they are in groups there, but this is probably xcodes fault because groups don't have to match the directory.
  • There is one unrelated change in the en_GB strings.
  • there are a few minor changes to interface builder .storyboard files, I can't tell if these are intensional. If they improve the look of the app it's fine to leave them in, I can't judge because I'm completely blind.

It doesn't modify any other functionality of the app, so apart from these minor issues I don't see any problem with merging it.

@RDMurray
Copy link
Contributor

sorry about the multiple comments, there was a github error every time I pressed submit.

Copy link
Member

@steinbro steinbro left a comment

Choose a reason for hiding this comment

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

Whoa, thanks for all of this! Without a pair of Bose Frames, I can't validate for myself, so I can only provide a few stylistic comments/places where localized strings belong. I'm in agreement with @RDMurray that we can merge after a little cleanup.

Comment on lines +112 to +115
callouts.append(StringCallout(.arHeadset, "Bose frames are connected (TODO: Add localized string for this!)"))
} else {
callouts.append(StringCallout(.arHeadset, "Bose frames are not connected (TODO: Add localized string for this!")) //"devices.callouts.check_audio.airpods.disconnected")))
}
Copy link
Member

Choose a reason for hiding this comment

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

We should address these TODOs before merging, just a few calls to GDLocalizedString.

Comment on lines +244 to +274
// Now, await notification of reading the config value and set new state from there


/*
for service in services {
if (service.uuid == BOSE_FRAMES_SERVICE_CONSTANTS.CBUUID_HEADTRACKING_SERVICE) {
for c in service.characteristics! {
switch c.uuid {
case BOSE_FRAMES_SERVICE_CONSTANTS.CBUUID_HEADTRACKING_CONFIG_CHARACTERISTIC:
GDLogBLEInfo("Found Bose Config Characteristic")
self.boseCharacteristicSensorConfig = c
self.peripheral.readValue(for: c)

case BOSE_FRAMES_SERVICE_CONSTANTS.CBUUID_HEADTRACKING_DATA_CHARACTERISTIC:
GDLogBLEInfo("Found Bose Data Characteristic")
self.boseCharacteristicSensorData = c
self.peripheral.setNotifyValue(true, for: c)
continue

default:
() // Noop
}
}
}
}

guard boseCharacteristicSensorData != nil && boseCharacteristicSensorConfig != nil else {
GDLogBLEError("Bose: onConnectionComplete ERROR. Did not find both Config and Data Characteristic. This will not work...")
return
}
*/
Copy link
Member

Choose a reason for hiding this comment

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

Can remove this commented-out block.

Comment on lines +37 to +71
/*
func ping(timeoutInterval: TimeInterval, completion: @escaping ReachabilityCompletion) {
GDLogHeadphoneMotionInfo("[PING] BoseMotionReachability.ping started")

guard isActive == false else {
completion(false)
return
}

isActive = true
self.completionHandler = completion

let bleManager = AppContext.shared.bleManager

bleManager.startScan(for: BoseFramesBLEDevice.self, delegate: boseStateDelegate!)

// Start timeout on main thread
DispatchQueue.main.async { [weak self] in
guard let `self` = self else {
return
}

GDLogHeadphoneMotionInfo("[PING] Awaiting Bose BLE connection...")
self.timer = Timer.scheduledTimer(withTimeInterval: timeoutInterval, repeats: false) { [weak self] _ in
guard let `self` = self else {
return
}

GDLogHeadphoneMotionInfo("[PING] Scheduled timer for Bose connection did fire")
// Timer fired before bose connected `headphoneMotionManagerDidConnect`
self.cleanup(isReachable: false)
}
}
}
*/
Copy link
Member

Choose a reason for hiding this comment

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

Can remove this commented-out block.

Comment on lines +97 to +162
/*
fileprivate class BoseDeviceStateDelegate: BLEManagerScanDelegate {
var boseDevice: BoseFramesBLEDevice?
var pingHandler: (Bool) -> Void
init(livePingHandler: @escaping (Bool) -> Void) {
pingHandler = livePingHandler
}

deinit {
// Stop and reset motion manager
boseDevice?.stopHeadTracking()
boseDevice = nil
}

func onDeviceStateChanged(_ device: BLEDevice) {
if let boseDev = device as? BoseFramesBLEDevice {
if(boseDev.state == .ready) {
GDLogHeadphoneMotionInfo("[PING] Bose Frames are ready, starting to sample the sensor")
boseDevice = boseDev
AppContext.shared.bleManager.stopScan()
boseDev.headingUpdateDelegate = self
boseDev.startHeadTracking()
} else {
GDLogHeadphoneMotionInfo("[PING] Bose Frames state changed, but still not ready (\(device.state))")
}
}
}

func onDeviceNameChanged(_ device: BLEDevice, _ name: String) {

}

func onDevicesChanged(_ discovered: [BLEDevice]) {
if boseDevice == nil {
for i in 0..<discovered.count {
if(discovered[i] is BoseFramesBLEDevice) {
boseDevice = (discovered[i] as! BoseFramesBLEDevice)
boseDevice?.stateDidChangeDelegate = self
return
}
}
}
}
}
extension BoseDeviceStateDelegate: BoseBLEStateChangeDelegate {
func onBoseDeviceStateChange(oldState: BLEDeviceState, newState: BLEDeviceState) {
<#code#>
}

func onBoseDeviceReady() {
GDLogHeadphoneMotionInfo("[PING] Bose Frames are ready (but doing nothing, as I think this was handled above)")
}

func onBoseDeviceDisconnected() {

}


}
extension BoseDeviceStateDelegate: BoseHeadingUpdateDelegate {
func onHeadingUpdate(newHeading: HeadingValue!) {
GDLogHeadphoneMotionInfo("[PING] BoseMotionReachability received a heading update")
self.pingHandler(true)
}
}
*/
Copy link
Member

Choose a reason for hiding this comment

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

Can remove this commented-out block.

Comment on lines +32 to +52
/*
case self.currentSensorConfig?.accelerometerId:
GDLogBLEInfo("Got an accellerometer data update, read 10 bytes: \(valueAsArr)")
processVectorData(vectorByteArray: valueAsArr)
return

case self.currentSensorConfig?.gyroscopeId:
GDLogBLEInfo("Got an gyroscope data update, read 10 bytes: \(valueAsArr)")
processVectorData(vectorByteArray: valueAsArr)
return


case self.currentSensorConfig?.gamerotationId:
GDLogBLEInfo("Got an gameRotation data update, read 11(!) bytes: \(valueAsArr)")
processQuaternionData(quaternionByteArray: valueAsArr, hasAccuracy: false)
return

default:
GDLogBLEError("READ: Unknown sensor!")
return
*/
Copy link
Member

Choose a reason for hiding this comment

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

Can remove this commented-out block.

Comment on lines +238 to +243
// I think this one was wrong...
/* var vecX: [Int16] = [_matrix.elements[0], _matrix.elements[4], _matrix.elements[8]]
var vecY: [Int16] = [_matrix.elements[1], _matrix.elements[5], _matrix.elements[9]]
var vecZ: [Int16] = [_matrix.elements[2], _matrix.elements[6], _matrix.elements[10]]
*/

Copy link
Member

Choose a reason for hiding this comment

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

Can remove this commented-out block.

case .ready:
return GDLocalizedString("settings.bluetooth.forget")
default:
return "What should we put here? What can the device state be when View.state is .paired"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe return GDLocalizationUnnecessary("")?

@NikMel
Copy link
Contributor Author

NikMel commented Feb 3, 2025

Thanks for doing all this work! Sorry if it takes us a while to get round to merging it.

Glad that our work can be of use. And no worries about taking time to merge. On our side, we're deploying from our develop-branch, so we're not blocked. Take your time :)

We can't merge without first reverting the unrelated changes such as the app name and entitlements. I see this is from your main branch which you might want to keep as is. I would suggest creating another branch for the PR to keep it separate.

You could either create a branch off soundscape-community/soundscape main and then cherry-pick the relevant commits, or branch from your main and revert the unwanted changes.

We were planning to use main as out upstream-branch, but let's see how this PR plays out! We may reconsider...

Thanks for the quick feedback! We'll address the comments shortly!

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.

4 participants