Volume Updates #71
Volume Updates #71
Conversation
| return min(max(int(self._data["audio"].get("volume", 7)), 1), 20) | ||
| def audio_volume(self) -> float: | ||
| return min(max(float(self._data["audio"].get("volume", 0.35)), 0.0), 1.0) |
There was a problem hiding this comment.
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.
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.py that 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 data dictionary directly instead of the property, something like launcher_config.data["audio"].get("volume", 0.6)
Part of me was tempted to make these return None instead 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 for None and act accordingly if we did make that change though.
There was a problem hiding this comment.
Your argument here makes sense, although appending default_ to the constructor parameters would clear it up a little.
To follow on your None idea, one solution would be to allow assigning None within adafruit_fruitjam.peripherals.Peripherals.volume which 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. 👍
|
I think all of these volume updates are in place now. Ideally this should be merged at the same time as these PRs after review: |
3 participants
Made the boot animation louder by default, users can change the volume using
launcher.conf.json. This change only takes effect if there is no volume specified.Updated launcher config for float volume API change from adafruit/Adafruit_CircuitPython_FruitJam#22