-
Notifications
You must be signed in to change notification settings - Fork 441
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
base: master
Are you sure you want to change the base?
Conversation
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 |
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.
I have updated the I have also made Currently, I think it is fine as all three of those APIs seem to only return the default device, however I think if they were expanded to be able to select audio devices, it would be better to leave them returning As for the syncing issue, I'm unsure about what you mean, would you care to elaborate? |
From your PR description, which I edited to show the progress made since:
Is that still necessary? |
Thanks for the update @roderickvd ! I'll try getting the code updated soon for another review.
Currently I believe it is still necessary to convert it to a This is primarily for if you are using the |
But it already does I don't have a Windows machine myself, so am trying to understand. |
So right now I'm having it return the The snippet about That code is more about external access using the |
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 uponcpal
function.Architecture
The OS specific device ids are all stored in a
DeviceId
enum, that allows for support for the various data types that different operating systems and APIs use to store device ids (e.g.u32
for macOS andString
for WASAPI.To handle any errors that might be outputted in when running
Device::id()
, there is also a newDeviceIdError
enumCalling
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::UnsupportedOS
error will be returned.Current Status
Currently, support for Windows WASAPIString
Ids has been added, along with support for macOSu32
Ids. I am working to add support forALSA
and 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::UnsupportedOS
error.Testing
Device::id()
for macOS should return thestandarddevice uid that can be used anywhere.u32
For Windows, to convert the Rust
String
type to aPWSTR
, the following code can be used