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

feat: implement adb pull for usb #42

Merged
merged 10 commits into from
Oct 25, 2024
Merged

feat: implement adb pull for usb #42

merged 10 commits into from
Oct 25, 2024

Conversation

lavafroth
Copy link
Contributor

@lavafroth lavafroth commented Oct 18, 2024

Changes

  • Added USBSubcommand enum for subcommands like List, Stat, etc.
  • Added private send_and_expect_okay and recv_and_reply_okay method to ADBUSBDevice since the host and the device quite frequently acknowledge each others' messages with OKAY commands.
  • New members in ADBUSBDeviceExt trait
    • A stat method to stat a file on the device.
    • The pull function which takes a source file path (string) to read from the device and writes its contents to a Writer output.
    • Added USB subcommands pull to the CLI

Note

stat and pull are implemented for the USB side but they are left as todo!s for ADBServerDevice

To pull a file called src from the device and save it as dst, run

cargo r -- usb --vendor-id 2209 --product-id 2700 pull src dst

@cocool97
Copy link
Owner

Thanks for the PR !

  • Can you add the code in adb_cli ? You can see how it's done in pull for physical devices
  • Can you add at least the pull function in the trait ? It is implemented in physical devices and just needs to be moved :)

I'll review your PR and use some functions you wrote for the main USB branch

@lavafroth
Copy link
Contributor Author

Hello, I added a subcommand to adb_cli now you can run the command as described above to pull a file and save it to a destination. I'm afraid I didn't quite understand your second point. I have added the pull function in the trait. It is just not implemented for the ADBServerDevice yet. 😅

@cocool97
Copy link
Owner

Pull command is implemented, just named recv 😉
I think we can rename it to be more consistent with upstream ADB cli !

@lavafroth
Copy link
Contributor Author

Okay thank you. I have moved the recv method to the ext trait. The server, as you mentioned, uses the existing implementation. The ADBUsbDevice uses the implementation from this pr.

@cocool97
Copy link
Owner

cocool97 commented Oct 24, 2024

Thanks for your improvement. There are still some things that might be improved :

  • Stat method in ADBDeviceExt needs a local_id and remote_id, but some other methods also need it without require it in public API (like shell e.g). From a user perspective, these identifiers should be hidden. Must an OPEN command be sent before trying to pull a file from device ? If yes, we can merge behaviours with shell and maybe store these identifiers in the USBTransport structure ?
  • Stat is also implemented for ADBServerDevice, can you replace the todo!() with the current implementation ?
  • The server implementation returns an ADBStatResponse (src/server/device_commands/stat.rs), can you merge this implementation with your FileStat to have only one ?

I've done the rebase on usb branch so after these modifications I'll test it and merge :)

@lavafroth
Copy link
Contributor Author

lavafroth commented Oct 24, 2024 via email

@lavafroth
Copy link
Contributor Author

Hey there, I have made the necessary changes. The user should no longer have to be concerned about the local_id and remote_id. The server and usb stat response structs are merged as well!

@cocool97
Copy link
Owner

cocool97 commented Oct 25, 2024

I had a look at your code, everything is fine. The begin_transaction method is nice, I now use it in shell too :)

Just have a minor issue before merging it, stat command seems to not return values in the correct order and does not close correctly (as seen after)

(/data/local/tmp/bigfile has just been created in this case with dd if=/dev/urandom of=/tmp/bigfile bs=100k count=1024 and has a size of 100Mb)

First run:

RUST_LOG=adb_client=debug cargo run -- usb --vendor-id 04e8 --product-id 6860 stat /data/local/tmp/bigfile
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.15s
     Running `target/debug/adb_cli usb --vendor-id 04e8 --product-id 6860 stat /data/local/tmp/bigfile`
[2024-10-25T08:15:19Z INFO  adb_client::usb::adb_usb_device] Authentication OK, device info device::ro.product.name=gts210vewifixx;ro.product.model=SM-T813;ro.product.device=gts210vewifi;features=cmd,shell_v2
File permissions: 1413567571
File size: 33279 bytes
Modification time: 1973-04-28 15:06:40.000000000 UTC

Second run:

Error: ADB request failed - Wrong command received CLSE

@cocool97
Copy link
Owner

Behaviour is the same when trying to pull the same file twice without unplugging the device

@lavafroth
Copy link
Contributor Author

lavafroth commented Oct 25, 2024

The pull and stat commands were leaving ADB on the device hanging but this has been fixed! We had to end a sync transaction using a quit command.

@cocool97
Copy link
Owner

Thanks, this works better !

Last point about stat command, output is not as expected for a single file.
To reproduce create a file, run CLI in "ADBServerDevice" mode and run it in "USB" mode, outputs are not the same. I tried and "server" mode outputs are correct.

@lavafroth
Copy link
Contributor Author

Ah yes, it looks like there is an extra u32 in the beginning of the stat response structure. Since we are not using it, I have made the parser skip it as well.

@cocool97
Copy link
Owner

Yes, extra u32 is the litteral "STAT". I add a comment to your last commit and merge it right after

@cocool97 cocool97 merged commit 73021b1 into cocool97:usb Oct 25, 2024
@lavafroth
Copy link
Contributor Author

lavafroth commented Oct 25, 2024 via email

@lavafroth lavafroth deleted the adb-pull branch October 25, 2024 11:39
cocool97 added a commit that referenced this pull request Oct 25, 2024
* feat: implement adb pull for usb

* feat: add subcommands for pull and shell

* feat: add recv method to `ADBDeviceExt` trait, use existing impl for server

* feat: merge stat commands for server and usb `impl`s

* feat: improve shell; fix: minor rewording

* fix: reply with quit usb subcommand at the end of a transaction

* fix: adb stat works on a file, not always an apk

* fix: ignore extra stat field for now

---------

Co-authored-by: LIAUD Corentin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants