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

Finding characteristic by UUID + required property #128

Closed
wants to merge 9 commits into from

Conversation

solvek
Copy link

@solvek solvek commented Jun 29, 2021

This allows to find an appropriate characteristic when there are duplicating two characteristics by UUID (but with different property).
So for writing we use the charactertistic with WRITE or WRITE_NO_RESPONSE property, for observing we use only property with NOTIFY or INDICATE.

BTW, what is the easiest way to test this locally for me? How can I use my modified library in my local project?

@solvek solvek requested a review from twyatt as a code owner June 29, 2021 17:49
@solvek solvek requested review from a team and sdonn3 June 29, 2021 17:49
@twyatt twyatt removed the request for review from sdonn3 June 29, 2021 18:10
@twyatt
Copy link
Member

twyatt commented Jun 29, 2021

Thanks for the PR!
I'm pretty swamped at the moment, so I'll take a look at this PR when I have a free moment.

BTW, what is the easiest way to test this locally for me? How can I use my modified library in my local project?

cd kable/
./gradlew publishToMavenLocal

It will default to installing as version main-SNAPSHOT. To specify an alternate version, use the VERSION_NAME property:

./gradlew publishToMavenLocal -PVERSION_NAME=0.0.1-example-SNAPSHOT

Versions should be suffixed with -SNAPSHOT to disable publication signing.

Then, in the app that is using Kable, add mavenLocal() to list of repositories, for example:

allprojects {
    repositories {
        mavenLocal()
        google()
        mavenCentral()
    }
}

https://github.com/JuulLabs/sensortag/blob/main/build.gradle.kts#L19-L21

And where you use the library, to use your local version, for example:

kotlin {
    android()

    sourceSets {
        val commonMain by getting {
            dependencies {
                implementation("com.juul.kable:core:main-SNAPSHOT")
            }
        }
    }
}

@solvek
Copy link
Author

solvek commented Jun 30, 2021

Thanks a lot.
Tested this locally and fixed a small bug.

@twyatt
Copy link
Member

twyatt commented Jul 1, 2021

I really like the idea of only performing operations on characteristics that identify as having the corresponding capabilities (i.e. only searching for characteristic that have properties that indicate they can be observed for starting observations) but I worry that all too often peripherals are improperly implemented and might not be setup with the correct properties.

I'm thinking this functionality should be configurable; the hard part: thinking of what the terminology would be for the configuration for how to search for characteristics.

What you have in this PR (which is great) I would refer to as strict search mode, but not quite sure what I would call the configuration parameter. Seems like a good fit for the peripheral builder lambda though.

@twyatt
Copy link
Member

twyatt commented Jul 25, 2021

@solvek thanks again for this PR! Apologies I haven't given it the attention it deserves. I haven't forgotten about it and I'll try to find time soon to get it reviewed and/or merged.

@twyatt twyatt added this to the 0.10.0 milestone Aug 29, 2021
@twyatt
Copy link
Member

twyatt commented Sep 3, 2021

Apologies it has taken so long to follow up on this.

Took some time and was going to update this PR with some minor implementation updates but then ran into the following unknowns that I'm not sure the best way this should be approached.

Imagine the following scenario; a user has a peripheral with the following characteristic (and corresponding properties):

Characteristic Properties
A Indicate
B Write
C Notify

Let's assume they all have the same UUID (definitely a poorly structured service/characteristic tree, but lets go with this extreme as an example).

For Peripheral.observe, which characteristic should we observe? If we do an iterative search then A would technically be a good match, but C is also a good candidate (and for most users Notify is usually what would be preferred).

I'd imagine it'd be good to prefer characteristic properties (for observe) in the following order:

  • Notify
  • Indicate
  • first match

Whereas we search all characteristics with matching UUID, if we find Notify then our search is done, if not then search for Indicate, if not found then first match. Perhaps something along the lines of?:

private fun List<PlatformService>.findObserveCharacteristic(
    serviceUuid: Uuid,
    characteristicUuid: Uuid,
): PlatformCharacteristic {
    val characteristics = first(serviceUuid).characteristics

    var indicateCharacteristic: PlatformCharacteristic? = null
    var firstMatch: PlatformCharacteristic? = null
    for (characteristic in characteristics) {
        if (characteristic.characteristicUuid == characteristicUuid) {
            val properties = characteristic.bluetoothGattCharacteristic.properties
            if (properties and PROPERTY_NOTIFY != 0) return characteristic
            if (indicateCharacteristic == null && properties and PROPERTY_INDICATE != 0) indicateCharacteristic = characteristic
            if (firstMatch == null) firstMatch = characteristic
        }
    }
    if (indicateCharacteristic != null) return indicateCharacteristic
    if (firstMatch != null) return firstMatch
    throw NoSuchElementException("No compatible characteristic found for observation matching service $serviceUuid, characteristic $characteristicUuid")
}

Some outstanding unknowns for me:

  • Does Android always return characteristics in the same order?
  • Is this a good search algorithm? To prefer certain properties (that way less preferred properties incur higher search cost)?

@solvek
Copy link
Author

solvek commented Sep 6, 2021

I see the point.
Because for both Indicate and Notify we use the same method there can be a conflict.
But in reality I do not think there can be such case. Most probably the same UUID will be used for Read/Notify or Write/Indicate.
So, your idea with priority should be working fine.

@twyatt
Copy link
Member

twyatt commented Sep 8, 2021

I see the point.
Because for both Indicate and Notify we use the same method there can be a conflict.
But in reality I do not think there can be such case. Most probably the same UUID will be used for Read/Notify or Write/Indicate.
So, your idea with priority should be working fine.

Due to the complexity of the new characteristic search mechanism, I'd like to put it behind a configuration flag. I'll try to find some time soon to update the PR and get it merged.

@twyatt twyatt modified the milestones: 0.10.0, 0.11.0 Sep 8, 2021
@twyatt twyatt removed this from the 0.11.0 milestone Nov 22, 2021
@solvek
Copy link
Author

solvek commented Dec 14, 2021

Hello
Is ths PR planned for 0.11 version?

@twyatt
Copy link
Member

twyatt commented Dec 14, 2021

Hello Is ths PR planned for 0.11 version?

Apologies, I've neglected this PR for way too long.
I should've been way more transparent about continually prioritizing other features/bugs.

I was honestly struggling to decide on the appropriate API for making this behavior configurable. I believe the implementation of this PR is more correct than the current implementation, but I worry that many library users may rely on the current behavior.

All that being said, thank you for pinging me about this, I have been swamped at work with other projects (that depend on other priorities for this library) but this PR certainly deserves the last bit of effort to get it integrated. I will do my best to find some time soon.

Again, sorry for the very long delay.

@twyatt
Copy link
Member

twyatt commented Jan 6, 2022

Update on this PR: I've begun work on a branch that performs the same functionality of this PR while also addressing #93, and supports multiplatform.

Once I have the related PR up, I'll close this PR.

Is ths PR planned for 0.11 version?

We'll likely ship 0.11.x w/o this functionality (simply to keep the changeset in 0.11.x small) but then we plan to release 0.12.x shortly after and it will include this functionality.

@twyatt
Copy link
Member

twyatt commented Jan 6, 2022

Superseded by #238.

@twyatt twyatt closed this Jan 6, 2022
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