search

diwic / alsa-rs

Thin but safe ALSA wrappers for Rust
Apache License 2.0
139 stars 66 forks source link

Add `AudioTstampType` and related funtions. #61

Closed alexmoon closed 3 years ago

diwic commented 3 years ago

@alexmoon So I did some digging and found the constants and figured that the right thing to do was to send a patch upstream to alsa-lib. If they do we can then regenerate through alsa-sys and get the constants that way.

As for the #[non_exhaustive] part that should probably be added to more enums (now that I think of it), but I guess that attribute was not in Rust when I started writing on those bindings.

alexmoon commented 3 years ago

@alexmoon So I did some digging and found the constants and figured that the right thing to do was to send a patch upstream to alsa-lib. If they do we can then regenerate through alsa-sys and get the constants that way.

That makes sense to me.

alexmoon commented 3 years ago

BTW, one other API I considered was instead of adding PCM::status_with_audio_htstamp_config() to create a StatusBuilder struct with a set_audio_htstamp_config(..) method and a build(self, pcm: &PCM) -> Status method. That would be more convenient if they add more settings in the future, but seemed like overkill for right now.

alexmoon commented 3 years ago

So I thought about it some more and decided that the builder API was closer to the underlying ALSA API and in some ways conceptually simpler so I've switched to it. Once alsa-sys is updated with the new version of alsa-lib we should be able to wrap this PR up.

diwic commented 3 years ago

@alexmoon so alsa-sys 0.3.1 has been released now with relevant upstream changes in it, you can update your PR. Please squash it when you think it's ready for merging.

alexmoon commented 3 years ago

@diwic ok, I've squashed the commits. Let me know if there are any other changes you would like to see.

diwic commented 3 years ago

Merged now. Thanks!