Skip to content

Conversation

@geom3trik
Copy link
Collaborator

@geom3trik geom3trik commented Mar 17, 2025

Description

Adds ASIO backend.

Needs #31

Fixes #16

Type of Change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Performance improvement
  • Code cleanup or refactor

How Has This Been Tested?

Todo

Checklist:

  • My code follows the style guidelines of this project
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Wherever possible, I have added tests that prove my fix is effective or that my feature works. For changes that
    need to be validated manually (i.e. a new audio driver), use examples that can be run to easily validate them.
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

Screenshots (if appropriate):

Additional Notes:

Add any additional notes about the PR here.

Copy link
Owner

@SolarLiner SolarLiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I got too hasty reviewing this when it's still draft, but I thought I'd do it anyway and you can take the comments into account when you want

@SolarLiner SolarLiner added feature New feature or request P-Windows Issues and PRs targetting Windows labels Mar 17, 2025
@github-project-automation github-project-automation bot moved this to Todo in Backlog Mar 17, 2025
@SolarLiner SolarLiner added this to the 0.1.0 milestone Mar 17, 2025
@SolarLiner SolarLiner linked an issue Mar 17, 2025 that may be closed by this pull request
@SolarLiner SolarLiner moved this from Todo to In Progress in Backlog Mar 17, 2025
@geom3trik geom3trik force-pushed the asio-duplex branch 2 times, most recently from b49e376 to 690ae2c Compare March 29, 2025 13:22
@geom3trik geom3trik marked this pull request as ready for review March 29, 2025 14:08
@geom3trik geom3trik requested a review from SolarLiner March 29, 2025 14:08
Copy link
Owner

@SolarLiner SolarLiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna make a few new changes to the Duplex API branch to remove the DeviceType::Duplex enum since it's useless. I should have done it sooner and it made you do a bunch of extra work for it.

If you haven't rebased onto that branch yet, don't do it just yet; I'll ping you when I'm done

Copy link
Owner

@SolarLiner SolarLiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be good now

@geom3trik
Copy link
Collaborator Author

I'm gonna make a few new changes to the Duplex API branch to remove the DeviceType::Duplex enum since it's useless. I should have done it sooner and it made you do a bunch of extra work for it.

Hmm okay, I was using that to return the default input and output device since on ASIO that could be a single duplex device.

@geom3trik geom3trik force-pushed the asio-duplex branch 2 times, most recently from 925fa0d to 375e531 Compare April 16, 2025 13:12
Copy link
Owner

@SolarLiner SolarLiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good now, you have the last suggestion to include and I think we can merge this sooner rather than later –- this way we can iterate over the Duplex API as a whole instead of having to juggle between all those PRs

@github-project-automation github-project-automation bot moved this from In Progress to Can be merged in Backlog Apr 16, 2025
@geom3trik
Copy link
Collaborator Author

I think it's good now, you have the last suggestion to include and I think we can merge this sooner rather than later –- this way we can iterate over the Duplex API as a whole instead of having to juggle between all those PRs

Yep I agree 👍 was waiting for this to merge before reviewing the duplex api pr. Should be able to finish this off in the next couple of days.

@geom3trik geom3trik requested a review from SolarLiner April 17, 2025 14:22
Copy link
Owner

@SolarLiner SolarLiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (pending the couple of comments I made).

I think I'll want to make more changes to the general Duplex API (especially the StreamConfig I think I'll separate the input and output channels, it's a little bit of duplication but I think it'll be worth it).

Comment on lines +38 to +44
let device_type = match (is_input, is_output) {
(true, true) => DeviceType::DUPLEX,
(true, false) => DeviceType::INPUT,
(false, true) => DeviceType::OUTPUT,
// todo
(false, false) => return Err(AsioError::BackendError(asio::AsioError::NoDrivers)),
};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also add DeviceType::PHYSICAL as a property for hardware devices

Comment on lines +63 to +67
let buffer_size = match stream_config.buffer_size_range {
(Some(min), Some(max)) if min == max => Some(min as i32),

_ => None,
};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since we can control the exact buffer size, we should go with min or max.min(512) instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand. I thought when min == max then the user has specified a specific buffer size.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, however currently you're not respecting the conditions the user has provided in any other case (ex. (None, Some(128) or (Some(256), Some(1024)).

The expression I gate would translate in Rust as min.or(max).unwrap_or(512). But I since saw that the buffer size you pass to asio-sys is actually an Option, so you can remove the .unwrap_or part.

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

Labels

feature New feature or request P-Windows Issues and PRs targetting Windows

Projects

Status: Can be merged

Development

Successfully merging this pull request may close these issues.

Add ASIO driver

2 participants