Conversation
Byte Utils Client side binding
Read/Write will now just provide an accessor for completion. value() for Descriptor removed value() for Characteristic will automatically handle subscription
Read/write still untested
Code coverage
|
thoutbeckers
left a comment
thoutbeckers
left a comment
There was a problem hiding this comment.
some small things that can be looked at, other things (efficiency etc) could be for later consideration.
I do also think more extensive tests is of course desirable, but it's worth it to wait for server <-> client mocking perhaps.
| fun buildByteArray(order: ByteOrder = ByteOrder.LEAST_SIGNIFICANT_FIRST, block: ByteArrayBuilder.() -> Unit) = ByteArrayBuilderImpl(order).apply(block).bytes | ||
|
|
||
| private class ByteArrayBuilderImpl(val order: ByteOrder) : ByteArrayBuilder { | ||
| var bytes = byteArrayOf() |
There was a problem hiding this comment.
This implementation does have some inefficiencies with copying this byte array over and over (both copies in general but in particular object allocation)
I appreciate however that it's behind an interface, and it's probably the best way to create a reference implementation and validate the test suite etc.
The common optimization is to expand the array in chunks, and have a size suggestion for the first chunk. Compare API/implementation of https://docs.oracle.com/javase/8/docs/api/java/io/ByteArrayOutputStream.html. In the implementationcurrentByte would be an index pointer in this case, and when you actually write you check the index range to see if you need to add another chunck (I think it's common to double)
Also, converting other values to ByteArray (like Long.toByteArray) could fairly easily be modified to (also) support writing into an existing an existing ByteArray, again avoiding copies.
If the user size hints the chunk correctly (often in such scenarions it's easy to know) and you take care to only expand the array when there is an actual mutation past the boundry you can construct the byte array with 0 copies. In application code you can add a warning if your eventual size does not match your hint so you can build this robustly too.
Alternetly/additionally, a secondary fixed size builder could be considered (if the size is known ahead of time you might as well fail if it's not that size).
To be clear, I do support keeping this implementation in any case. Whether we add a (usually) more optimal one can be up for debate. It seems a bit nitty but even for small byte structures it's easily an order of magnitude less objects.
I think with the serialization stuff we probably can calculate a good "hint" or even know when the result is fixed size.
| @@ -0,0 +1,23 @@ | |||
| package com.splendo.kaluga.base.utils | |||
|
|
|||
| infix fun Byte.shl(bitCount: Int) = toUByte().shl(bitCount).toByte() | |||
There was a problem hiding this comment.
AFAIK these methods are free (no runtime cost), but have you considered just going UByteBuilder by default?
| Switch this value to use the location permission on Android when using bluetooth. | ||
| */ | ||
| const val useBluetoothForLocation = false | ||
| const val USE_LOCATION_FOR_BLUETOOTH = true |
There was a problem hiding this comment.
I think this should be false by default.
There was a problem hiding this comment.
Reverted, but note that since the Example also demoes beacons, which do need the location permission, we cannot add neverForLocation and the example will just not scan anything.
There was a problem hiding this comment.
We don't actually derive location from the beacons.
There was a problem hiding this comment.
Not having neverForLocation is weird though, the example seemed to work without this. Maybe missed commiting it on my side.
| val captor = AnyOrNullCaptor<DeviceAction<*>>() | ||
| connectionManager.performActionMock.verifyWithin(value = captor) | ||
| assertIs<DeviceAction.Notification.Enable>(captor.lastCaptured) | ||
| yield() |
There was a problem hiding this comment.
the cause of having to do these random yields is probably still using BluetoothFlowTest and mainAction, something from the times of not having background dispatchers on iOS.
| ), | ||
| ) { | ||
| test { | ||
| assertEquals(Double.NaN, it) |
There was a problem hiding this comment.
Not the most used API but could probably use a (value) type in a 2.0 API.
| ) : IOSServerState(), | ||
| ServerState.AwaitingBluetoothEnabled { | ||
|
|
||
| private val serverQueue = dispatch_queue_create("BluetoothServer", null) |
There was a problem hiding this comment.
Should this not be raised up, maybe as a lazy field? AFAIK these queues will not autoclose.
There was a problem hiding this comment.
They should auto close. There is also no way of closing them manually as dispatch_release has been deprecated and Apple explicitly says not to use it (anymore)
bluetooth/src/iosMain/kotlin/server/KalugaCBPeripheralManagerDelegate.kt
Show resolved
Hide resolved
…negrained-logging
Less copying used in implementation
|
@thoutbeckers the most pressing issues have been fixed here #925 |
PR remarks for BT
…gging Added support for fully customizing BT logging
Bluetooth Serializer
Make extension accessors have same safety as other functions
…_builder_encoding_extensions
…ing_extensions More ByteBuilder optimizations
Fix for isNotifying not being a StateFlow
3 participants
A significant rewrite of the Bluetooth API (2.0) that focusses primarily on supporting Bluetooth Servers.
Changes to Client Side
To make the API as good as possible, some breaking changes had to be made:
Service,CharacteristicandDescriptorhave been renamed toRemoteService,RemoteCharacteristicandRemoteDescriptorrespectively. Their old names have now been reserved for an interface that spans both Server (Local) and Client (Remote) implementations.ConnectableDevice. From the server side it is calledConnectedDevice.BaseBluetoothBuilder.createhas been deprecated. UsecreateClientinstead.device.services()[uuid]may now fail if called before services have been discovered. Usedevice.discoveredServices()[uuid]instead if you only want a flow of the service after discovery (this will fail if service is not present), or usedevices.services.getOrNull(uuid). This aligns better with other Kotlin APIsRemoteCharacteristicandRemoteDescriptorare no longer Flows of ByteArray. Instead:GattResponse.ReadResponse. If this isReadSuccessthe data read will be available as its value.GattResponse.WriteResponse.RemoteCharacteristic.subscribe {}. The Resulting Subscription must be used to unsubscribe. Calling this method will automatically callenabledNotifications/disableNotifications()if needed.Flow<RemoteCharacteristic?>.value()will automatically subscribe/unsubscribe as the flow is being collected.Flow<RemoteDescriptor?>.value()has been removed.Bindapi makes it easy to bind an object to aConnectedDevice,RemoteService,RemoteCharacteristic, orRemoteDescriptor. Simply callobject.bind(service) {}Within its closure you can set up observation actions and read or write triggers. The resulting StateFlow will help keep track of any mutations that happened to the object.Server
BaseBluetoothBuilderhas a new methodcreateServerto set up a Bluetooth server.notifiable. The resultingLocalCharacteristic.Notifiablehas methods for sending notifications to devices. A conveniencecollectAsNotification/consumeAsNotificationapi offers the ability to bind flows directly to a characteristic, notifying each subscriber as the flow updates.