WIP: Use "D-Bus over TCP/IP over Ethernet over BLE" for communicating with the watch #216
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Currently working on the |
dodoradio
reviewed
May 3, 2024
I-asked
force-pushed
the
libslirp
branch
2 times, most recently
from
acffbbd
to
5c85a4c
Compare
|
I believe the 14-bytes Ethernet frame can be shaved down to 1 byte for point-to-point (a pair of nibbles for source + destination MAC) |
I-asked
commented
May 4, 2024
| import java.util.Objects | ||
|
|
||
| class NotificationService(private val mCtx: Context, private val connectionProvider: IDBusConnectionProvider) : INotificationHandler, DBusSigHandler<NotificationClosed?> { | ||
| private val mapping: BiMap<String, UInt32> = HashBiMap.create() |
There was a problem hiding this comment.
mapping should probably be persisted in case the application is closed…
| # build script scope). | ||
| project("sync") | ||
|
|
||
| add_subdirectory(libslirp) |
There was a problem hiding this comment.
Let's ship libslirp as a git submodule so we don't have to copy paste files across repositories and we benefit from libslirp's commit log etc...
| JNIEXPORT void JNICALL Java_org_asteroidos_sync_connectivity_SlirpService_finalizeNative | ||
| (JNIEnv* env, jobject thisObject) { | ||
|
|
||
| auto mySlirp = GET_MYSLIRP(env, thisObject); |
| @@ -142,7 +162,7 @@ public final boolean isRequiredServiceSupported(@NonNull final BluetoothGatt gat | |||
| @Override | |||
| protected final void initialize() { | |||
| beginAtomicRequestQueue() | |||
| .add(requestMtu(256) // Remember, GATT needs 3 bytes extra. This will allow packet size of 244 bytes. | |||
| .add(requestMtu(517) | |||
There was a problem hiding this comment.
Any specific reason for that MTU ? :)
| @@ -58,4 +58,9 @@ public class AsteroidUUIDS { | |||
| public static final UUID NOTIFICATION_SERVICE_UUID = UUID.fromString("00009071-0000-0000-0000-00A57E401D05"); | |||
| public static final UUID NOTIFICATION_UPDATE_CHAR = UUID.fromString("00009001-0000-0000-0000-00A57E401D05"); | |||
| public static final UUID NOTIFICATION_FEEDBACK_CHAR = UUID.fromString("00009002-0000-0000-0000-00A57E401D05"); | |||
|
|
|||
| // SlirpService | |||
There was a problem hiding this comment.
We shouldn't call the BLE standard "slirp service" because technically it's agnostic to the existence of slirp. Slirp is an implementation detail of the Android companion where we do not have the privileges to inject raw frames.
|
|
||
| // SlirpService | ||
| public static final UUID SLIRP_SERVICE_UUID = UUID.fromString("0000A071-0000-0000-0000-00A57E401D05"); | ||
| public static final UUID SLIRP_OUTGOING_CHAR = UUID.fromString("0000A001-0000-0000-0000-00A57E401D05"); |
There was a problem hiding this comment.
Let's keep the naming of characteristics consistent with tap2ble so they are easier to map. Outgoing and Incoming are similarly confusing as RX and TX, let's just stick to one
| import java.util.Map; | ||
| import java.util.Objects; | ||
|
|
||
| public class DBusNotificationService implements INotificationHandler, DBusSigHandler<Notifications.NotificationClosed> { |
There was a problem hiding this comment.
Ideally, to keep this code maintainable there should only be one NotificationService and it should choose between using DBus or the legacy BLE profile based on what is supported. It will make it easier to share code and to evolve this over time
| if (notificationOption == NotificationPreferences.NotificationOption.NO_NOTIFICATIONS) | ||
| return; | ||
|
|
||
| int id = intent.getIntExtra("id", 0); |
There was a problem hiding this comment.
I guess this should be key now. One more reason to share code between the two implementations
| versionCode = 29 | ||
| versionName = "0.29" | ||
| ndk.abiFilters.clear() | ||
| ndk.abiFilters.add("arm64-v8a") | ||
| ndk.abiFilters.add("armeabi-v7a") | ||
| ndk.abiFilters.add("x86") | ||
| ndk.abiFilters.add("x86_64") |
There was a problem hiding this comment.
Uh, now these are re-added and cgi is deleted. That's a lot of back and forths to review. I realize the PR is marked as draft, it would be nice to clean up this backlog. I'll stop reviewing here until then ;)
| @@ -39,7 +42,7 @@ android { | |||
| srcDir("src/main/lib/powerampapi/poweramp_api_lib/res/") | |||
| } | |||
| jniLibs { | |||
| srcDir("/work/android-root/lib") | |||
| srcDir("/tmp/android-root/lib") | |||
There was a problem hiding this comment.
I see, this is supposed to be the outcome of the bash script now. Maybe it doesn't hurt to add a comment above to describe where this is coming from
| @@ -107,7 +137,7 @@ public SlirpService(Context ctx, IAsteroidDevice device) { | |||
| long read = vdeRecv(rx, 0, mtu - 3); | |||
| assert read <= (mtu - 3); | |||
| if (read > 0) { | |||
| Log.d("SlirpService", "Received " + read + " bytes"); | |||
| // Log.d("SlirpService", "Received " + read + " bytes"); | |||
There was a problem hiding this comment.
No commented code, let's just drop it
|
@I-asked I've been testing your PRs, great work! But I'm wondering what kind of performance we can expect? Currently pinging seems to take some time: I've tested this with |
Oh my… that is very weird… This should not happen… I can't work on this right now, but the network connection worked very well last time I checked… |
Indeed very interesting! I've now tried on another phone it where it works much better: For reference the phone showing terrible results is a Samsung Galaxy S23 Ultra (Android 14). The phone with proper looking results is the a Xiaomi Mi5 (LineageOS with Android 11). The difference in performance is enough to actually successfully connect to ssh (via the phone) |
|
Perhaps the Samsung has some weird "battery saving" feature you could try disabling? |
|
We already prompt to disable battery saving. Maybe this isn't enough on Samsung. Or maybe it's something with newer Android devices? What phone did you test this with? |
|
Is the Samsung connected to a wi-fi network? Could you try those two phones both over cellular and wi-fi? |
|
I last tested on my Xperia 1 IV which was running A13. |
|
There appears to be no difference when I use WiFi or cellular. I've slightly altered your version before the dbus library change and the additional dbus services as there have been a some SDK updates which result in runtime errors. This version is available here: https://github.com/MagneFire/AsteroidOSSync/commits/slirp-2/ Attached are the logs for both phones: Interestingly D-Bus connection seems to also fail on the Samsung phone. |
|
Not entirely sure why but it seems that on the Samsung phone the SlirpService freezes in one of the Replacing the Changes are available here: https://github.com/MagneFire/AsteroidOSSync/commits/slirp-2/ |
|
Hmm, after more testing it seems to be related to MTU. The app requests 247 (which is initially also used for libslirp). If the requested MTU is 515 it all seems to work fine, but in this case the libslirp re-initialization does not occur. Forcing a libslirp re-initialization (by overriding
Tested on the known working phone. It seems that the MTU doesn't change once connected, it stays at 247. To find out if the issues is with MTU or libslirp re-initialization I've changed the default MTU to 515 and request a different MTU (247) upon second connect, which results in a libslirp re-initialization. Letting libslirp re-initialize results in the same issue as with the other phone (ping not working/very large ttl). Communication works fine when the libslirp re-initialization isn't executed (Thus using MTU 247 with a BLE MTU of 515). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi @FlorentRevest!
What a coincidence indeed…
(We came up with the same idea within the span of two weeks 😳)
Sooo this is my attempt at replacing ATT-based comms with something more streamlined (D-Bus). ✌️
The session bus is exported over TCP to the companion for seamless event transfer between the two devices.
A simplified version of asteroid-tap2ble forwards packets from a TAP interface over BLE. (since the link is relatively slow [26 kbps avg.], we should stick to lightweight protocols, so D-Bus-over-TCP, 9p, etc.; SSH is
sadlyway too bulky for this connection.)The packets should then be handled on the companion in an appropriate manner.
To achieve this on Android, I am using
libslirp, which allows me to create a virtual network adapter that translates Ethernet packets into unprivileged calls.To do
diodon the watch)ScreenshotService)MediaServiceNotificationService(also made it bi-directional 😉; now it's just missing action buttons 😶🌫️)ScreenshotServiceTimeService(could be scrapped in favor of NTP)WeatherService(could be scrapped in favor of the community daemon)