-
Notifications
You must be signed in to change notification settings - Fork 451
feat: stable Device::id() #1014
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
Conversation
roderickvd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Your proposal literally crossed paths as I was working on something also. No matter, all the better I could provide some review comments with actual code suggestions.
Please also add a changelog, it's worth it 😄
|
Hello @roderickvd ! I have implemented most of the changes requested and renamed the enum variants to the suggested names. For I have also added I will work on changing the macOS CoreAudio type to The changelog has also been updated 😄. |
|
Cool stuff let me know when you're ready for me to take another look. |
|
I have finished the changes and now One thing that I noticed was that there was already a The code I'm currently using is basically 90% the same as the |
roderickvd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some ideas and one point about the macOS implementation.
roderickvd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sticking through 👍 this is a pretty important feature so let's get it right!
|
Thanks, I’ll review later as I’m away for a few days. |
|
Hello @roderickvd, Just wondering if you were available to review the updates, as I was hoping to get the If it comes at an inconvenient time, I'm happy to wait. |
roderickvd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping and sorry for the wait. Yes lots of work done, thanks for that. Here's a few hopefully small ones.
| "emscripten" => Ok(DeviceId::Emscripten(data.to_string())), | ||
| "ios" => Ok(DeviceId::IOS(data.to_string())), | ||
| "null" => Ok(DeviceId::Null), | ||
| &_ => todo!("implement DeviceId::FromStr for {platform}"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we discussed this earlier but I find myself doubting here.
Should we panic here or return DeviceIdError::UnsupportedPlatform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a DeviceIdError::UnsuportedPlatform would be better in this case as it would leave more flexibility for the end user, if they want their application to panic if the API is unsupported that option is allowed, but we also give them the option to accept it as an error instead of crashing their app.
|
Thanks for all the work and updating with the custom host support. I wonder if we can support custom hosts better in the future, but that can be added later if necessary. Eager to get this out with v0.17 and see the community response! |
This PR adds a
Device::id()command that returns the OS native device id on supported operating systems.Currently, only macOS and Windows (WASAPI) are supported.Function
By returning a
Device::id(), it will be easier to ensure that the same device gets connected on application startup. This also allows for greater extension capability for other libraries to extend uponcpalfunction.Architecture
The OS specific device ids are all stored in a
DeviceIdenum, that allows for support for the various data types that different operating systems and APIs use to store device ids (e.g.u32for macOS andStringfor WASAPI.To handle any errors that might be outputted in when running
Device::id(), there is also a newDeviceIdErrorenumCalling
Device::id()will return aResult<DeviceId, DeviceIdError>. If the OS is supported and the device id is obtained, it will return aDeviceId. If the OS is unsupported, aDeviceIdError::UnsupportedOSerror will be returned.Current Status
Currently, support for Windows WASAPIStringIds has been added, along with support for macOSu32Ids. I am working to add support forALSAand maybepulseaudio(once that gets merged).For other APIs, I do not have the hardware to test or program for them,
so they currently return aDeviceIdError::UnsupportedOSerror.Testing
Device::id()for macOS should return thestandarddevice uid that can be used anywhere.u32For Windows, to convert the Rust
Stringtype to aPWSTR, the following code can be used