-
Notifications
You must be signed in to change notification settings - Fork 5
Volume Updates #71
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
Merged
Merged
Volume Updates #71
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Any thoughts on specifying the default volume in the constructor and using it here? Same goes for
audio_volume_override_danger.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.
I think I prefer not to have another place that the default volume and limits can be set.
I do see how it could be convenient in some situations for instance the changes in the PR to
boot_animation.pythat increase the volume if it isn't specified in the launcher config could be made a little bit simpler.But personally I think it also adds an extra source of potential confusion if someone is does something like
launcher_config = LauncherConfig(audio_volume=0.6)expecting that it alone will have an impact on the volume level.And it would mean that the default volume level inside of launcher config could then differ from the default value inside of fruit jam library here: https://github.com/FoamyGuy/Adafruit_CircuitPython_FruitJam/blob/volume_api_floats/adafruit_fruitjam/peripherals.py#L197.
In my mind any user code that wants to have a different default value for any of the configurations could instead use the
datadictionary directly instead of the property, something likelauncher_config.data["audio"].get("volume", 0.6)Part of me was tempted to make these return
Noneinstead of a default value if they aren't set in the file. That way this class is solely accessing information from the config file, not providing any of it's own values for anything. Code using it would need to check forNoneand act accordingly if we did make that change though.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.
Your argument here makes sense, although appending
default_to the constructor parameters would clear it up a little.To follow on your
Noneidea, one solution would be to allow assigningNonewithinadafruit_fruitjam.peripherals.Peripherals.volumewhich it would recognize and reset the volume to the default... but at that point, I think it's better just to handle it how you're doing here. 👍