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

Add supervision timeout to CBMPeriphalSpec, make rssi deviation public and mutable #117

Merged
merged 8 commits into from
Mar 11, 2025

Conversation

nathandud
Copy link
Contributor

@nathandud nathandud commented Mar 10, 2025

Two changes to make the library more friendly for unit testing to address #116.

  1. RSSI deviation is now an internal private(set) mutable variable that can be set via a new public function simulateRSSIDeviation. It is serves as configuration to CBCentralManagerMock. A new internal enum Deviation has been added to CBMPromixity to limit the possible values to either .default which is the current implementation of +/- 15 dBm and .none which is 0.
CBMCentralManagerMock.simulateRSSIDeviation(.none)
  1. The supervisionTimeout has been added to CBMPeripheralSpec so that it can be optionally initialized with .connectable similar to the mtu and connectionInterval. Default value is 4.0 seconds to match the existing private implementation.
.connectable(
    name: "Test_Device",
    services: [],
    delegate: self,
    connectionInterval: 0.01,
    supervisionTimeout: 0,
    mtu: 32
)

These are non-breaking changes that only add functionality to the existing API. Existing clients are unaffected.

Adding the above changes to the unit test in #116 allow the tests to pass without any delays

Single test

Test Case '-[BluetoothMockExampleTests.BluetoothDelegateTests testPeripheralConnectThenOutOfRange]' passed (0.048 seconds).
Test Suite 'BluetoothDelegateTests' passed at 2025-03-10 12:52:04.728.
	 Executed 1 test, with 0 failures (0 unexpected) in 0.048 (0.049) seconds

100,000 Repeated Tests

Test Suite 'BluetoothDelegateTests' passed at 2025-03-10 13:35:49.387.
	 Executed 100000 tests, with 0 failures (0 unexpected) in 1147.365 (1164.023) seconds

@nathandud nathandud marked this pull request as draft March 10, 2025 02:29
Originally this was a publicly mutable static var but that is an issue for clients using Swift 6 that throw an error for shared mutable state.
@nathandud
Copy link
Contributor Author

Had to update the implementation of the RSSI for Swift 6 clients. A public shared mutable variable rssiDeviation throws an error in Swift 6. In theory, simulateRSSIDeviation(:) is not 100% thread safe but repeated tests succeed as the internal rssiDeviation is not an oft-accessed bit of state. The entire package could be updated for Swift Concurrency but that would be a much more ambitious change.

Test Suite 'BluetoothDelegateTests' passed at 2025-03-10 19:33:55.930.
	 Executed 1000 tests, with 0 failures (0 unexpected) in 11.465 (11.629) seconds

@nathandud nathandud marked this pull request as ready for review March 10, 2025 06:56
Copy link
Member

@philips77 philips77 left a comment

Choose a reason for hiding this comment

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

Excellent work! Thank you!
I added just few minor change requests. When you fix them, I'll push a release.

@@ -49,6 +49,14 @@ public enum CBMProximity {
case .outOfRange: return 127
}
}

/// The amount the proximity should randomly deviate when reporting RSSI
public enum Deviation: Int {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps, instead of extending Int, there could be a computed property reporting the deviation?
You could also add a custom(_ deviation: Int) to improve it even more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this - it opts for a more internal implementation and maintains a clean public API.

I added a case for a custom value like you recommended. There is potential for a user to set a deviation of 10000+ and the RSSI can be something well out-of-range for its associated proximity, but it doesn't affect anything downstream and won't cause the library to crash or anything. I successfully ran multiple tests with a variety of RSSI deviation values. However, the value must be >= 0 so the absolute value is used if the user provides a negative value. I toyed with the idea of using UInt8 but that just felt clunky especially since CBProximity.Deviation is part of the public-facing API.

Better punctuation in comments
Added custom RSSI deviation case
@nathandud nathandud requested a review from philips77 March 11, 2025 00:56
@philips77 philips77 merged commit c0f314c into NordicSemiconductor:main Mar 11, 2025
1 check passed
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