D7652: Better broadcast/neighbor networking

[Daniel Tang]

daniel.z.tg added a comment.


  The 3 codepaths can be found in `NetworkHelper`. They were the following (but now changed):
  
  - "255.255.255.255"
  - Broadcast addresses from `NetworkInterface`s. A list of these can be generated on Linux Java 8 using:
  
    Collections.list(NetworkInterface.getNetworkInterfaces()).stream()
    		.filter(x -> isInterfaceNameCompatible(x.getName()))
    		.map(NetworkInterface::getInterfaceAddresses).flatMap(List::stream)
    		.map(InterfaceAddress::getBroadcast).filter(Objects::nonNull)
    		.collect(Collectors.toList());
  
  - Network neighbors, (from ARP, or other places). On Android/Linux shell:
  
    $ cat /proc/net/arp
    $ ip n
    $ arp
  
  The ARP codepath has been removed in the latest change, and exactly one of the other two can be active at a time. Previously, enabling multiple codepaths would cause things to randomly and silently break.
  
  Also, getting rid of `isInterfaceNameCompatible()` does not have any effect on my phone. My mobile data interface, `rmnet_data0`, does not have a broadcast address, so it does not get included. My virtual interfaces like loopback don't have broadcast addresses either. Is this guaranteed for all devices? If so, we can fully remove `isInterfaceNameCompatible()`. But if this is not the case, removing it might cause a routing loop if things get sent to loopback interfaces.

INLINE COMMENTS

> albertvaka wrote in BackgroundService.java:58-65
> Why making them static?

1. They were already effectively static, since only 1 instance of `BackgroundService` gets created, and that instance is stored is a static field, and all users use that static object to access instance fields & methods.
2. `BackgroundService` needed to be called everywhere, and to do this either the instance needed to be passed around, or `getInstance()` needed to be called. It also looks cleaner, and is more efficient to avoid calling `getInstace()` every time.
3. `KdeConnectBroadcastReceiver` needs `BackgroundService.refreshNetwork()`. `refreshNetwork()` in turn needs to use the static locks. It also needs to use `onNetworkChange()`. Mixing instance and static when they could be the same looks wrong, so I changed `onNetworkChange()` to static. It needed `linkProviders` to be static, so I changed that as well. For consistency, I changed the other collections, `devices` and `discoverymodeAcquisitions` to static, along with their accessor methods. In the end, nothing except the initialization-related methods needed the `BackgroundService` instance anymore, so I stopped passing it around in `onServiceStart()`.

They don't need to be static, but it just simplifies things to make them that way. They will still work if changed back to instance fields/methods and chained with `getInstance()`.

> albertvaka wrote in BackgroundService.java:248
> Why is it costly?

It's not anymore (only 5ms). It used to be costly earlier, when a lot of broadcast receivers were added, and debug messages were printed everywhere.

Now, the filtering is only used to help with connection problems. If the network is refreshed twice before a neighbor can respond, the connection to that neighbor might require another refresh to get working. This is made worse by more broadcast receivers being added, and them actually refreshing the network every time it changes (before, they didn't always get registered).

> albertvaka wrote in NetworkHelper.java:125-127
> How do we know every USB tethering will start with 'rndis', etc? Is this documented anywhere?

Yes, it is documented, and is standard in AOSP.

https://github.com/android/platform_frameworks_base/blob/b09cf3fbf1c7e340bc06e8aba06461d4a0bae457/core/java/android/hardware/usb/UsbManager.java#L64-L65
https://github.com/android/platform_frameworks_base/blob/b09cf3fbf1c7e340bc06e8aba06461d4a0bae457/services/core/java/com/android/server/connectivity/Tethering.java#L194
https://stackoverflow.com/questions/7509924/detect-usb-tethering-on-android/43916414#43916414

> albertvaka wrote in DeviceFragment.java:341
> Unrelated change?

I don't know the side effects of creating the plugin list, so I just skip it when it's invisible and all that should show is an error message. Probably doesn't make a difference.

REPOSITORY
  R225 KDE Connect - Android application

REVISION DETAIL
  https://phabricator.kde.org/D7652

To: daniel.z.tg, #kde_connect
Cc: albertvaka, daniel.z.tg, jeanv, tfella, aboudhar, seebauer, bugzy, progwolff, MayeulC, menasshock, ach, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20170904/10b09dea/attachment.html>