Skip to content

Add ALSA-based volume control and test logic#8

Open
WasabiFan wants to merge 7 commits intoev3dev:ev3dev-jessiefrom
WasabiFan:add-audio-controls
Open

Add ALSA-based volume control and test logic#8
WasabiFan wants to merge 7 commits intoev3dev:ev3dev-jessiefrom
WasabiFan:add-audio-controls

Conversation

@WasabiFan
Copy link
Member

This PR adds an "Audio" section in the main menu and associated sub-windows to allow volume control from brickman. The primary volume page looks like this:
volume control window

There is also a list window that only appears if there are no mixer elements or there is more than one mixer element. That window can be accessed from the desktop test.

That menu has the following options:

  • Volume up
  • Volume down
  • Half volume
  • Maximum volume
  • Minimum volume
  • Mute (this one is disabled in certain cases, including on the EV3 -- see below.)

This audio system is architected slightly differently from other current modules in brickman. I created a ITestableMixerElement interface (not a perfect name, but the best I could come up with) with two implementations: one of them is the AlsaBackedMixerElement and the other is the FakeMixerElement. The views operate on ITestableMixerElements, so the controllers just instantiate whichever implementation they need and hand it to the view and the view handles the common display logic.

The views connect to the notify signals on elements they are given, so they update automatically whenever the underlying elements change. This is mostly used by the test implementation, but it is also how the volume percentage on the main page operates: when the user hits a volume button, the view emits a signal; the controller then updates the volume value on the base element accordingly and the view gets the property change notification and updates the label.

Some notes:

  • The mixer that is configured by default on the EV3 (which may also apply to the other platforms) does not have a mute switch. This means that the mute toggle is always hidden in the volume control. While one can set the volume to zero, the speakers still emit audible clicks when sound starts playing. I have tested the mute functionality on my desktop PC and know that it works.
  • The standard ALSA notation for mixer elements is the name and index separated by a comma. It turns out that the font in brickman makes commas very hard to discern after certain letters, especially _k_s (which, unfortunately, is what "playback" ends in). So, after some fiddling, I decided on putting the index in brackets before the name -- if there are other ideas for how this should be formatted I can try those out too.
  • I'm pretty sure I didn't do the CMakeLists.txt additions correctly. Specifically, on line 139, I added asound after ${DEPS_LIBRARIES} because I couldn't figure out where that variable was coming from. Guidance on where that should go would be appreciated.

@dlech
Copy link
Member

dlech commented Jun 21, 2016

This PR adds an "Audio" section

How about calling it "Sound" instead?

That menu has the following options:

  • Volume up
  • Volume down
  • Half volume
  • Maximum volume
  • Minimum volume
  • Mute (this one is disabled in certain cases, including on the EV3 -- see below.)

I would rather just have volume up and down (eventually, we should just have a graphic slider, but text is OK for now). If we make the change step by 10%, then you don't really need the half/min/max. Also, if the volume is set to 0, then behind the scenes, it should activate the mute if it exists, so we don't need a separate mute option.

I created a ITestableMixerElement interface (not a perfect name, but the best I could come up with)

It sounds like you are doing MVVM in which case I would call it IMixerElementViewModel. However, this is not how any of the other views/controllers are implemented. In other controllers, we bind properties of the data model to the view rather than binding properties of a view model in the view as you are doing here. There's nothing wrong with doing it the way you have here, but I would rather stay consistent with the existing patterns.

By using the property binding feature of GObjects, we don't have to implement a function to attach to each notify signal. We only need a translate function for special cases like converting an enum to a string.

I added asound after ${DEPS_LIBRARIES} because I couldn't figure out where that variable was coming from. Guidance on where that should go would be appreciate

The alsa library is in pkg-config, so for now, you can add it to pkg_check_modules on line 120. Really some of those libraries should be separated out so that they are not used in the test version. You have to delete the cmake cache in order for it to pick up the libraries again.

@WasabiFan
Copy link
Member Author

WasabiFan commented Jun 21, 2016

How about calling it "Sound" instead?

OK, I'll change that.

I would rather just have volume up and down (eventually, we should just have a graphic slider, but text is OK for now) ... Also, if the volume is set to 0, then behind the scenes, it should activate the mute if it exists, so we don't need a separate mute option.

I think there needs to be either a mute option or a "minimum" option; having to hit the "volume down" button 10 times to mute the audio would probably get pretty annoying. How about I remove the mute button as it stands along with the half and max buttons and rename the "minimum" button to "Mute"? That way, you can still easily mute it, and it all fits on one page.

Barring a slider, I was originally hoping for something simple that would let you use the left/right buttons to modify the volume so that you didn't need to select the separate menu items. The current capabilities of ev3devKit weren't enough to lay that out without significant additions, however. We'll have to revisit this sound UI if/when someone implements a slider or other similar control.

It sounds like you are doing MVVM...

Yes, I aimed for something closer to MVVM or MVC because it seemed like there was a fair amount of manual UI management in the controller that I wanted to avoid. Essentially, I wanted to avoid the controller having any references to MenuItems and other UI elements -- the controller manages the data, and the view decides how to render those data.

I think I might go back and change it a bit to use the preexisting binding logic to bind the models to the views; in some cases, that might help clean up the code.

Edit: After looking through the code, I've decided I won't try to add bindings to replace notify signals. There isn't anywhere meaningful that it would help simplify the code. The biggest issues are with interfacing with the control panel GUI, and that is limited by Gtk+'s capabilities, not our code.

...in which case I would call it IMixerElementViewModel

Will-do.

The alsa library is in pkg-config, so for now, you can add it to pkg_check_modules on line 120.

Oh, I think I see what I did wrong -- I didn't clear the cache. After doing that, I was able to remove my other hack and it seems to be working.

@WasabiFan
Copy link
Member Author

PR updated.


namespace BrickManager {
public class AlsaBackedMixerElement: IMixerElementViewModel, Object {
private const SimpleChannelId primary_channel_id = SimpleChannelId.MONO;
Copy link
Member

Choose a reason for hiding this comment

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

Are you planning on changing this value? I don't see a need to. So, we don't really need a constant for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the equivalent of a "magic number" and is used in two places, so having it as a constant makes the code more readable and maintainable. While I don't intend to change it, I already had to change it a few times in development, so there's no reason to think I wouldn't need to change it again.

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.

3 participants