jonasoreland / runnerup

A open source run tracker
GNU General Public License v3.0
752 stars 275 forks source link

Write some hrdevice javadoc #207

Closed xbao closed 7 years ago

xbao commented 9 years ago

Howdy. I'm writing up some JavaDoc for the main classes that client code of the hrdevice module uses. Pretty much done, just one question if you don't mind.

What's the purpose of HRProvider#isBondingDevice()?

To refresh your mind:

It returns true for PolarHRM, StHRMv1 and false for AntPluse, AndroidBLEHRProvider, SamsungBLEHRProvider.

If it's true, a button labelled Pairing is available that launches the Bluetooth settings menu

jonasoreland commented 9 years ago

it is essentially the same as Bluetooth 2.0, for those you need to "pair" the phone with the HR using the system dialog. and there are android API to list them all devices that has been paired (bonded)

sounds reasonable ? (it has been a while since i looked at that)

/Jonas

On Tue, Mar 3, 2015 at 1:15 AM, Xiao Bao Clark notifications@github.com wrote:

Howdy. I'm writing up some JavaDoc for the main classes that client code of the hrdevice module uses. Pretty much done, just one question if you don't mind.

What's the purpose of HRProvider#isBondingDevice()? It returns true for PolarHRM, StHRMv1 and false for AntPluse, AndroidBLEHRProvider, SamsungBLEHRProvider.

— Reply to this email directly or view it on GitHub https://github.com/jonasoreland/runnerup/issues/207.

xbao commented 9 years ago

Oh right, yeah that makes sense. Although it would also apply to BLE right? At least, that's the case for my one, I have a Polar H7 and it needs to be paired to show up in the scan.

I'll change AndroidBLEHRProvider and SamsungBLEHRProvider to return true if you agree?

xbao commented 9 years ago

Hmm, I just did a search and it seems some BLE devices don't require pairing:

http://electronics.stackexchange.com/questions/47273/is-bluetooth-communication-possible-without-pairing

I would still suggest we have the pairing button there, so that users who do require it can use it.

jonasoreland commented 9 years ago

is that the only current use?

if (isBonding()) addButtonFor SystemDialog();

/Jonas

On Wed, Mar 4, 2015 at 12:09 AM, Xiao Bao Clark notifications@github.com wrote:

Hmm, I just did a search and it seems some BLE devices don't require pairing:

http://electronics.stackexchange.com/questions/47273/is-bluetooth-communication-possible-without-pairing

I would still suggest we have the pairing button there, so that users who do require it can use it.

— Reply to this email directly or view it on GitHub https://github.com/jonasoreland/runnerup/issues/207#issuecomment-77059905 .

xbao commented 9 years ago

Yes that's correct. Here's the complete code snippet:

    if (hrProvider.isBondingDevice()) {
        builder.setNeutralButton("Pairing", new DialogInterface.OnClickListener() {

            @Override
            public void onClick(DialogInterface dialog, int which) {
                dialog.cancel();
                Intent i = new Intent(Settings.ACTION_BLUETOOTH_SETTINGS);
                startActivityForResult(i, 123);
            }
        });
    }

If you're concerned about it being erroneously used later for something else, I could rename it to canRequirePairing()?

jonasoreland commented 9 years ago

naaa...no need...go ahead with your plans...

On Thu, Mar 5, 2015 at 3:14 AM, Xiao Bao Clark notifications@github.com wrote:

Yes that's correct. Here's the complete code snippet:

if (hrProvider.isBondingDevice()) {
    builder.setNeutralButton("Pairing", new DialogInterface.OnClickListener() {

        @Override
        public void onClick(DialogInterface dialog, int which) {
            dialog.cancel();
            Intent i = new Intent(Settings.ACTION_BLUETOOTH_SETTINGS);
            startActivityForResult(i, 123);
        }
    });
}

If you're concerned about it being erroneously used later for something else, I could rename it to canRequirePairing()?

— Reply to this email directly or view it on GitHub https://github.com/jonasoreland/runnerup/issues/207#issuecomment-77293247 .

gerhardol commented 7 years ago

Changes merged