feat: fixup API usage to make ios-deploy work also with wifi connections #385
Conversation
|
Any chance of seeing this merged soon? |
|
This also fixes problems I'm having with launching the debugServer on iOS13 (not over Wifi) |
Nice to hear. Have not gotten around on testing anything on ios 13 yet. Sadly it seems that noone is actually looking at this. |
|
As a general usage note should anyone wish to use this particular change before it is pulled in, you can do so by running: |
|
The WiFi detect feature doesn't seem to work on the corporate network, will have to try at home. Tasks:
|
|
When I do this, it detects fine (USB): But when I try to deploy an app, it gets stuck: It is fine if I set a timeout value: |
|
Did some tests with
So based on this evidence, I think it's clear this PR has introduced a bug that needs to be resolved before it can be merged. |
|
If you omit the With versions |
src/ios-deploy/ios-deploy.m
Outdated
| @@ -1726,7 +1698,7 @@ void handle_device(AMDeviceRef device) { | |||
| void device_callback(struct am_device_notification_callback_info *info, void *arg) { | |||
| switch (info->msg) { | |||
| case ADNCI_MSG_CONNECTED: | |||
| if(device_id != NULL || !debug || AMDeviceGetInterfaceType(info->dev) != 2) { | |||
| if(device_id != NULL || !debug || detect_only) { | |||
There was a problem hiding this comment.
This line causes the breakage, if you revert the change it works as before (backwards compatible)
There was a problem hiding this comment.
If this is reverted of course it will be problematic since WiFi devices won't be detected (the 2 is for WiFi devices). So this if statement needs to be reworked
There was a problem hiding this comment.
Found the original PR that added that added that code block which explains the logic: #48
There was a problem hiding this comment.
Yeah, I misunderstood the if statement. It was crafted to detect wifi devices only if we don't have debug set or device_id is specified.
When you specify the timeout, the best_device_match is used after the timeout and that is why it works.
As the AMDeviceGetInterfaceType(info->dev) != 2 was always true for USB devices, I think we can just remove the if and also the best_device_match handling. It seems that old behaviour was to use USB device and fall back to Wifi if no USB device was found. (Even if no-wifi is specified).
I propose rewriting the device_callback to:
void device_callback(struct am_device_notification_callback_info *info, void *arg) {
switch (info->msg) {
case ADNCI_MSG_CONNECTED:
if (no_wifi && AMDeviceGetInterfaceType(info->dev) == 2)
{
NSLogVerbose(@"Skipping wifi device (type: %d)", AMDeviceGetInterfaceType(info->dev));
}
else
{
NSLogVerbose(@"Handling device type: %d", AMDeviceGetInterfaceType(info->dev));
handle_device(info->dev);
}
default:
break;
}
}
There was a problem hiding this comment.
I think that the logic is not needed anymore as we use the device over wifi now as needed. It would be nice to favor USB over Wifi, but I'm not sure that we can implement that without having some delay to collect devices and then choose the best.
There was a problem hiding this comment.
I may be misunderstanding, but it seems that in your variant the if(_timeout ..) part would cause the last device detected to be chosen and only after the timeout has expired. As now we dont have special handling for Wifi Here, I think that the best_device_match does not have any usable logic left in it currently, but the timeout part breaks actual timeout usage.
Also, why we dont wanto deploy over wifi if debug is present? It's my understanding that this PR fixes the reasoning why it was there.
There was a problem hiding this comment.
Regarding the no debug over wifi, I was relying on the information in PR #48, I don't have experience with wifi deploys. My guess is it was put there because we can't link lldb over wifi -- if we can, then we can remove the restriction.
The if(_timeout...) code was to preserve the existing logic with the timeout and how that works (save the last detected device, for use when the timeout expires, so it can handle it). Ok, I also now see a memory leak there in the CFRetain if more than 1 device is connected before the timeout... (should release the previous if it is set)
Personally I would love to refactor this, but I am settling for "not breaking existing functionality" for now.
There was a problem hiding this comment.
Yes, running and debugging over wifi now works.
I think you are misunderstanding the old functionallity. The AMDeviceGetInterfaceType(info->dev) != 2 is always true for USB devices, so best_device_match is set only if we are trying to debug with only Wifi device present. (it will search for USB devices, if finds none - falls back to wifi after timeout).
Now, as the wifi debugging works, we dont need to fallback, we can just use the first device we get.
Atleast for the Iphone I have, the USB connection is always detected first, which would be preferrable, but this may not be the case always. For cases when you want to force the USB connection, one can still use the --no-wifi flag.
There was a problem hiding this comment.
Alright. Can you make the changes needed as you proposed and I'll retest.
As previous commit fixes debugging over wifi, it is unnecessery to only fallback to wifi if no valid usb devices are found.
|
I am going to leave the testing via pre-release versions I will release via npm, and fix bugs as reports come in. I will update this thread. |
shazron
changed the title
|
Beta release, please test. Instructions: https://github.com/ios-control/ios-deploy/releases/tag/1.10.0-beta.1 |
…os-control#385) * Fixup API usage to make ios-deploy work also with wifi connections * remove best match logic As previous commit fixes debugging over wifi, it is unnecessary to only fallback to wifi if no valid usb devices are found. * remove the unnecessary !found_device
This quite a large change, comes dowm to few changes.
ServiceConnRefandAFCConnectionReftypedefs to better separate when what is used.start_remote_debug_serverandstart_house_arrest_service.assertusage, that would break if someone compiled it for release.