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

Fix audio_get_status result #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@ssalonen
Copy link
Owner

ssalonen commented Sep 12, 2024

Thanks!

Hmm the enum*.rs is autogenerated by https://github.com/ssalonen/libcec-sys/blob/master/scripts/run-bindgen/bindgen.sh

So somethings needs to be done there... need to have a bit closer look to comment something sensible

can you add simple tests to the conversion similar to other such functions in this lib?

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Sep 12, 2024

Oh ok! I'll just add them back. I mainly just removed them because the new struct made those enums unnecessary.

I'll also keep using the constants from libcec_sys for the struct, because CEC_AUDIO_VOLUME_STATUS_UNKNOWN conflicts with CEC_AUDIO_VOLUME_STATUS_MASK. It'd be a bit confusing to useCecAudioStatus::MuteStatusMask to check when the status is unknown.

@kira-bruneau kira-bruneau force-pushed the fix-audio-get-status-result branch 2 times, most recently from 560d074 to 0888e34 Compare September 12, 2024 17:40
@ssalonen
Copy link
Owner

Yes I think it's ok to keep all enums as-is and having this usability-layer separately like you have done here with new struct

@kira-bruneau kira-bruneau force-pushed the fix-audio-get-status-result branch 5 times, most recently from 666685e to 8182776 Compare September 13, 2024 12:25
@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Sep 13, 2024

I refactored my changes a bit so this new struct can be used with https://github.com/ssalonen/cec-alsa-sync.

  • Replaced KnownCecAudioStatus::new with a method that takes a separate volume & is_muted parameter.
  • Added a TryFrom<u8> implementation to handle converting from a raw status (also adds a variant for the reserved value error case)

@kira-bruneau kira-bruneau force-pushed the fix-audio-get-status-result branch 2 times, most recently from f9f908e to 62d8dfe Compare September 13, 2024 12:37
Copy link
Owner

@ssalonen ssalonen left a comment

Choose a reason for hiding this comment

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

Thank you! Added few small colments

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
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.

2 participants