-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add supervision timeout to CBMPeriphalSpec
, make rssi deviation public and mutable
#117
Conversation
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.
Had to update the implementation of the RSSI for Swift 6 clients. A public shared mutable variable
|
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Two changes to make the library more friendly for unit testing to address #116.
internal private(set)
mutable variable that can be set via a new public functionsimulateRSSIDeviation
. It is serves as configuration toCBCentralManagerMock
. A new internal enumDeviation
has been added toCBMPromixity
to limit the possible values to either.default
which is the current implementation of+/- 15 dBm
and.none
which is 0.CBMPeripheralSpec
so that it can be optionally initialized with.connectable
similar to themtu
andconnectionInterval
. Default value is4.0
seconds to match the existing private implementation.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
100,000 Repeated Tests