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

#40-enable mute toggle. vol-up/dn also unmutes if needed #42

Merged

Conversation

pheck
Copy link
Contributor

@pheck pheck commented Apr 27, 2018

Successfully tested on my system.
Change describption: see #40

@MiczFlor
Copy link
Owner

Hi @pheck

thanks for the commit. Looks clean and working, one question, because I need to rely on your testing:

You set VOLSTEP to 5. And later you change the volume up and down with
(amixer sset \'$DEVICE\' ${VOLSTEP}+)

If you are not using % when changing the volume, wouldn't the value then be much bigger? I used 500+ and 500- originally.

Using % did not work for me, for reasons I don't understand. So I am happy not to see % :) but I am not sure that these are the right values?

All the best, micz

@itkevin
Copy link
Contributor

itkevin commented Apr 28, 2018

I think it depends on the Device you are using. I use a VOLSTEP of 10 on my Pi Zero as 255 is 100% on my device. My volume range is 0-255 on a Pi Zero with a Miniamp. So the 500+ step will always max out to 255 for me. A Pi 3 should have a much wider volume range. Hope that helps.

@pheck
Copy link
Contributor Author

pheck commented Apr 29, 2018

Yeah, like @itkevin pointed out, I also think it's device-depending.
On my Pi 3b with the USB sound card from your original item list, the absolute volume scale goes from 0-150.

(I had to change $DEVICE to "Speaker" to make it work in the first place, which also is a sign that things work a little differently depending on the system.)

Lastly, it's also a matter of (my) personal preference to have really fine-grained volume steps.

But I can as well change $VOLSTEP to 10 and update the PR, as this might be the better default for most people.
Your call, @MiczFlor :)

@MiczFlor
Copy link
Owner

MiczFlor commented May 2, 2018

Hi @pheck
I will get to this only end of the week, this week is very short in Berlin (first of May and all...). Until then, there is one thing which was brought up in the original feature post: could you change and test the path of the volume file into the /tmp folder? I will also make changes to have everything in that folder that is created for each session.
The only thing that this might require is a third variable like $defaultVolumeValue as a "preset", a starting point to read from. Something like:

if /temp/volume file does not exist
write $defaultVolumeValue into /temp/volume file

@pheck
Copy link
Contributor Author

pheck commented May 2, 2018

Hi @MiczFlor,
sure, no worries.
My bad, I missed the /var/tmp change in the actual PR. Will add/test that soon.

As for your second thought: if I got you right, you want to add a default value for the volume.

Imho, setting a default value only makes sense once - if ever: after the system starts. When unmuting, what I want is the same volume level like before I muted. And even after a system start, I'd prefer to have the volume level that I left with (it seems to be persistent across reboots) instead of a default.
Plus, technically, as the system always is at some volume level that can be read, not having a default wouldn't break anything either.

I might be short-sighted here, but I don't see a reason to implement a default.

@MiczFlor
Copy link
Owner

MiczFlor commented May 3, 2018

Hi @pheck
You are right. Let's keep the volume file intact. Because /tmp files are deleted on reboot I introduced a settings folder
https://github.com/MiczFlor/RPi-Jukebox-RFID/tree/master/settings
Could you alter your pull request to work with that folder? Not sure what this folder will also contain in the future, but it might make sense to have all settings in one place, like spotify etc.

When you git pull and work with the latest repo, please check if the changes to your volume file require sudo or not. I prefer if they don't.

Thanks for your input, makes me understand the usability issues better.

All the best, m

@MiczFlor
Copy link
Owner

MiczFlor commented May 7, 2018

HI @pheck
I might have lost you in my long thread of questions :) here is another one:
the volume level that is stored is only stored and changed if somebody uses the up and down option. If somebody "sets" the volume to e.g. 80% and then uses the volume down option, it will not reduce the value from 80% but from the last stored up / down value.
Could you make it work in such a way that it will "always" store the latest volume level in the vol file to read for the next action?

@MiczFlor MiczFlor mentioned this pull request May 7, 2018
@pheck
Copy link
Contributor Author

pheck commented May 9, 2018

Hi @MiczFlor,
True. I fixed the behaviour for setvolume like this: if present, setvolume now deletes the temporary volume file, so any subsequent volup/dn commands will work correctly, i.e. based on the current, not the stored volume.

Will have a look at the settings directory now.

@pheck
Copy link
Contributor Author

pheck commented May 9, 2018

Hi @MiczFlor,
while checking the settings-directory, I had another look at the previous comments and I think there might have been a mistunderstanding: the volume level itself is already preserved accross reboots.
So, there's no need to save/restore it manually. And in turn, no need to move the volume-file to a non /tmp location like "settings". (let alone hooking into some "before shutdown/reboot" and "after startup" events to do the actual save/restore at the right time)

Imho, the current implementation covers all aspects, which sum up to:

  • mute command toggles between mute/unmute, unmuting to the volume before mute
  • if muted, volup/dn commands both also unmute before increasing/decreasing the volume
  • setvolume command also deletes any temporarily stored volume level
  • temp file to store volume level goes into tmp directory as suggessted by ralle12345
  • bonus: the system starts with the same volume level it had before reboot/shutdown (system default)

Please review and let me know if you like me to change anything.

@MiczFlor
Copy link
Owner

MiczFlor commented May 9, 2018

Hi @pheck looks good and it will be next on my list of things to do. Promise (apologies for the delay). What I will be doing: moving $DEVICE, $VOLUME and $VOLSTEP into the settings folder as files with a value as content. the /tmp/ issue that was brought up earlier is not needed then anymore ( location would be /tmp/ as this is the system temp folder).
Thanks again! I will come back with questions, I am sure :)

@pheck
Copy link
Contributor Author

pheck commented May 9, 2018

Hi @MiczFlor ,
sure, sounds great :)

And good idea to pull out the settings into a central place.
As these tend to grow over time, it might be worth thinking about using a config file (.sh) that just sets environment variables as needed, and include it into playout_controls.sh and where ever else it's appropriate.

@MiczFlor MiczFlor changed the base branch from master to pr-testing-volume-toggle May 11, 2018 12:41
@MiczFlor MiczFlor merged commit d065dbc into MiczFlor:pr-testing-volume-toggle May 11, 2018
@pheck pheck mentioned this pull request May 23, 2018
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.

None yet

3 participants