Binder/AIDL content updates #1618

randomPoison · 2023-12-29T20:45:12Z

So far this PR makes two broad improvements to the Android course materials:

It fleshes out the existing course materials by adding more speaker notes and showing more of the generated code.
It adds a new section going over how AIDL types are supported by Rust and mapped to Rust types.

There are still additional changes to do (add an exercise, add a section on async binder), but those can be split into separate PRs if necessary.

This PR is still WIP because I need to test the code examples around the Binder types; Some of them are basically just psuedo-code at the moment.

rinon

This is looking really great! More depth than was there before, and I like the new notes you've incorporated. I think that covers most of what I've added off slides when I've taught this, but I'll think if there's anything else we should add.

src/android/aidl/birthday-service.md

src/android/aidl/birthday-service/birthday_service/Android.bp

...idl/birthday-service/birthday_service/aidl/com/example/birthdayservice/IBirthdayService.aidl

src/android/aidl/birthday-service/client.md

ahomescu · 2024-01-09T20:34:14Z

...irthday-service/birthday_service/aidl/com/example/birthdayservice/IBirthdayInfoProvider.aidl

@@ -1,4 +1,4 @@
-// Copyright 2022 Google LLC
+// Copyright 2024 Google LLC


Nit: 2022-2024 or just keep the original year?

This is a new file I created, so I thought we'd want to use 2024 in the comment since that's when it's written?

I think using 2024 is correct for new files. My understanding is that we leave the years alone in existing files.

Way back when I first read about licenses for open source projects, the advice was to update the year of files that have received "significant changes" or so. Many projects just updated the year of every file in the beginning of the year. It's now my understanding that we don't do that, we just leave the files alone and only add new notices to new files.

src/android/aidl/birthday-service/birthday_service/src/client.rs

src/android/aidl/birthday-service/service-bindings.md

src/android/aidl/types.md

src/android/aidl/types/file-descriptor.md

src/android/aidl/types/parcelables.md

randomPoison · 2024-01-20T01:37:37Z

@ahomescu @rinon I've finished updating the example code and have addressed the feedback from your first review. Give this another look over when you have a chance.

src/android/aidl/birthday_service/src/client.rs

ahomescu · 2024-01-20T06:11:31Z

src/android/aidl/birthday_service/src/lib.rs

+        &self,
+        provider: &SpIBinder,
+    ) -> binder::Result<String> {
+        use binder::binder_impl::Proxy;


Isn't this re-exported by binder? If it's not but it's needed, we should fix that upstream.

It does not currently appear to be re-exported.

Never mind, see the next comment.

src/android/aidl/birthday_service/src/lib.rs

src/android/aidl/example-service/interface.md

src/android/aidl/example-service/server.md

ahomescu · 2024-01-20T06:16:49Z

src/android/aidl/types/arrays.md

+| ---------------------- | ------------- |
+| `in` argument          | `&[T]`        |
+| `out`/`inout` argument | `&mut Vec<T>` |
+| Return                 | `Vec<T>`      |


Also parcelable fields.

What about parcelable fields? I'm not sure how that's relevant on this slide.

I'm saying if you have a parcelable field that's an array, it will also use a Vec<T>. E.g.

parcelable Foo { int[] a; }

Ah, that makes sense. I think I'll keep the slide specific to arrays in function signatures, but I'll add this to the speaker notes.

src/android/aidl/types/file-descriptor.md

src/android/aidl/types/parcelables.md

* Add speaker notes covering more details about how AIDL/Binder works. * Add examples of generated Rust bindings. * Flesh out the section on updating a Binder API.

* Add interface methods for passing binder objects and file descriptors. * Partially implement the Rust service/client/server changes for the updated AIDL definitions.

* Fix code in `client.rs` for sending Binder objects. * Add client code for sending a file descriptor.

ahomescu · 2024-02-01T00:02:33Z

src/android/aidl/birthday_service/src/client.rs

 /// Call the birthday service.
-fn main() -> Result<(), binder::Status> {
+fn main() -> Result<(), Box<dyn Error>> {


I hope people don't start doing this in their production code. Otoh I'm not sure it's worth talking about anyhow and thiserror here.

src/android/aidl/birthday_service/src/lib.rs

ahomescu · 2024-02-01T00:10:24Z

src/android/aidl/birthday_service/src/lib.rs

+        &self,
+        provider: &SpIBinder,
+    ) -> binder::Result<String> {
+        use binder::binder_impl::Proxy;


Never mind, see the next comment.

ahomescu · 2024-02-01T00:14:46Z

src/android/aidl/types/arrays.md

+
+The array types (`T[]`, `byte[]`, and `List<T>`) get translated to the
+appropriate Rust array type depending on how they are used in the function
+signature:


Maybe this was the source of the confusion: types can occur in function signatures but also in parcelable field definitions. References are never used for the latter, because they would add lifetimes to the corresponding Rust types.

ahomescu · 2024-02-01T00:17:41Z

src/android/aidl/types/primitives.md

+| --------- | --------- | ----------------------------------- |
+| `boolean` | `bool`    |                                     |
+| `byte`    | `i8`      | Note that bytes are signed.         |
+| `char`    | `u16`     | Note the usage of `u16`, NOT `u32`. |


Depending which language you come from, you'd expect a different type here: char would be u8 coming from C, u16 from Java, or u32 from Rust. AIDL picked the Java option.

ahomescu

LGTM, looks like I'm not allowed to approve the PR.

thedataking

Approving on behalf of Andrei 🚀

mgeisler · 2024-02-20T10:37:48Z

Thanks all for the much-needed updates here! 😄

Two ideas:

Can we link this from our internal Binder documentation? I feel this section has now become a quite nice reference.
Can we figure out a way to get this tested in CI? Perhaps you could prepare a Docker image and use this somehow?
- This is the first hit: https://github.com/kylemanna/docker-aosp, but it looks like it hasn't been touched in ~7 years
- Another hit: https://android.googlesource.com/platform/build/+/master/tools/docker, but this has also not been touched in ~4 years

randomPoison requested a review from fw-immunant December 29, 2023 20:45

rinon reviewed Jan 9, 2024

View reviewed changes

ahomescu reviewed Jan 9, 2024

View reviewed changes

randomPoison force-pushed the legare/binder-updates branch from 59ebd7b to b7f4655 Compare January 20, 2024 01:32

randomPoison marked this pull request as ready for review January 20, 2024 01:32

ahomescu reviewed Jan 20, 2024

View reviewed changes

randomPoison added 17 commits January 24, 2024 08:16

Flesh out existing Binder content

ca53547

* Add speaker notes covering more details about how AIDL/Binder works. * Add examples of generated Rust bindings. * Flesh out the section on updating a Binder API.

Move Birthday Service section into its own directory

fcddc17

Add first pass at types section

f634395

Remove unused string.md

8756902

Add speaker notes for parcelables slide

0d132ef

Partially extend the BirthdayService example

c94e0f2

* Add interface methods for passing binder objects and file descriptors. * Partially implement the Rust service/client/server changes for the updated AIDL definitions.

Fix binder object example

dc837fc

Fixup and flesh out the objects slide

829a75c

* Fix code in `client.rs` for sending Binder objects. * Add client code for sending a file descriptor.

Cleanup parcelables slide

3af6844

Implement the service side of things

6b6f112

Implement service side of wishFromFile

af905af

Cleanup birthday service code examples

dd6365d

Reword birthday-service.md

dde7958

Re-organize birthday_service example

1029f40

Improve wording in speaker notes

9cbe86f

Misc code review cleanup

d76d7f4

Run formatter

7eb9cb7

randomPoison force-pushed the legare/binder-updates branch from 3dea292 to 7eb9cb7 Compare January 24, 2024 16:16

randomPoison added 4 commits January 30, 2024 09:59

Normalize error handling in client.rs

ea184a6

Misc minor improvements

2b87d25

Remove comment about vendor_available: true

995005a

Fix failing tests

bc415a1

ahomescu reviewed Feb 1, 2024

View reviewed changes

randomPoison added 2 commits February 1, 2024 15:12

Add note about arrays in parcelables

2f51ba2

Use into_interface to downcast SpIBinder

f517a4e

ahomescu reviewed Feb 9, 2024

View reviewed changes

thedataking approved these changes Feb 9, 2024

View reviewed changes

randomPoison merged commit f5f2c6b into main Feb 9, 2024
35 checks passed

randomPoison deleted the legare/binder-updates branch February 9, 2024 23:11

mani-chand pushed a commit to mani-chand/comprehensive-rust that referenced this pull request Feb 16, 2024

Binder/AIDL content updates (google#1618)

3034c69

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binder/AIDL content updates #1618

Binder/AIDL content updates #1618

randomPoison commented Dec 29, 2023

rinon left a comment

ahomescu Jan 9, 2024

randomPoison Jan 20, 2024

mgeisler Jan 25, 2024

randomPoison commented Jan 20, 2024

ahomescu Jan 20, 2024

randomPoison Jan 31, 2024

ahomescu Feb 1, 2024

ahomescu Jan 20, 2024

randomPoison Jan 31, 2024

ahomescu Jan 31, 2024 •

edited

Loading

randomPoison Feb 1, 2024

ahomescu Feb 1, 2024

ahomescu Feb 1, 2024

ahomescu Feb 1, 2024

ahomescu Feb 1, 2024

ahomescu left a comment

thedataking left a comment

mgeisler commented Feb 20, 2024

		@@ -1,4 +1,4 @@
		// Copyright 2022 Google LLC
		// Copyright 2024 Google LLC

Binder/AIDL content updates #1618

Binder/AIDL content updates #1618

Conversation

randomPoison commented Dec 29, 2023

rinon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randomPoison commented Jan 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahomescu Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahomescu left a comment

Choose a reason for hiding this comment

thedataking left a comment

Choose a reason for hiding this comment

mgeisler commented Feb 20, 2024

ahomescu Jan 31, 2024 •

edited

Loading