Add basic Bluetooth LE advertising and scan support.

[xpconanfan]

• Basic advertising and scan features working.
• Change existing BT serialization to utilize Bundle for consistency with SnippetEvent and reducing dup code.

---

This change is 

[adorokhine]

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 17 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 38 at r1 (raw file):

/** Snippet class exposing Android APIs in WifiManager. */
@TargetApi(Build.VERSION_CODES.LOLLIPOP_MR1)

Seems like we need @RpcMinSdk on all of these @rpc methods?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 54 at r1 (raw file):

    public BluetoothLeAdvertiserSnippet() {
        mAdvertiser = BluetoothAdapter.getDefaultAdapter().getBluetoothLeAdvertiser();
        Log.i("Constructed BLE advertiser.");

Is this log valuable? We don't have this on the other snippets.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

     *
     * @param callbackId
     * @param advertiseSettings A JSONObject with the info on the settings for the advertising. For

Is this JSONObject just a serialized AdvertiseSettings instance? If so can we say that explicitly?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 65 at r1 (raw file):

     * @param callbackId
     * @param advertiseSettings A JSONObject with the info on the settings for the advertising. For
     *     example: {"AdvertiseMode": {@link AdvertiseSettings#ADVERTISE_MODE_BALANCED}, "Timeout":

Would it be possible to format this better?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 69 at r1 (raw file):

     *     AdvertiseSettings#ADVERTISE_TX_POWER_LOW} }
     * @param advertiseData A JSONObject representing the data to include in advertising beacon. For
     *     example: {"IncludeDeviceName": true, "ServiceData": [A Base64 encoded string],

Is ServiceData an arbitrary base64 encoded string? If so could we say that?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 70 at r1 (raw file):

     * @param advertiseData A JSONObject representing the data to include in advertising beacon. For
     *     example: {"IncludeDeviceName": true, "ServiceData": [A Base64 encoded string],
     *     "ServiceUuid": [A string representation of the UUID]}

Where does one get a UUID from? Can it be random or does it have to match something?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 89 at r1 (raw file):

    @Rpc(description = "Stop BLE advertising.")
    public void bleStopAdvertitsing(String id) throws BluetoothLeAdvertiserSnippetException {

Spelling in method name

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 90 at r1 (raw file):

    @Rpc(description = "Stop BLE advertising.")
    public void bleStopAdvertitsing(String id) throws BluetoothLeAdvertiserSnippetException {
        if (!mAdvertiseCallbacks.containsKey(id)) {

Just do remove() and check for null; less map lookups :)

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 97 at r1 (raw file):

    }

    private class DefaultAdvertiseCallback extends AdvertiseCallback {

Can this class be static?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 177 at r1 (raw file):

    @Override
    public void shutdown() {
        for (String id : mAdvertiseCallbacks.keySet()) {

You're iterating over the keySet() and doing get() for each one? Why not just iterate over the values()?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 39 at r1 (raw file):

/** Snippet class exposing Android APIs in WifiManager. */
@TargetApi(Build.VERSION_CODES.LOLLIPOP_MR1)

Needs @RpcMinSdk?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 58 at r1 (raw file):

    }

    @AsyncRpc(description = "Start BLE scan.")

We seem to have totally different scan APIs between bluetooth and bluetoothle; is there a fundamental reason? If not could we be consistent?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 64 at r1 (raw file):

                    "Bluetooth is disabled, cannot start BLE scan.");
        }
        DefaultScanCallback callback = new DefaultScanCallback((callbackId));

Double parens? Does that do anything?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 80 at r1 (raw file):

    public void shutdown() {
        for (String id : mScanCallbacks.keySet()) {
            mScanner.stopScan(mScanCallbacks.get(id));

Same concern

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonDeserializer.java, line 71 at r1 (raw file):

    public static ParcelUuid stringToParcelUuid(String uuidString) throws JSONException {
        return ParcelUuid.fromString(uuidString);

Do we really need this three-liner to wrap this one-liner?

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

    @TargetApi(Build.VERSION_CODES.LOLLIPOP_MR1)
    private Bundle serializeBleScanRecord(ScanRecord record) {
        Bundle result = new Bundle();

Are we no longer using gson to automatically convert all the fields? If so can we add javadocs showing the examples of what these serialized objects look like after we apply our filtering?

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 199 at r1 (raw file):

    }

    private String bleAdvertiseTxPowerToString(int advertiseTxPower) {

Can you reuse this in the advertiser rather than reproducing?

---

Comments from Reviewable

[xpconanfan]

Review status: all files reviewed at latest revision, 17 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 38 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Seems like we need @RpcMinSdk on all of these @rpc methods?

Sorry, I forgot what the difference is again...
Do we need both?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 54 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Is this log valuable? We don't have this on the other snippets.

Added for debugging, removed.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Is this JSONObject just a serialized AdvertiseSettings instance? If so can we say that explicitly?

It is not the same as the one serialized by our serializer as it has int instead of string for fields like "AdvertiseMode".

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 65 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Would it be possible to format this better?

Couldn't figure out a way that makes formatter happy. Suggestions?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 69 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Is ServiceData an arbitrary base64 encoded string? If so could we say that?

What do you mean by "arbitrary"?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 70 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Where does one get a UUID from? Can it be random or does it have to match something?

This is the UUID the user wants to advertise, related to how BLE advertising works.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 89 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Spelling in method name

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 90 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Just do remove() and check for null; less map lookups :)

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 97 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Can this class be static?

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 177 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

You're iterating over the keySet() and doing get() for each one? Why not just iterate over the values()?

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 58 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

We seem to have totally different scan APIs between bluetooth and bluetoothle; is there a fundamental reason? If not could we be consistent?

Because Android APIs for them are completely different.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 64 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Double parens? Does that do anything?

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 80 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Same concern

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonDeserializer.java, line 71 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Do we really need this three-liner to wrap this one-liner?

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Are we no longer using gson to automatically convert all the fields? If so can we add javadocs showing the examples of what these serialized objects look like after we apply our filtering?

Some of the ble APIs have nested complex types that gson doesn't handle very well.
To make sure the important fields have consistent names, I'm handling the serialization with custom code.

The custom code is pretty self-documenting in terms of what they produce though. I don't think explaining straightforward code with plain English again in javadoc is good practice.

Is there anything particular that's not clear from code? Maybe we could focus on that instead.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 199 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Can you reuse this in the advertiser rather than reproducing?

Done.

---

Comments from Reviewable

[adorokhine]

Reviewed 1 of 4 files at r2.
Review status: 4 of 7 files reviewed at latest revision, 8 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 38 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Sorry, I forgot what the difference is again...
Do we need both?

@TargetApi is a directive to Studio/Lint to avoid generating lint warnings for use of APIs newer than this value. It has no runtime retention so it can't be used at runtime.
@RpcMinSdk has runtime retention and is used by the snippet engine to throw a nice error if you call a new api on an old phone, instead of crashing with NoSuchMethod.
So yep, you need both.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

It is not the same as the one serialized by our serializer as it has int instead of string for fields like "AdvertiseMode".

Does that mean you can't take an AdvertiseMode from one of our methods and plug it back into another one of our methods? You'd have to rewrite the fields?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 65 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Couldn't figure out a way that makes formatter happy. Suggestions?

<pre> should work; does it really rewrite it?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 69 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

What do you mean by "arbitrary"?

I mean if a user wants to call bleStartAdvertising(), do they just make up a ranom base64-encoded string to plug into this field?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 70 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

This is the UUID the user wants to advertise, related to how BLE advertising works.

Can you mention that or give a link?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 58 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Because Android APIs for them are completely different.

I thought we weren't directly exposing Android APIs? Can we decide on a common API for all scannable things (wifi, bluetooth, ble) to make sure mbs is consistent and easy to use from module to module? How about scanAndGetResults(), getCachedResults(), enable() and disable() (if possible)?

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Some of the ble APIs have nested complex types that gson doesn't handle very well.
To make sure the important fields have consistent names, I'm handling the serialization with custom code.

The custom code is pretty self-documenting in terms of what they produce though. I don't think explaining straightforward code with plain English again in javadoc is good practice.

Is there anything particular that's not clear from code? Maybe we could focus on that instead.

I don't want to explain it, I want to give an example of the output :) One thing that's not clear from the code is "what fields does this have" because it calls to different functions so you have to jump around in the file and remember at what level you're nested. The other issue is that code does not end up in javadocs.

---

Comments from Reviewable

[xpconanfan]

Review status: 4 of 7 files reviewed at latest revision, 8 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 38 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

@TargetApi is a directive to Studio/Lint to avoid generating lint warnings for use of APIs newer than this value. It has no runtime retention so it can't be used at runtime.
@RpcMinSdk has runtime retention and is used by the snippet engine to throw a nice error if you call a new api on an old phone, instead of crashing with NoSuchMethod.
So yep, you need both.

Another question, is it sufficient to mark the class, or does every method need to be marked?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Does that mean you can't take an AdvertiseMode from one of our methods and plug it back into another one of our methods? You'd have to rewrite the fields?

Right.

I made the one returned through callback here return string for readability since this return value is not meant to be used to start another advertising.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 65 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

<pre> should work; does it really rewrite it?

Oh, I was expecting it to automatically format the json within pre... I guess I need to do it myself...

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 69 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

I mean if a user wants to call bleStartAdvertising(), do they just make up a ranom base64-encoded string to plug into this field?

No, this is a base64 string encoded from a regular string like "Hello World".

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 70 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Can you mention that or give a link?

Done.

---

Comments from Reviewable

[dthkao]

Review status: 3 of 7 files reviewed at latest revision, 8 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 70 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Done.

No change?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 58 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

I thought we weren't directly exposing Android APIs? Can we decide on a common API for all scannable things (wifi, bluetooth, ble) to make sure mbs is consistent and easy to use from module to module? How about scanAndGetResults(), getCachedResults(), enable() and disable() (if possible)?

+1 to this BT snippet family is slowly veering towards domain expertise.

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 7 files reviewed at latest revision, 8 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 70 at r1 (raw file):

Previously, dthkao (David T.H. Kao) wrote…

No change?

The object it needs is linked

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 7 files reviewed at latest revision, 8 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

I don't want to explain it, I want to give an example of the output :) One thing that's not clear from the code is "what fields does this have" because it calls to different functions so you have to jump around in the file and remember at what level you're nested. The other issue is that code does not end up in javadocs.

Since this code is just putting key-value pairs in a straightforward way, repeating the output of this in javadoc is just adding maintenance cost for no reason. These functions produce the output that can be easily checked if the user wants to see what the output looks like.

This is different from the deserialization function which requires users to create the input themselves, hence I documented those but not these.

---

Comments from Reviewable

[dthkao]

Review status: 3 of 7 files reviewed at latest revision, 9 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 75 at r3 (raw file):

     *     </pre>
     *
     * @param advertiseData A JSONObject representing a {@link AdvertiseData} object. E.g.

English super nit: "... object; e.g., ..." (also above).

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 7 files reviewed at latest revision, 9 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 58 at r1 (raw file):

Previously, dthkao (David T.H. Kao) wrote…

+1 to this BT snippet family is slowly veering towards domain expertise.

It does not make sense to force ble to follow wifi and bt scanning.

In wifi and bt, only one scan can happen at one time and there's no settings. Scans for wifi and bt are designed for "single thread", "single consumer", so it makes sense to force them to do one scan at a time.

Whereas in ble, scan is designed to have "multi consumers".

Multiple ble scans can be triggered and going on at the same time; the results from each trigger are supposed to be delivered to the corresponding callback object. This behavior is required in testing. In addition to this, another Rcp bleStartScanWithFilter will be added after this PR, which will kick off ble scans with different filters and expecting different set of scan results to be delivered via the corresponding callback.

In this context, scanAndGetResults and getCachedResults don't really make sense, since both functions assume the "single consumer" model.

---

Comments from Reviewable

[dthkao]

Review status: 3 of 7 files reviewed at latest revision, 9 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 58 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

It does not make sense to force ble to follow wifi and bt scanning.

In wifi and bt, only one scan can happen at one time and there's no settings. Scans for wifi and bt are designed for "single thread", "single consumer", so it makes sense to force them to do one scan at a time.

Whereas in ble, scan is designed to have "multi consumers".

Multiple ble scans can be triggered and going on at the same time; the results from each trigger are supposed to be delivered to the corresponding callback object. This behavior is required in testing. In addition to this, another Rcp bleStartScanWithFilter will be added after this PR, which will kick off ble scans with different filters and expecting different set of scan results to be delivered via the corresponding callback.

In this context, scanAndGetResults and getCachedResults don't really make sense, since both functions assume the "single consumer" model.

What about going the other way, and using a callback model for all scannable things?

As a side question, will every bleStartScan call require an analogous stopScan?

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 7 files reviewed at latest revision, 9 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 58 at r1 (raw file):

Previously, dthkao (David T.H. Kao) wrote…

What about going the other way, and using a callback model for all scannable things?

As a side question, will every bleStartScan call require an analogous stopScan?

I don't see the benefit for that since regular wifi and bt scanning are single-consumer based and there's no reason to make them async atm. If there's a concrete need later we could add that.

Overall just because they are called "scan" doesn't mean we have to force them to follow the same convention. We shouldn't ignore the fundamental difference in these technologies and the requirements for them in tests. We are trying to make test writing easier with MBS, not fool-proof.

If you are toggling wifi in your test, you probably know what wifi is and what turning wifi on/off means. Similarly, if you are using ble in your test, you should probably understand what ble is and how the advertise/scan API works. There's a difference between making something easier to use for tests and making something for users completely ignorant of the tech.

---

Comments from Reviewable

[adorokhine]

Reviewed 1 of 4 files at r2.
Review status: 4 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 38 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Another question, is it sufficient to mark the class, or does every method need to be marked?

Currently RpcMinSdk is only enforced per-method. We can enforce it by class as well; please file an issue to me if you want that.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Right.

I made the one returned through callback here return string for readability since this return value is not meant to be used to start another advertising.

This comment is out of date right? In the latest code were using json.deserialize which means our serialization and deserialization would work correctly with each other right?

Correspondingly, can we add some unit tests to make sure we can deserialize what we serialize?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 69 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

No, this is a base64 string encoded from a regular string like "Hello World".

So it's any base64-encoded string of the user's choice and doesn't have to match anything? Can we mention that?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 70 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

The object it needs is linked

I meant an HTTP link that teaches people about UUID and serviceData.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 75 at r3 (raw file):

Previously, dthkao (David T.H. Kao) wrote…

English super nit: "... object; e.g., ..." (also above).

This seems a little bit pedantic even by our already exacting standards. :)

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 79 at r3 (raw file):

     *          {
     *            "IncludeDeviceName": true,
     *            "ServiceData": [A Base64 encoded string],

Upstream, this seems to be a map. What kind of use-cases does it being a map enable that we're not supporting here?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 58 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

I don't see the benefit for that since regular wifi and bt scanning are single-consumer based and there's no reason to make them async atm. If there's a concrete need later we could add that.

Overall just because they are called "scan" doesn't mean we have to force them to follow the same convention. We shouldn't ignore the fundamental difference in these technologies and the requirements for them in tests. We are trying to make test writing easier with MBS, not fool-proof.

If you are toggling wifi in your test, you probably know what wifi is and what turning wifi on/off means. Similarly, if you are using ble in your test, you should probably understand what ble is and how the advertise/scan API works. There's a difference between making something easier to use for tests and making something for users completely ignorant of the tech.

That sounds good as long as we're happy with reusing this kind of API for all async scan usecases, and the wifi API for all sync scan usecases. Are we? If so is there a good place to document the general scan API approach so we can just point people at it next time they're adding one?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 41 at r3 (raw file):

/** Snippet class exposing Android APIs in WifiManager. */
@TargetApi(Build.VERSION_CODES.LOLLIPOP_MR1)
@RpcMinSdk(Build.VERSION_CODES.LOLLIPOP_MR1)

Currently applies only to methods (see other comment)

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Since this code is just putting key-value pairs in a straightforward way, repeating the output of this in javadoc is just adding maintenance cost for no reason. These functions produce the output that can be easily checked if the user wants to see what the output looks like.

This is different from the deserialization function which requires users to create the input themselves, hence I documented those but not these.

The reason is that right now they'd have to download and read the source code in order to understand what's in the object. Seeing an example in javadoc would be much simpler. In general I'm not convinced that asking users to read code to understand APIs makes for a good API.
(Do we have javadocs on readthedocs.io for this project? If not can we set that up).

---

Comments from Reviewable

[xpconanfan]

Review status: 4 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 38 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Currently RpcMinSdk is only enforced per-method. We can enforce it by class as well; please file an issue to me if you want that.

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

This comment is out of date right? In the latest code were using json.deserialize which means our serialization and deserialization would work correctly with each other right?

Correspondingly, can we add some unit tests to make sure we can deserialize what we serialize?

The serialize method still serialize the consts to strings though. There's no change in behavior from before..

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 70 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

I meant an HTTP link that teaches people about UUID and serviceData.

I don't think we should start linking tutorials on ble 101. The purpose of this doc is not to teach ppl the Bluetooth protocol. If a test writer does not know what UUID means in bluetooth, one should learn that before using this API.

I've updated this section to make it easier to see what's needed here.

Another solution is to make python representation of these complex data classes on the client side, which may be a better way to handle this in the long term than purely relying on documentation. Let's discuss that later?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 39 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Needs @RpcMinSdk?

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 58 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

That sounds good as long as we're happy with reusing this kind of API for all async scan usecases, and the wifi API for all sync scan usecases. Are we? If so is there a good place to document the general scan API approach so we can just point people at it next time they're adding one?

No, I don't think we should create conventions based on the name of the API without regards for how the technologies actually work. It seems unreasonable to assume all things need to follow some kind of convention just because they have the word "scan" in their names.

They are completely different techs with different needs in test writing. In addition to wifi, bt, ble, there's also telephony, or even audio-based nearby scan discovery. We should check and evaluate what makes sense for test writing and the tech's characteristics, instead of pre-determining a convention before we know the use case.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 41 at r3 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Currently applies only to methods (see other comment)

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

The reason is that right now they'd have to download and read the source code in order to understand what's in the object. Seeing an example in javadoc would be much simpler. In general I'm not convinced that asking users to read code to understand APIs makes for a good API.
(Do we have javadocs on readthedocs.io for this project? If not can we set that up).

I think the plan was to bring back @RpcXxx markers that were removed in initial porting so rpc method docs can be easily accessed without src code.
Like google/mobly-snippet-lib#25

Another solution is to make python representation of these complex data classes on the client side.

---

Comments from Reviewable

[xpconanfan]

Review status: 4 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 69 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

So it's any base64-encoded string of the user's choice and doesn't have to match anything? Can we mention that?

Done.

---

Comments from Reviewable

[adorokhine]

Review status: 4 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

The serialize method still serialize the consts to strings though. There's no change in behavior from before..

How are you supposed to get AdvertiseSettings#ADVERTISE_MODE_BALANCED on the python side? I thought we rewrote all constants to strings for this purpose.

---

Comments from Reviewable

[xpconanfan]

Review status: 4 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

How are you supposed to get AdvertiseSettings#ADVERTISE_MODE_BALANCED on the python side? I thought we rewrote all constants to strings for this purpose.

AdvertiseSettings#ADVERTISE_MODE_BALANCED is an int.

In the old world, we (the test writers) duped these enums on the python side to call with, like this

This is not something we can easily solve from the java side right now.

We could provide an accompanying python lib to MBS that contains these stuff...or somehow generate these on the fly from the Python side based on messages passed from java side...

---

Comments from Reviewable

[adorokhine]

Review status: 4 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

AdvertiseSettings#ADVERTISE_MODE_BALANCED is an int.

In the old world, we (the test writers) duped these enums on the python side to call with, like this

This is not something we can easily solve from the java side right now.

We could provide an accompanying python lib to MBS that contains these stuff...or somehow generate these on the fly from the Python side based on messages passed from java side...

Since it seems we don't have a new solution ready to go right now, our previous solution for this problem (stringifying the constants as part of serialization/deserialization) sgtm.

---

Comments from Reviewable

[xpconanfan]

Review status: 4 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Since it seems we don't have a new solution ready to go right now, our previous solution for this problem (stringifying the constants as part of serialization/deserialization) sgtm.

k, the current code follows the same convention as the old (sad) way.

Serialization stringifies the enums for readability on the python side; deserialization takes the original format of the enum, mostly int.

---

Comments from Reviewable

[adorokhine]

Review status: 4 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

k, the current code follows the same convention as the old (sad) way.

Serialization stringifies the enums for readability on the python side; deserialization takes the original format of the enum, mostly int.

That's a convention we have?! If our serialization doesn't match our deserialization that seems like a critical issue since we can't even deserialize the objects that we serialize. Can you imagine the mayhem if proto couldn't deserialize what it serialized?

---

Comments from Reviewable

[xpconanfan]

Review status: 4 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

That's a convention we have?! If our serialization doesn't match our deserialization that seems like a critical issue since we can't even deserialize the objects that we serialize. Can you imagine the mayhem if proto couldn't deserialize what it serialized?

It is what we've been doing bc we never had to pass the deserialized obj back to the java side, that's just not something that happen very often, or in our case, at all in tests.

It is bad, but with the lack of a better solution, it is what has been working for the purpose of testing.

Imagine the mayhem of having to look up the meaning of every single int in these types like AdvertiseSettings returned by the onSuccess callback, before you could make any sense of the test result. Or the annoyance of having to manually translate every int field before you can log them in test.

I'm all for solving this problem. But right now we don't have a good way because this jsonrpc protocol is incomplete due to the lack of object translation. So until then, it is indeed a protocol that demands mayhem in such cases.

---

Comments from Reviewable

[adorokhine]

Review status: 4 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

It is what we've been doing bc we never had to pass the deserialized obj back to the java side, that's just not something that happen very often, or in our case, at all in tests.

It is bad, but with the lack of a better solution, it is what has been working for the purpose of testing.

Imagine the mayhem of having to look up the meaning of every single int in these types like AdvertiseSettings returned by the onSuccess callback, before you could make any sense of the test result. Or the annoyance of having to manually translate every int field before you can log them in test.

I'm all for solving this problem. But right now we don't have a good way because this jsonrpc protocol is incomplete due to the lack of object translation. So until then, it is indeed a protocol that demands mayhem in such cases.

I thought our existing solution for this was to stringify int constants?

---

Comments from Reviewable

[xpconanfan]

Review status: 4 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

I thought our existing solution for this was to stringify int constants?

The existing convention only stringifies on the serializer side; the deserializer side still has the user pass in the actual int value... So it's this weird thing...

---

Comments from Reviewable

[adorokhine]

Review status: 4 of 7 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

The existing convention only stringifies on the serializer side; the deserializer side still has the user pass in the actual int value... So it's this weird thing...

I had no idea this was happening so I apologize for missing it before now. How did we end up with a convention like that? And why would we do something so completely different for input than for output?

Let's change that to be consistent and add unit tests to make sure we can deserialize what we serialize to make sure it doesn't accidentally sneak in again.

No preference for whether we decide to consistently use ints or strings, except that strings has the added convenience that we don't have to ship a set of lookup constants as well.

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 8 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 64 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

I had no idea this was happening so I apologize for missing it before now. How did we end up with a convention like that? And why would we do something so completely different for input than for output?

Let's change that to be consistent and add unit tests to make sure we can deserialize what we serialize to make sure it doesn't accidentally sneak in again.

No preference for whether we decide to consistently use ints or strings, except that strings has the added convenience that we don't have to ship a set of lookup constants as well.

Figured out a better way to handle this, take a look :)

---

Comments from Reviewable

[adorokhine]

Review status: 3 of 8 files reviewed at latest revision, 18 unresolved discussions.

---

build.gradle, line 6 at r4 (raw file):

    }
    dependencies {
        classpath 'com.android.tools.build:gradle:2.3.1'

Bad merge? (seems to have dropped from 2.3.2 to 2.3.1)

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 56 at r4 (raw file):

    private static final EventCache sEventCache = EventCache.getInstance();

    public static RpcEnum bleAdvertiseTxPowerEnums =

Mind sticking this stuff in the deserializer (or new dedicated class for RpcEnum) so we don't have to hunt these enums down around the codebase when we need them (or even worse, accidentally duplicate them)?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 141 at r4 (raw file):

        private final String mCallbackId;
        public static RpcEnum advertiseFailureErrorCodeEnums =
                new RpcEnum.Builder()

Could we keep these in the deserializer (or dedicated class for enums) so we don't have to hunt these down around the codebase?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 61 at r4 (raw file):

    @RpcMinSdk(Build.VERSION_CODES.LOLLIPOP_MR1)
    @AsyncRpc(description = "Start BLE scan.")
    public void bleStartScan(String callbackId) throws BluetoothLeScanSnippetException {

I thought our asyncrpc infrastructure already implicitly handled callback identification? Why do you have to pass your own secondary callback id here?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 73 at r4 (raw file):

    @RpcMinSdk(Build.VERSION_CODES.LOLLIPOP_MR1)
    @Rpc(description = "Stop BLE scan.")
    public void bleStopScan(String id) throws BluetoothLeScanSnippetException {

Is id here the same as callbackId in the other method? Can we give them consistent names?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 100 at r4 (raw file):

            final String nameCallbackType = "CallbackType";
            switch (callbackType) {
                case ScanSettings.CALLBACK_TYPE_ALL_MATCHES:

Should we be using RpcEnum for this?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 130 at r4 (raw file):

            String errorCodeString;
            switch (errorCode) {
                case SCAN_FAILED_ALREADY_STARTED:

RpcEnum?

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonDeserializer.java, line 57 at r4 (raw file):

        if (jsonObject.has("AdvertiseMode")) {
            int mode =
                    BluetoothLeAdvertiserSnippet.bleAdvertiseModeEnums.getIntValue(

We definitely shouldn't be digging stuff out of particular snippet classes in the serializer... let's move this here or into a special class.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonDeserializer.java, line 89 at r4 (raw file):

            JSONObject serviceData = jsonObject.getJSONObject("ServiceData");
            ParcelUuid parcelUuid = ParcelUuid.fromString(serviceData.getString("UUID"));
            byte[] data = Base64.decode(serviceData.getString("Data"), Base64.DEFAULT);

If we're actually decoding this into bytes, why does not have to be base64 in the first place? Why not just send the bytes and let the json encoder handle serialization?

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

I think the plan was to bring back @RpcXxx markers that were removed in initial porting so rpc method docs can be easily accessed without src code.
Like google/mobly-snippet-lib#25

Another solution is to make python representation of these complex data classes on the client side.

Which @RpcXxx marker would have helped you with this?

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 136 at r4 (raw file):

        switch (data.getBondState()) {
            case BluetoothDevice.BOND_NONE:
                result.putString(bondStateFieldName, "BOND_NONE");

RpcEnum?

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 150 at r4 (raw file):

            switch (data.getType()) {
                case BluetoothDevice.DEVICE_TYPE_CLASSIC:
                    result.putString(deviceTypeFieldName, "DEVICE_TYPE_CLASSIC");

RpcEnum?

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 198 at r4 (raw file):

        result.putString(
                "TxPowerLevel",
                BluetoothLeAdvertiserSnippet.bleAdvertiseTxPowerEnums.getStringValue(

Same concern as before, if both places need it it should be its own class.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/RpcEnum.java, line 29 at r4 (raw file):

 * <p>Once built, an RpcEnum object is immutable.
 */
public class RpcEnum {

Can you actually make this an enum? Java enums can handle most of the code in here in a built in way.

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 8 files reviewed at latest revision, 18 unresolved discussions.

---

build.gradle, line 6 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Bad merge? (seems to have dropped from 2.3.2 to 2.3.1)

Android studio wanted me to update to 2.3.2, but 2.3.2 doesn't seem to exist so build failed...

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 56 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Mind sticking this stuff in the deserializer (or new dedicated class for RpcEnum) so we don't have to hunt these enums down around the codebase when we need them (or even worse, accidentally duplicate them)?

These things don't fit into serializer because they are API version specific and they are not methods so @TargetApi can't be applied to them.
I've grouped them in classes based on API version. See if it works.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 141 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Could we keep these in the deserializer (or dedicated class for enums) so we don't have to hunt these down around the codebase?

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 61 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

I thought our asyncrpc infrastructure already implicitly handled callback identification? Why do you have to pass your own secondary callback id here?

I'm not sure what you mean.
The design is the first param is automatically filled by @AsyncRpc, but the function signature still needs to have this passed in.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 73 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Is id here the same as callbackId in the other method? Can we give them consistent names?

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 100 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Should we be using RpcEnum for this?

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 130 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

RpcEnum?

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonDeserializer.java, line 57 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

We definitely shouldn't be digging stuff out of particular snippet classes in the serializer... let's move this here or into a special class.

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonDeserializer.java, line 89 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

If we're actually decoding this into bytes, why does not have to be base64 in the first place? Why not just send the bytes and let the json encoder handle serialization?

What do you mean?
Json could not transport bytes as far as I know.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Which @RpcXxx marker would have helped you with this?

In sl4a, the docs are generated based on these @RpcXx markers.
So @RpcParam, @RpcDefault, @RpcOptional would have played the role in documenting APIs, if they are used as part of the doc generation we do in the new world.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 136 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

RpcEnum?

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 150 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

RpcEnum?

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 198 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Same concern as before, if both places need it it should be its own class.

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/RpcEnum.java, line 29 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Can you actually make this an enum? Java enums can handle most of the code in here in a built in way.

It can do look up with EnumClass.valueOf(String), but I don't see a way to easily find the string value given the int value.
I'd still need to have some kind of switch statement to do the lookup, which is not as nice as the BiMap way.

---

Comments from Reviewable

[adorokhine]

Review status: 3 of 11 files reviewed at latest revision, 18 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/enums/Api18Enums.java, line 10 at r5 (raw file):

/** Class for {@link RpcEnum} objects that represent classes introduced in Android API 18. */
@TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR2)
public class Api18Enums {

Do we have to have a separate enum class for every different API version? Seems kind of hard to read, manage and maintain... how about

public static RpcEnum bluetoothDeviceTypeEnums = getBluetoothDeviceTypeEnums();

private static RpcEnum getBluetoothDeviceTypeEnums() {
  RpcEnum.Builder builder = new RpcEnum.Builder()
      .add(constant1).add(constant2);
  if (Android.Build.SDK_VERSION >= 18) {
      builder.add(constant3).add(constant4);
  }
  if (Android.Build.SDK_VERSION >= 24) {
      builder.add(constant5).add(constant6);
  }
  return builder.build();
}

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 9 files reviewed at latest revision, 18 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/enums/Api18Enums.java, line 10 at r5 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Do we have to have a separate enum class for every different API version? Seems kind of hard to read, manage and maintain... how about

public static RpcEnum bluetoothDeviceTypeEnums = getBluetoothDeviceTypeEnums();

private static RpcEnum getBluetoothDeviceTypeEnums() {
  RpcEnum.Builder builder = new RpcEnum.Builder()
      .add(constant1).add(constant2);
  if (Android.Build.SDK_VERSION >= 18) {
      builder.add(constant3).add(constant4);
  }
  if (Android.Build.SDK_VERSION >= 24) {
      builder.add(constant5).add(constant6);
  }
  return builder.build();
}

Done.

---

Comments from Reviewable

[adorokhine]

Reviewed 2 of 7 files at r6.
Review status: 5 of 9 files reviewed at latest revision, 13 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 70 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

I don't think we should start linking tutorials on ble 101. The purpose of this doc is not to teach ppl the Bluetooth protocol. If a test writer does not know what UUID means in bluetooth, one should learn that before using this API.

I've updated this section to make it easier to see what's needed here.

Another solution is to make python representation of these complex data classes on the client side, which may be a better way to handle this in the long term than purely relying on documentation. Let's discuss that later?

If this is common knowledge, SGTM.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 79 at r3 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Upstream, this seems to be a map. What kind of use-cases does it being a map enable that we're not supporting here?

Bump. Here's the docs:
https://developer.android.com/reference/android/bluetooth/le/AdvertiseData.html

ServiceUuids is a list and ServiceData is a Map from uuid to byte. But in our version, both ServiceUuid and ServiceData are single values only. Why are they containers in Android and what functionality do we lose by making them single values only?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 133 at r6 (raw file):

        public static RpcEnum advertiseFailureErrorCodeEnums =
                new RpcEnum.Builder()
                        .add("ADVERTISE_FAILED_ALREADY_STARTED", ADVERTISE_FAILED_ALREADY_STARTED)

Could you move this enum to Enums?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 73 at r4 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Done.

Just so I understand: how do you call this?

callback = mbs.bleStartScan()
callback.bleStopScan()?

Or something else?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 87 at r6 (raw file):

    @Rpc(description = "Stop a BLE scan.")
    public void bleStopScan(String callbackId) throws BluetoothLeScanSnippetException {
        if (!mScanCallbacks.containsKey(callbackId)) {

Can you call remove() immediately and check the result for null instead of separately calling containsKey and remove?

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/Enums.java, line 11 at r6 (raw file):

/** Class for {@link RpcEnum} objects that represent classes introduced in Android API 5. */
public class Enums {

There's already an Enums class in Guava for working with Java native enums; maybe we could rename this class to RpcEnums to avoid collisions and confusion about its scope?

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/Enums.java, line 12 at r6 (raw file):

/** Class for {@link RpcEnum} objects that represent classes introduced in Android API 5. */
public class Enums {
    static RpcEnum bleAdvertiseModeEnum = buildBleAdvertiseModeEnum();

Are these package private for a reason? Do we not want anything other than mobly.snippet.bundled.utils to be able to see these?

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/Enums.java, line 17 at r6 (raw file):

    public static RpcEnum bleScanResultCallbackTypeEnum = buildBleScanResultCallbackTypeEnum();
    static RpcEnum bluetoothDeviceBondStateEnum = buildBluetoothDeviceBondState();
    static RpcEnum bluetoothDeviceTypeEnum = buildBluetoothDeviceTypeEnum();

These static initializers will crash on old devices; discussed offline.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonDeserializer.java, line 89 at r4 (raw file):

Previously, xpconanfan (Ang Li) wrote…

What do you mean?
Json could not transport bytes as far as I know.

Ah, TIL! What a weird limitation. Oh well.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

In sl4a, the docs are generated based on these @RpcXx markers.
So @RpcParam, @RpcDefault, @RpcOptional would have played the role in documenting APIs, if they are used as part of the doc generation we do in the new world.

Which particular @RpcXxx marker would be helpful for telling the user what's in this complex data class without src access?

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/RpcEnum.java, line 29 at r4 (raw file):

Previously, xpconanfan (Ang Li) wrote…

It can do look up with EnumClass.valueOf(String), but I don't see a way to easily find the string value given the int value.
I'd still need to have some kind of switch statement to do the lookup, which is not as nice as the BiMap way.

That's fair, sounds good. Would it be worth it to compute the reverse mapping during build() time so we don't have to reverse the map for every lookup?

---

Comments from Reviewable

[xpconanfan]

Review status: 5 of 9 files reviewed at latest revision, 13 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 79 at r3 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Bump. Here's the docs:
https://developer.android.com/reference/android/bluetooth/le/AdvertiseData.html

ServiceUuids is a list and ServiceData is a Map from uuid to byte. But in our version, both ServiceUuid and ServiceData are single values only. Why are they containers in Android and what functionality do we lose by making them single values only?

good catch. this structure is more complicated than I expected...
Will need to change this

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 133 at r6 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Could you move this enum to Enums?

This enum only makes sense in the context of this class though.
Per the minimal scope rule, shouldn't it be here instead?

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 73 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Just so I understand: how do you call this?

callback = mbs.bleStartScan()
callback.bleStopScan()?

Or something else?

callback = mbs.bleStartScan()
mbs.bleStartScan(callback.callback_id)

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/Enums.java, line 12 at r6 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Are these package private for a reason? Do we not want anything other than mobly.snippet.bundled.utils to be able to see these?

there's no usage outside of this package right now, so android studio is not happy when i made them public...

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Which particular @RpcXxx marker would be helpful for telling the user what's in this complex data class without src access?

We could potentially jam the structure example into the description of @RpcParam, which then could be printed out in help().

(I think in the long run, auto generating container classes on the client side may be the way to go)

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/RpcEnum.java, line 29 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

That's fair, sounds good. Would it be worth it to compute the reverse mapping during build() time so we don't have to reverse the map for every lookup?

ImmutableBiMap already does that, this is the benefit of using BiMap.

---

Comments from Reviewable

[xpconanfan]

Review status: 5 of 9 files reviewed at latest revision, 13 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 87 at r6 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Can you call remove() immediately and check the result for null instead of separately calling containsKey and remove?

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/Enums.java, line 11 at r6 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

There's already an Enums class in Guava for working with Java native enums; maybe we could rename this class to RpcEnums to avoid collisions and confusion about its scope?

Done.

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 10 files reviewed at latest revision, 13 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/MbsEnums.java, line 17 at r6 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

These static initializers will crash on old devices; discussed offline.

Done.

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 10 files reviewed at latest revision, 13 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 79 at r3 (raw file):

Previously, xpconanfan (Ang Li) wrote…

good catch. this structure is more complicated than I expected...
Will need to change this

Done

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 10 files reviewed at latest revision, 13 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

We could potentially jam the structure example into the description of @RpcParam, which then could be printed out in help().

(I think in the long run, auto generating container classes on the client side may be the way to go)

The thing is, some usages are inherently complicated, and when one is writing a test, it is easier to just look up the source or some other form of documentation than trying to figure out through help(). So maybe generating a site for these Rpcs is the better way in the long run.

---

Comments from Reviewable

[adorokhine]

Review status: 3 of 10 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 133 at r6 (raw file):

Previously, xpconanfan (Ang Li) wrote…

This enum only makes sense in the context of this class though.
Per the minimal scope rule, shouldn't it be here instead?

If we don't expect AdvertiseCallback to ever be created anywhere else, including from user snippets, sgtm. Otherwise it's a little awkward to hunt down serialization and deserialization code across the whole codebase; would be nice to keep it together.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 92 at r7 (raw file):

     *                      }
     *                ]
     *            "ServiceUuid": (A string representation of {@link ParcelUuid})

ServiceUuid seems to be a list in the upstream structure, but is a scalar here.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 73 at r4 (raw file):

Previously, xpconanfan (Ang Li) wrote…

callback = mbs.bleStartScan()
mbs.bleStartScan(callback.callback_id)

sgtm for this PR. Longer term is there any way for us to make callback.bleStopScan() possible? Would be nice...

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

The thing is, some usages are inherently complicated, and when one is writing a test, it is easier to just look up the source or some other form of documentation than trying to figure out through help(). So maybe generating a site for these Rpcs is the better way in the long run.

So it sounds like the question is between whether to put this into javadoc or @RpcParam? In that case, can you put it into javadoc for now (since that's the only thing that works) and we can consider moving it to @RpcParam once it is supported?

You might also consider javadoc to be a generated site containing these Rpcs. :)

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/MbsEnums.java, line 12 at r6 (raw file):

Previously, xpconanfan (Ang Li) wrote…

there's no usage outside of this package right now, so android studio is not happy when i made them public...

By "not happy" do you mean it made it grey? That's fine, all of our RPCs are like that too.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/MbsEnums.java, line 31 at r7 (raw file):

    @TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR2)
    private static RpcEnum buildBluetoothDeviceTypeEnum() {
        Utils.assertMinSdk(Build.VERSION_CODES.JELLY_BEAN_MR2);

This will crash as soon as the initializer runs. Needs to be in an if statement.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/RpcEnum.java, line 53 at r7 (raw file):

     */
    public String getString(int enumInt) {
        return mEnums.inverse().get(enumInt);

Would it be wise to throw here instead of returning null?

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 10 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 133 at r6 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

If we don't expect AdvertiseCallback to ever be created anywhere else, including from user snippets, sgtm. Otherwise it's a little awkward to hunt down serialization and deserialization code across the whole codebase; would be nice to keep it together.

Yeah, the ble advertise/scan APIs are pretty self-contained.
So this is the only place AdvertiseCallback is created atm.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeAdvertiserSnippet.java, line 92 at r7 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

ServiceUuid seems to be a list in the upstream structure, but is a scalar here.

Forgot to update this, done.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 73 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

sgtm for this PR. Longer term is there any way for us to make callback.bleStopScan() possible? Would be nice...

If optional param were available, we could have a special case where mbs.bleStartScan() works if and only if there is one cached callback obj.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/JsonSerializer.java, line 193 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

So it sounds like the question is between whether to put this into javadoc or @RpcParam? In that case, can you put it into javadoc for now (since that's the only thing that works) and we can consider moving it to @RpcParam once it is supported?

You might also consider javadoc to be a generated site containing these Rpcs. :)

yeah, i wonder why sl4a didn't do javadoc and went with documenting in annotations...
Added in java doc.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/MbsEnums.java, line 12 at r6 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

By "not happy" do you mean it made it grey? That's fine, all of our RPCs are like that too.

No, it was highlighted yellowish with a warning.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/RpcEnum.java, line 53 at r7 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Would it be wise to throw here instead of returning null?

if new fields were added to an enum in new API versions, it would be nice if we simply return null so the code that parses the enum doesn't have to worry about exceptions. And the test can fail as needed if null is not an expected value.

(also this would be more consistent with the RpcEnum being null on lower API versions too?)

---

Comments from Reviewable

[adorokhine]

Review status: 3 of 10 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/RpcEnum.java, line 53 at r7 (raw file):

Previously, xpconanfan (Ang Li) wrote…

if new fields were added to an enum in new API versions, it would be nice if we simply return null so the code that parses the enum doesn't have to worry about exceptions. And the test can fail as needed if null is not an expected value.

(also this would be more consistent with the RpcEnum being null on lower API versions too?)

That's not really consistent for how it would behave without serialization though... if you compiled new code and tried to run it on an old sdk, it wouldn't return null when referencing those constants, it would crash with NoSuchFieldException. Maybe it would be nice to maintain the same behaviour to avoid violating existing API expectations?

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 10 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/MbsEnums.java, line 31 at r7 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

This will crash as soon as the initializer runs. Needs to be in an if statement.

Done.

---

Comments from Reviewable

[xpconanfan]

Review status: 3 of 10 files reviewed at latest revision, 10 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/RpcEnum.java, line 53 at r7 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

That's not really consistent for how it would behave without serialization though... if you compiled new code and tried to run it on an old sdk, it wouldn't return null when referencing those constants, it would crash with NoSuchFieldException. Maybe it would be nice to maintain the same behaviour to avoid violating existing API expectations?

Added extra checks.

---

Comments from Reviewable

[adorokhine]

Reviewed 4 of 7 files at r7, 4 of 4 files at r8.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 73 at r4 (raw file):

Previously, xpconanfan (Ang Li) wrote…

If optional param were available, we could have a special case where mbs.bleStartScan() works if and only if there is one cached callback obj.

Optional param doesn't seem like the right fix here; would be nice if you could call more methods on the callback than just eventWaitAndGet. How would you use optional param for this? The class would cache the last seen callback ID? Lots of manual management for that...

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/MbsEnums.java, line 15 at r8 (raw file):

    static RpcEnum bleAdvertiseTxPowerEnum = buildBleAdvertiseTxPowerEnum();
    public static RpcEnum bleScanFailedErrorCodeEnum = buildBleScanFailedErrorCodeEnum();
    public static RpcEnum bleScanResultCallbackTypeEnum = buildBleScanResultCallbackTypeEnum();

Assuming these enums are immutable once constructed, seems like these should all be CAPS_WITH_UNDERSCORES per our convention?
https://source.android.com/source/code-style#follow-field-naming-conventions

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/MbsEnums.java, line 28 at r8 (raw file):

    }

    @TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR2)

Should probably strip off the @TargetApi annotations from all of these so the IDE doesn't hide situations where we've accidentally made mistakes in our if statements.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/RpcEnum.java, line 47 at r8 (raw file):

    public int getInt(String enumString) {
        if (Build.VERSION.SDK_INT < minSdk) {
            throw new NoSuchFieldError(

This error message will be misleading when different constants in this RpcEnum were added at different API levels. I'd just leave the min sdk out and throw NoSuchFieldError if it's not in the map.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/RpcEnum.java, line 102 at r8 (raw file):

         * @return
         */
        public Builder setMinSdk(int minSdk) {

Doesn't work properly when different fields in this enum were added in different API levels. Let's just simplify and remove it.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/Utils.java, line 36 at r8 (raw file):

     * @param minSdkVersion
     */
    public static void assertMinSdk(int minSdkVersion) {

Doesn't seem to be used anywhere.

---

Comments from Reviewable

[xpconanfan]

Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

src/main/java/com/google/android/mobly/snippet/bundled/BluetoothLeScannerSnippet.java, line 73 at r4 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Optional param doesn't seem like the right fix here; would be nice if you could call more methods on the callback than just eventWaitAndGet. How would you use optional param for this? The class would cache the last seen callback ID? Lots of manual management for that...

The class already cache all the callback Ids and their Callback objects.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/MbsEnums.java, line 15 at r8 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Assuming these enums are immutable once constructed, seems like these should all be CAPS_WITH_UNDERSCORES per our convention?
https://source.android.com/source/code-style#follow-field-naming-conventions

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/MbsEnums.java, line 28 at r8 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Should probably strip off the @TargetApi annotations from all of these so the IDE doesn't hide situations where we've accidentally made mistakes in our if statements.

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/RpcEnum.java, line 47 at r8 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

This error message will be misleading when different constants in this RpcEnum were added at different API levels. I'd just leave the min sdk out and throw NoSuchFieldError if it's not in the map.

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/RpcEnum.java, line 102 at r8 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Doesn't work properly when different fields in this enum were added in different API levels. Let's just simplify and remove it.

Done.

---

src/main/java/com/google/android/mobly/snippet/bundled/utils/Utils.java, line 36 at r8 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Doesn't seem to be used anywhere.

Done.

---

Comments from Reviewable

[adorokhine]

Looks good, but please add some tests for serialization and deserialization consistency as a follow-up. :)

---

Reviewed 1 of 7 files at r7, 7 of 7 files at r9.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[xpconanfan]

Thanks! :D