Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect unrooted device, send user to root #176

Closed
j005u opened this issue Oct 1, 2022 · 13 comments · Fixed by #191, #335 or #338
Closed

Detect unrooted device, send user to root #176

j005u opened this issue Oct 1, 2022 · 13 comments · Fixed by #191, #335 or #338
Assignees
Milestone

Comments

@j005u
Copy link
Contributor

j005u commented Oct 1, 2022

Despite #163 being unfixable, i.e. we can't share permissions between serial and adb, we can still improve the user experience.

When the user attempts to connect, we would look for the DJI device by vid and pid. If we get a device and no adb interface is present, we can "force" the user to root in the UI automatically. If the adb interface is present (re-checking each time the device connects) we can init webadb, which should pick up the device via the original pairing?

This means users will no longer be confused when no device is present on the front page pre-root, and we can still unify the UI flow for the install process. The user would simply need to connect an additional time for rooting.

@j005u
Copy link
Contributor Author

j005u commented Apr 9, 2023

Necroing.

On an unrooted vista I now get "Could not connect to device, make sure that adb server is not running on your machine (adb kill-server) and that you are not connected in another tab/window." rather than being asked to root or not being able to connect at all from the front page.

I beleive this is also the reason for several reported user issues.

@stylesuxx
Copy link
Collaborator

I could reproduce the issue on a Vista. There are two issues:

  1. I did not check for the subclass, only interface Class - this is now fixed.
  2. Vista shows at first up with 6 interfaces, takes a bit and then has a seventh interface. This is especially an issue on the initial plug in. Since the vista is then being detected as not rooted which would lead to a false-positive of an unrooted device and redirection to the root page.

So, to mitigate this and keep the root detection, I would suggest to run the adbCheck for 20 seconds - if it is not detected in this time frame we redirect to the root page.

@j005u
Copy link
Contributor Author

j005u commented Apr 10, 2023

That makes sense since the patch that enables adb runs shortly after startup. Such a timeout would also help with getting the adb kill-server message briefly during startup due to the same issue.

I'll try do some testing and determine if 20 seconds is the sweet spot.

@stylesuxx
Copy link
Collaborator

I think it needs to be longer than that, I now changed the check to check every n seconds for a total of y seconds. If after y seconds we don't get an ADB device, I consider it unrooted and redirect.

@j005u
Copy link
Contributor Author

j005u commented Apr 10, 2023

It takes 11 seconds for the usb configuration to change on the goggles side on V1 and 13 seconds on a Vista from the time the interfaces first get inited.

However on Windows it may take several more seconds for the USB subsystem state to update accordingly, there appears to be a bug where it gets slower and slower the longer you don't restart. I've experienced this several times before, although it's not reliably reproducible.

Maybe this warrants a new status message at the top while the device is detected and we're waiting for adb to come up? "Waiting for wtfos to start"?

@stylesuxx
Copy link
Collaborator

Yes, I was thinking about something similar, maybe another loading spinner from the time the connection is being detected till the time ADB is found or the redirect happens.

On my machine I am closer to 20 seconds - my vista tends to disconnect after the initial connect then re-connect, and from there I have to wait a bit for the correct device to show up. I now added a counter reset for such a case.

@j005u
Copy link
Contributor Author

j005u commented Apr 10, 2023

The timing seemed pretty consistent on the device side according to dmesg but yeah driver stacks can take a while to figure out what's going on.

The disconnection happens because in order to add the adb interface the gadget must first be set to a config of 'none'. This happens every time USB mode is changed for whatever reason.

@stylesuxx
Copy link
Collaborator

OK, perfect - makes sense.

@j005u
Copy link
Contributor Author

j005u commented Apr 11, 2023

Everything works as advertised on V1 Goggles, which should have the same behavior as a Vista. I'll do the next run on a Vista.
A few things though:

  • We should probably set another toast after the timeout to tell the user something along the lines of "ADB not found after timeout. Please root your device"
  • This toast should persists if I navigate back to the root page/elsewhere, not trigger another 30s timeout that redirects me back to root again. This can be pretty confusing.
  • "Device found" could maybe just be "Device connected"?
  • Feels like more than a moment, "a few moments"? Straight up "30 seconds"?
  • I got two somewhat conflicting toasts at the same time on the front page image
  • We can now remove the word rooted from the second toast (when it is appropriate)
  • I just realized I don't think you have to poll the state at all since you'll always get a new connection event when the usb config changes in the OS? Right now there's a flash of "checking for ADB" even though technically there doesn't have to be I think. #notabigdeal though.

I absolutely hate the 30s wait, but I can't really think of any reliable way of making it faster given the boot process as it is. I guess it's down to improving the startup speed if possible.

@stylesuxx
Copy link
Collaborator

stylesuxx commented Apr 11, 2023

Technically the user should be redirected to the root page after the timeout, is this not happening for you? Because this then would make the suggested toast useless, no?

I don't need to poll the state, but this would mean I would have to wait in every case, even with the goggle V2 where the device is there immediately. So the polling is there to speed up the potential wait time. I could show the "checking for ADB" only in case the first check fails.

I am not a big fan of the wait either, but I guess it's simply the better user experience - but sure if we can improve the startup speed, I am all up for it.

@j005u
Copy link
Contributor Author

j005u commented Apr 11, 2023

I do get redirected to the root page, but I just think the user might be somewhat confused by suddenly being on a page with a single blue button. The toast would let them know why they're here and remind them in case they navigate away, until we're in this state.

Right, v2 goggles have that funny exec error for a few seconds at startup, I forgot. Fair enough, no point in fiddling with the working code then.

@stylesuxx stylesuxx linked a pull request Apr 11, 2023 that will close this issue
4 tasks
@stylesuxx
Copy link
Collaborator

Feedback implemented - @j005u could you give it another try please?

@j005u
Copy link
Contributor Author

j005u commented Apr 12, 2023

Brilliant, I didn't fine any issues and the UX is much more clear now.

Sorry for the scope creep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment