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

Dart vdd #108

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Conversation

Dampfwalze
Copy link

@Dampfwalze Dampfwalze commented Apr 1, 2024

Related: #101

root
├───rust
│   └───dart_vdd                        # Rust - Dart bridge (Dart package)
│       ├───example
│       │   └───dart_vdd_example.dart
│       ├───lib
│       │   └───src
│       │       └───generated
│       │           └───api.dart        # Dart API generated from Rust
│       └───rust                        # Cargo project
│           └───src
│               └───api.rs              # Rust API
└───vdd_control                         # Flutter app
    └───lib
        └───main.dart

There are a few points of interest:

I used a Dart feature called native-assets to build and link the Rust library, which is currently not stable and behind an experimental flag. It is essentially a build.dart file, similar to Rusts build.rs. Traditionally, it was not possible to distribute binaries with just Dart. Flutter has its own mechanisms for this, but then, the package can only be used in Flutter projects. The native-assets feature is meant to replace these mechanisms in the future. But for now, it is only available in Flutter on the main channel (like nightly in Rust).

The API uses a Dart Stream as the main way of communicating the driver's state (as an immutable data structure). It would stream any changes made to the driver, regardless of who did it, so it would always reflect the correct state of the driver. This pattern is quite native to Flutter to build on top of.

Copy link

github-actions bot commented Apr 1, 2024

CLA Assistant Lite bot All Contributors have signed the CLA.

@Dampfwalze
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@MolotovCherry
Copy link
Owner

MolotovCherry commented Apr 9, 2024

@Dampfwalze Okay, bindings should be hooked up now. Let me know if this initial implementation is ok or needs reworking 😁

A note about the under the hood Result returns:

  • persist() fails when it couldn't save registry key
  • notify() fails when either serialization failed, it failed to write to the pipe, or failed to flush the pipe
  • add_monitor() fails when it can't find a new id (a failure should be impossible here)
  • set_monitor() fails if the monitor id was not found, or if monitor id is a duplicate of another already existing one, or if duplicate modes exist, or if duplicate refresh rates exist (basically, it checks whether the passed in monitor data is valid). This should never fail if you handle duplicate checking on your side
  • set_monitors() fails similarly to above. It does duplicate checking.
  • new() fails if the pipe could not be connected too (which means the driver is not running) (pipe name is currently a no-op since I still have it hardcoded)

stream is just running a thread under the hood. it's meant to only be set once (which is what I assume your use-case was. let me know if this assumption was wrong). not sure how to handle a possible error for stream failure here, so an error is a no-op right now

Some rust features cannot be bridged to Dart at the moment. These include type aliases and slices.
@MolotovCherry
Copy link
Owner

MolotovCherry commented Apr 11, 2024

I made breaking changes to the bindings again. I'll fix them on this branch later today

@Dampfwalze
Copy link
Author

I made breaking changes to the bindings again. I'll fix them on on this branch later today

I reverted the merge for now with a force push

@MolotovCherry
Copy link
Owner

Should be good now. I feel like I've more or less settled on a good and robust api (though let me know if I did miss something).

@Dampfwalze
Copy link
Author

Should be good now. I feel like I've more or less settled on a good and robust api (though let me know if I did miss something).

I feel like you are desperately trying to hide the tokio runtime inside your api and keeping your api synchronous. I don't think this is beneficial. It should be the responsibility of the user to provide a runtime. In fact, flutter_rust_bridge provides its own tokio based runtime.

There are many methods which can just be asynchronious, which not only reduces complexity, but also gives the control to the user.

@MolotovCherry
Copy link
Owner

MolotovCherry commented Apr 12, 2024

Should be good now. I feel like I've more or less settled on a good and robust api (though let me know if I did miss something).

I feel like you are desperately trying to hide the tokio runtime inside your api and keeping your api synchronous. I don't think this is beneficial. It should be the responsibility of the user to provide a runtime. In fact, flutter_rust_bridge provides its own tokio based runtime.

There are many methods which can just be asynchronious, which not only reduces complexity, but also gives the control to the user.

Tokio is indeed an intentional implementation detail, in which the runtime was required to properly handle things for the sync api.

Later on I will re-organize the API and internal code more to better reflect both cases, and in the async case, use the users runtime. When I was talking about settling on something good, I meant with the overall internal design and style of the api (not whether parts were sync or async; that's easily solvable). For example, things like, "did you need access to the monitor state but an api method to access them isn't there?" and "I want to add a mode, but the api doesn't have that function, and I'd rather not write it myself since it's such a common thing"

I was not avoiding making more async methods that match their sync counterparts, or avoiding making a clear delineation between both async/sync apis. The design is after all still in flux, and is an iterative process as most things are.

Did you have need for async equivalents right now?

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.

None yet

2 participants