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

Functionality to set preferred PHY and to read the set PHY #840

Merged
merged 19 commits into from
Jan 4, 2024
Merged

Functionality to set preferred PHY and to read the set PHY #840

merged 19 commits into from
Jan 4, 2024

Conversation

JamesDougherty
Copy link
Contributor

Details are in the following issue: #839

@CLAassistant
Copy link

CLAassistant commented Oct 16, 2023

CLA assistant check
All committers have signed the CLA.

@dariuszseweryn
Copy link
Owner

I'll try to look at this PR this week

@JamesDougherty
Copy link
Contributor Author

I'm working on one of our projects today and was creating an annotation for the PHY values. One thing that came to mind is how to ensure people using the library use the correct PHY values as this causes confusion. For instance, when calling the setPreferredPhy function the *_MASK constants are used since the values can be OR'd together:

PHY_LE_1M_MASK = 1 (1 << 0)
PHY_LE_2M_MASK = 2 (1 << 1)
PHY_LE_CODED_MASK = 4 (1 << 2)

This is the main reason I redefined them in the RxBleConnection class. However, when the values are being read (i.e. onPhyUpdated), the non-mask constants are used:

PHY_LE_1M = 1
PHY_LE_2M = 2
PHY_LE_CODED = 3

As you can see the coded PHY constants differ. If someone wanted to request coded PHY and accidently used the non-mask version for the setPreferredPhy function (i.e. 3), then it would be the same as (PHY_LE_2M_MASK | PHY_LE_1M_MASK ), which would obviously give undesired results.

It would be nice to use something like an enum so the user knows what values are expected, but you can't use bitwise operators on enums in Java. I'll put more thought into it as I'm working on our project and if I think of an ideal solution, then I'll run it past your to get your input. Likewise, if you happen to think of something, then that would be great too.

* Improved the code to improve readability
* Unified the PHY setting options so there is no longer any confusion on the correct values to use (*_MASK vs non-*_MASK settings)
Removed several annotations that were created, but not used.
@JamesDougherty
Copy link
Contributor Author

I thought about this some more and changed the code some to address the issue I brought up in the previous comment. Here are the changes I made:

  • Removed the PHY constants in the RxBleConnection class and created an enum to hold the values. You can also still use bit flags by using EnumSet (see example below).
    • Note that this eliminates the confusion with the PHY_LE_CODED_MASK vs PHY_LE_CODED that I mentioned in my previous comment. The library will make the necessary changes on the backend to accommodate this. It should be seamless and straightforward to anyone using it.
  • Created a PhyPair class that will hold the Tx and Rx values
  • Removed the use of Pair<Integer, Integer> and replaced it with the new PhyPair class. This eliminates the confusion when reading the PHY values with the readPhy function. The user no longer needs to remember what the first() value is or the second() value is, which makes it more user friendly and improves readability.
  • There is a new PHY_UNKNOWN (in the RxBlePhy enum) that is used if the PHY can't be read. If that value is used for the setPreferredPhy function then the radio will use the default PHY, which should always be 1 Mb/s.

New Example Use

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
    addDisposable(mConnectionObservable
            .flatMapSingle(connection -> connection.setPreferredPhy(
                    EnumSet.of(RxBlePhy.PHY_2M, RxBlePhy.PHY_1M),
                    EnumSet.of(RxBlePhy.PHY_2M, RxBlePhy.PHY_1M),
                    RxBlePhyOption.PHY_OPTION_NO_PREFERRED
            ))
            .take(1)
            .subscribe(
                    (@NonNull final Boolean succeeded) ->
                            CsLog.bluetooth("PHY Set [Succeeded: " + succeeded + "]"),

                    (@NonNull final Throwable throwable) ->
                            CsLog.bluetooth("On setPreferredPhy Error: " + throwable.getMessage())
            )
    );

    addDisposable(mConnectionObservable
            .flatMapSingle(RxBleConnection::readPhy)
            .take(1)
            .subscribe(
                    (@NonNull final PhyPair phy) ->
                            CsLog.bluetooth("PHY Read - Tx: " + phy.txPhy.name() + "  Rx: " + phy.rxPhy.name()),

                    (@NonNull final Throwable throwable) ->
                            CsLog.bluetooth("On setPreferredPhy Error: " + throwable.getMessage())
            )
    );
}

Phy

I'm done making changes for now until I hear back from you. I'll tweak and make changes you need once you review everything.

* Changed to explicitly use PHY_1M as the default if no options were specified.
* Added explicit values to the PHY options (RxBlePhyOption) so the ordinal is not relied on, which is again best practices. Changed to use getValue() like RxBlePhy.
@JamesDougherty
Copy link
Contributor Author

Sorry, I made a few minor changes.

  • Changed to explicitly use PHY_1M as the default if no options were specified instead of assuming the radios default (even though it should always be PHY_1M).
  • Added explicit values to the PHY options (RxBlePhyOption) so the ordinal is not relied on, which is again best practices. Changed to use getValue() like RxBlePhy.

Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

This took me significantly more time to look on than I anticipated. I have added some comments. Could you also add a test for the new operation?

@JamesDougherty
Copy link
Contributor Author

Thank you for taking the time to review the changes. I will go over these next week and push an update with the changes.

JamesDougherty and others added 3 commits November 6, 2023 07:44
* Added some comments and documentation to the code
* Made requested changes (see #840)
@JamesDougherty
Copy link
Contributor Author

I was able to make the changes you requested and added some comments to clarify a few things. I added some testing, but I'm not 100% familiar with Rx testing framework. Let me know if there is anything else that needs tweaked. Appreciate it.

* Changed it to handle the logic better when setting the preferred PHY
@JamesDougherty
Copy link
Contributor Author

I cleaned up the logic that was taking the values from the EnumSet and converting them to the value setPreferredPhy was expecting. I'm working with Nordic on something and in doing so I noticed that it wasn't handling the cases properly. I changed it to explicitly handle no items in the EnumSet, only one item in the EnumSet or multiple.

Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

We're almost there. Just minor things left, mostly documentation and naming.

And again, sorry for the silent time. My first child was born in the meantime and it was pretty hectic on my side :) Anyway — I have a few weeks off-work now so I should be more responsive.

Comment on lines 603 to 605
* @implNote RxBlePhy.PHY_UNKNOWN is used for the onPhyRead and onPhyUpdate callbacks in cases where the GATT operation
* was not successful. Using RxBlePhy.PHY_UNKNOWN as the sole value in either of the txPhy or rxPhy parameters
* will result in the default value being used (RxBlePhy.PHY_1M).
Copy link
Owner

Choose a reason for hiding this comment

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

Is a case where status == GATT_SUCCESS but the operation was not successful possible? I haven't played with PHY myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The status == GATT_SUCCESS just means that the request was sent successfully. It's ultimately up to the Bluetooth controller to accept the request or deny it. Also, some controllers don't even support PHY updates, while others are optional. For instance, with the nRF52840 you can set it so PHY negotiation isn't an option and you can give it a static PHY you want the controller to run at. I don't believe this would be common for controllers that plan on having mobile app interactions though. It's more for cases where you build two devices, you control both devices, and you want them to only talk to each other. In that case you have control of both ends and can optimize the PHY based on the project. For instance, you have a device at one end and then use CODED PHY to communicate to another device that a mile away type of thing. In that case, PHY 1Mbps and PHY 2Mbps wouldn't work.

Here is part of the documentation:
Set the preferred connection PHY for this app. Please note that this is just a recommendation, whether the PHY change will happen depends on other applications preferences, local and remote controller capabilities. Controller can override these settings. BluetoothGattCallback.onPhyUpdate will be triggered as a result of this call, even if no PHY change happens. It is also triggered when remote device updates the PHY.

To summarize, it's only if the request was sent successfully and nothing more.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, the documentation of BluetoothGatt says that the callback will be called even if nothing changes. In this case I would expect current PHY settings being correctly returned — that would be PHY_LE_1M in case the peripheral would be a 4.0 device or other valid result if the peripheral supports it.

In other words I think that RxBlePhy.PHY_UNKNOWN value could be useful only if more PHY settings would be added to the Bluetooth Specification in the future. The user has no use-case to use this value on their own. If that's the case maybe we could come up with a better future-proof approach so the library would not need an update as soon as new BT spec is released. WDYT?

We could achieve that by making RxBlePhy an interface that would return a value (and something more probably). That interface could have some static properties that would be its own implementations. Then we would be able to return a Set of predefined values and log warnings when the OS would give us unrecognised values but still return them. WDYT?

return RxBlePhy.PHY_UNKNOWN;
}

public static int enumSetToInt(EnumSet<RxBlePhy> set) {
Copy link
Owner

Choose a reason for hiding this comment

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

This is a bit vague imo. Maybe enumSetToValuesMask?
Additionally, since this is a public function, maybe it is worth to have a javadoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see the latest commit. I also added javadoc to the other public function as well, just to be complete. I also made a minor change to allow the EnumSet to be null as well.

@JamesDougherty
Copy link
Contributor Author

Hey! No worries, we're in crunch time on our project, so it's been a little crazy on my end. Congratulations on your newborn! Enjoy every minute of it as it goes by quick. My one and only is turning 16 in February, seems like it was just yesterday.

I'll review the other suggestions you had and make the changes today.

Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

I have found one minor problem. Generally this PR is mergeable from the quality standpoint for me. I would like to hear your thoughts about an idea of RxBlePhy being interface to future-proof potential new PHY values.

If this PR would be created from a non-master branch maybe I could be contributing directly — unfortunately I do not have permissions to write to your master (obviously). I have added two operation tests, please commit them:

OperationReadPhyTest.groovy

package com.polidea.rxandroidble2.internal.operations

import android.bluetooth.BluetoothGatt
import com.polidea.rxandroidble2.PhyPair
import com.polidea.rxandroidble2.RxBlePhy
import com.polidea.rxandroidble2.RxBlePhyOption
import com.polidea.rxandroidble2.exceptions.BleGattCallbackTimeoutException
import com.polidea.rxandroidble2.exceptions.BleGattOperationType
import com.polidea.rxandroidble2.internal.connection.RxBleGattCallback
import com.polidea.rxandroidble2.internal.serialization.QueueReleaseInterface
import com.polidea.rxandroidble2.internal.util.MockOperationTimeoutConfiguration
import io.reactivex.schedulers.TestScheduler
import io.reactivex.subjects.PublishSubject
import spock.lang.Specification

import java.util.concurrent.TimeUnit

public class OperationPhyReadTest extends Specification {

    static long timeout = 10
    static TimeUnit timeoutTimeUnit = TimeUnit.SECONDS
    QueueReleaseInterface mockQueueReleaseInterface = Mock QueueReleaseInterface
    BluetoothGatt mockBluetoothGatt = Mock BluetoothGatt
    RxBleGattCallback mockGattCallback = Mock RxBleGattCallback
    TestScheduler testScheduler = new TestScheduler()
    PublishSubject<PhyPair> readPhyPublishSubject = PublishSubject.create()
    PhyReadOperation objectUnderTest

    def setup() {
        mockGattCallback.getOnPhyRead() >> readPhyPublishSubject
        prepareObjectUnderTest()
    }

    def "should call BluetoothGatt.readPhy() exactly once when run()"() {

        when:
        objectUnderTest.run(mockQueueReleaseInterface).test()

        then:
        1 * mockBluetoothGatt.readPhy()
    }

    def "should emit an error if RxBleGattCallback will emit error on RxBleGattCallback.getOnPhyRead() and release queue"() {

        given:
        def testSubscriber = objectUnderTest.run(mockQueueReleaseInterface).test()
        def testException = new Exception("test")

        when:
        readPhyPublishSubject.onError(testException)

        then:
        testSubscriber.assertError(testException)

        and:
        (1.._) * mockQueueReleaseInterface.release() // technically it's not an error to call it more than once
    }

    def "should timeout if will not response after 10 seconds "() {

        given:
        def testSubscriber = objectUnderTest.run(mockQueueReleaseInterface).test()

        when:
        testScheduler.advanceTimeTo(timeout + 5, timeoutTimeUnit)

        then:
        testSubscriber.assertError(BleGattCallbackTimeoutException)

        and:
        testSubscriber.assertError {
            ((BleGattCallbackTimeoutException)it).getBleGattOperationType() == BleGattOperationType.PHY_READ
        }
    }

    private prepareObjectUnderTest() {
        objectUnderTest = new PhyReadOperation(mockGattCallback, mockBluetoothGatt,
                new MockOperationTimeoutConfiguration(timeout.intValue(), testScheduler))
    }
}

OperationUpdatePhyTest.groovy

package com.polidea.rxandroidble2.internal.operations

import android.bluetooth.BluetoothDevice
import android.bluetooth.BluetoothGatt
import com.polidea.rxandroidble2.PhyPair
import com.polidea.rxandroidble2.RxBlePhy
import com.polidea.rxandroidble2.RxBlePhyOption
import com.polidea.rxandroidble2.exceptions.BleGattCallbackTimeoutException
import com.polidea.rxandroidble2.exceptions.BleGattOperationType
import com.polidea.rxandroidble2.internal.connection.RxBleGattCallback
import com.polidea.rxandroidble2.internal.serialization.QueueReleaseInterface
import com.polidea.rxandroidble2.internal.util.MockOperationTimeoutConfiguration
import io.reactivex.schedulers.TestScheduler
import io.reactivex.subjects.PublishSubject
import spock.lang.Specification

import java.util.concurrent.TimeUnit

public class OperationPhyUpdateTest extends Specification {

    static long timeout = 10
    static TimeUnit timeoutTimeUnit = TimeUnit.SECONDS
    QueueReleaseInterface mockQueueReleaseInterface = Mock QueueReleaseInterface
    BluetoothGatt mockBluetoothGatt = Mock BluetoothGatt
    RxBleGattCallback mockGattCallback = Mock RxBleGattCallback
    TestScheduler testScheduler = new TestScheduler()
    PublishSubject<PhyPair> updatedPhyPublishSubject = PublishSubject.create()
    PhyUpdateOperation objectUnderTest
    EnumSet<RxBlePhy> rxSet = EnumSet.of(RxBlePhy.PHY_1M)
    EnumSet<RxBlePhy> txSet = EnumSet.of(RxBlePhy.PHY_1M, RxBlePhy.PHY_2M)
    RxBlePhyOption phyOption = RxBlePhyOption.PHY_OPTION_S8

    def setup() {
        mockGattCallback.getOnPhyUpdate() >> updatedPhyPublishSubject
        prepareObjectUnderTest()
    }

    def "should call BluetoothGatt.setPreferredPhy(int, int, int) exactly once when run()"() {

        when:
        objectUnderTest.run(mockQueueReleaseInterface).test()

        then:
        1 * mockBluetoothGatt.setPreferredPhy(
                RxBlePhy.enumSetToValuesMask(txSet),
                RxBlePhy.enumSetToValuesMask(rxSet),
                phyOption.value
        )
    }

    def "should emit an error if RxBleGattCallback will emit error on RxBleGattCallback.getOnPhyUpdate() and release queue"() {

        given:
        def testSubscriber = objectUnderTest.run(mockQueueReleaseInterface).test()
        def testException = new Exception("test")

        when:
        updatedPhyPublishSubject.onError(testException)

        then:
        testSubscriber.assertError(testException)

        and:
        (1.._) * mockQueueReleaseInterface.release() // technically it's not an error to call it more than once
    }

    def "should timeout if will not response after 10 seconds "() {

        given:
        println(objectUnderTest.toString())
        def testSubscriber = objectUnderTest.run(mockQueueReleaseInterface).test()

        when:
        testScheduler.advanceTimeTo(timeout + 5, timeoutTimeUnit)

        then:
        testSubscriber.assertError(BleGattCallbackTimeoutException)

        and:
        testSubscriber.assertError {
            ((BleGattCallbackTimeoutException)it).getBleGattOperationType() == BleGattOperationType.PHY_UPDATE
        }
    }

    private prepareObjectUnderTest() {
        objectUnderTest = new PhyUpdateOperation(mockGattCallback, mockBluetoothGatt,
                new MockOperationTimeoutConfiguration(timeout.intValue(), testScheduler), txSet, rxSet, phyOption)
    }
}

protected boolean startOperation(BluetoothGatt bluetoothGatt) {
bluetoothGatt.setPreferredPhy(
RxBlePhy.enumSetToValuesMask(txPhy),
RxBlePhy.enumSetToValuesMask(txPhy),
Copy link
Owner

Choose a reason for hiding this comment

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

Probably a copy-paste error. Found that when adding tests.

Suggested change
RxBlePhy.enumSetToValuesMask(txPhy),
RxBlePhy.enumSetToValuesMask(rxPhy),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

Comment on lines 603 to 605
* @implNote RxBlePhy.PHY_UNKNOWN is used for the onPhyRead and onPhyUpdate callbacks in cases where the GATT operation
* was not successful. Using RxBlePhy.PHY_UNKNOWN as the sole value in either of the txPhy or rxPhy parameters
* will result in the default value being used (RxBlePhy.PHY_1M).
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, the documentation of BluetoothGatt says that the callback will be called even if nothing changes. In this case I would expect current PHY settings being correctly returned — that would be PHY_LE_1M in case the peripheral would be a 4.0 device or other valid result if the peripheral supports it.

In other words I think that RxBlePhy.PHY_UNKNOWN value could be useful only if more PHY settings would be added to the Bluetooth Specification in the future. The user has no use-case to use this value on their own. If that's the case maybe we could come up with a better future-proof approach so the library would not need an update as soon as new BT spec is released. WDYT?

We could achieve that by making RxBlePhy an interface that would return a value (and something more probably). That interface could have some static properties that would be its own implementations. Then we would be able to return a Set of predefined values and log warnings when the OS would give us unrecognised values but still return them. WDYT?

@JamesDougherty
Copy link
Contributor Author

I invited you to be a member of the repository, so you can push whatever changes you would like. I'm going to be on vacation until January, but I'll keep an eye on this in case you need anything else.

Thank you for creating those tests!

I manly created the PHY_UNKNOWN to try and catch those special cases, but creating something more elaborate isn't a bad idea either. I chose for the catch-all route because it takes years for Bluetooth SIG to approve anything. You're right that users shouldn't use it directly, but I added a case for that as well just to be safe.

…xBlePhy.UNKNOWN. Closed to modification, opened to extension.
@dariuszseweryn
Copy link
Owner

I have did (probably too much) thinking and came up with the interfaces. With the last PR we should be able to use static values for the standard operation. Yet when there will be more PHY (options) available one can provide their own interface implementation which should unlock immediately without any changes to the lib — in that case a warning will be logged with a request for a PR.
WDYT?

@JamesDougherty
Copy link
Contributor Author

JamesDougherty commented Jan 4, 2024

I seen where SIG is indeed going to be changing it. They're getting into the unlicensed 6 GHz realm and they're also going to be making a PHY 3M, a PHY 4M, and maybe a PHY 5M since they need 4.6 Mbps (I assume?). Here is a quote:

"In fact, the proposed increase in Bluetooth LE’s data rate would be enough for lossless hi-res audio at up to 24-bit/96kHz, which needs 4.6 megabits per second (Mbps), and even standard-definition video (between 3 and 4 Mbps)."
https://www.digitaltrends.com/home-theater/bluetooth-data-throughput-will-double-lossless-audio/

Here is where they mention it on their official website:
https://www.bluetooth.com/specifications/specifications-in-development/

With that being said, the changes you made are well warranted, sound great, and look great. We don't make libraries of this nature since all of our code is proprietary, but this is a great solution and one I'll keep in mind for the future.

@dariuszseweryn dariuszseweryn merged commit e9e45cc into dariuszseweryn:master Jan 4, 2024
3 checks passed
@dariuszseweryn
Copy link
Owner

And it is being released. Thank you for your time and collaboration! Cheers!

@JamesDougherty
Copy link
Contributor Author

Awesome! You're more than welcome. It was something we've been wanting to see and I seen it being requested by several others, so it's good to give back. Thank you for helping to wrap it up, appreciate it. Cheers 🍻

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.

3 participants