Add bluetooth scanning #686
Conversation
Feel free to make changes!
There was a problem hiding this comment.
I think this is not quite the right API to expose. IIUC what you've got is:
start- begin collecting discovered devices in the backgroundinfo- return a time-ordered but untimestamped list of every device-sighting sincestart(ed: corrected this after the initial comment)stop- stop collecting devices and return that list one last time
Two things feel wrong about this to me:
- This sorta kinda resembles how bluetooth discovery UIs work, but you're building a programmatic interface. You don't need to keep history inside this API because it's going to be consumed by code, not by human eyes that might look away for a second and miss something.
- You don't have any last-seen tracking, which means that if I wait to scan for more than a handful of seconds, I have no way to distinguish between devices I just saw and devices I saw once, right after
start, and never again.
I spent a day chewing on this and imo this should be:
start- as abovedumporpoll- return devices seen since the lastdumporpoll(infoisn't a good verb whether you change the semantics or not)stop- just stop scanning, no devices returned
What do you think?
| private static BluetoothAdapter adapter; | ||
| private static boolean scanning = false; | ||
|
|
||
| private static final BroadcastReceiver receiver = new BroadcastReceiver() { |
There was a problem hiding this comment.
Please give this a better name, even though it's the only BroadcastReceiver in sight. device_found_receiver?
|
|
||
| public class BluetoothScanAPI { | ||
|
|
||
| private static final ArrayList<BluetoothDevice> devices = new ArrayList<>(); |
There was a problem hiding this comment.
I think it's strictly necessary to track and return last-seen timestamps. It also looks like the scan API will trigger multiple times for the same devices as it's repeatedly discovered, and a longer scan with the code would result in multiple copies of the same device in this array and the returned output.
This seems like it should be HashMap<BluetoothDevice, long>.
| public void onReceive(Context context, Intent intent) { | ||
| if (Objects.equals(intent.getAction(), BluetoothDevice.ACTION_FOUND)) { | ||
| BluetoothDevice device = intent.getParcelableExtra(BluetoothDevice.EXTRA_DEVICE); | ||
| if (device != null) { |
There was a problem hiding this comment.
Any reason we'd expect an ACTION_FOUND with device == null? Add a comment if so, log an error saying "that shouldn't happen it but" did if not.
|
|
||
| } | ||
|
|
||
| private static void printList(JsonWriter out, boolean clear) { |
There was a problem hiding this comment.
nit: This method doesn't actually print. writeDiscoveredDevices, something like that?
(If you accept my suggested API change, you should remove boolean clear and clear unconditionally after calling this in the dump/poll handler.)
| } | ||
|
|
||
| private static boolean start(Context context) { | ||
| if (scanning) return false; |
There was a problem hiding this comment.
mega nit: { return false; }, and same in stop(), please. :)
| out.name("error").value(false); | ||
| } else { | ||
| out.name("error").value(true); | ||
| out.name("reason").value("Already running!"); |
There was a problem hiding this comment.
Instead of having two keys, how about error only being present if it has an error message and returning an empty object on success? There's approximate precedent for that in the NFC API.
This was my initial thought process, but i realized that force quitting the process would not stop discovery. I do not have time atm, but i would be happy if someone made it automatically discover and keep the process running (so that info would be removed, so would the list). |
|
I don't follow. My suggestion was purely about the ergonomics and usefulness of the API; the fact that discovery would continue if you force-stopped the main termux process (I think that's what you meant) seems the same either way. |
|
If the api was written in java, this would not be the case. Currently its getting through 2 languages, so its pretty hard to manage. If the api used jni, you could share types and objects (but it would be harder for a normal user to implement). If we want to include more features, we would probably need an api rework. |
|
Right, but that's not an option. I'm saying that I think you can and should make those changes within the constraints of a normal |
Feel free to review and make changes!
termux-api-package pull request: termux/termux-api-package#187