Add gain setting option to device settings
Opened this issue · 13 comments
@shockdesign @maks as I said in the PR, I think there is value to this and we can implement it, but in a different way. Right now it seems that it's not possible with one setting to cater to line out and headphone users, and I think there are different opinions among headphone users too. Want to make sure we don't rush changes that may be difficult to maintain in the future.
What I'm thinking is similar to some headphone amplifiers, provide a gain setting. This could be a dB setting that maps to a certain bit shift in the driver, or could be a potentially more user friendly option for "high/mid/low" where high is no bit shift and presumably used by line out users (I'd argue that even line out could be a bit attenuated). We'd have to see if 3 settings is enough, probably yes.
This brings to other issues that I wanted to tackle before tho:
- Device options screen: Right now some device level options are set in the project screen for lack of a better place, but eventually I think it would be good to have a separate screen for this, and this could go there. For the moment I think this option could go there as well for the moment and not be a big issue. (working on the additional device config level screen has other issues like memory usage, which I'm working on)
- Device level options saving: As I said in the PR, I want to discourage the config.xml file usage as it's not something I'm willing to support long term. I was thinking we could reserve some space in flash to save device level options. If we reserve some space at the end of the flash, it would survive reboots. This place, or adjacent to it could be used also to save the current project and make it survive reboots (but that's another discussion)
Mostly agreed @democloid .
I've made a new issue for a separate project settings page to remove need for config.xml. But I think that is a separate issue and shouldn't really block this one as all existing settings in config.xml would need to be migrated to a device settings screen anyway once its in place.
In the case of having configurable gain options though, I think there is a limitation in what can be done as I ran into not being able to use the ISR in the PIO asm to pass in a value for the offset and still get the timing right with sidesets for i2s framing hence my suggestion to @shockdesign to just use separate PIO asm files for the "headphone" vs "line level" settings. We could of course have a third file for a "medium" setting but I think more than that and using this method will get out of hand :-)
If you support config.xml, then when you switch you have to support code to migrate those settings to device level settings, or not do it which will frustrate users. I don't see a problem with taking things slower in order to do it right. I'd even release, potentially a line level firmware and a headphone one in the mean time.
Same goes for not being able to dynamically change these levels with the current code. It means it needs more work to get to the right approach, rather than rushing something out.
If we think that the attenuation is not ready for prime time, that's why this isn't a release version, we can turn this back and wait for the right approach.
To add to the points I made earlier, I think a third requirement should be to support changing these settings without having to reboot. This would require having the audio off while we do it, but that's fine as there are other settings that require such an approach.
@democloid As I mentioned in the PR, we can leave that one hanging for a moment while we work out a better approach..
I'm all for having a seperate settings page.. as for how we store that settings, I don't think we should store it in Flash, but rather write it out to the SDCard... like config.xml but maybe a better format (removing the xml parser would reduce some code indeed).. However lets discuss this in the correct issue.
I think it's okay to go through multiple revisions and various ideas as that follows the Agile approach which is handy with type of software.
I'm still looking at a better way to be able to have configurable output levels.. I was just hoping in the meantime this would be a good stop gap solution for those wanting a loud lineout for Studio equipment and those using headphones for day to day use.
I don't think having two versions of the software is a good idea. Especially if you are switching back and forth.. However happy to work with you and @maks to get a Settings page done, so we can toggle between the two for this PR before we think about releasing it.
Looking at other PIO code, it seems very good at doing one thing only. So in that sense, if we wanted to have multiple drivers for different levels, this would be the way to go (only with a better way of handling the differences).
In general I have to admit I'm also much more of the iterative development school too, my preference is to release things as early & quickly as possible to get user feedback as soon as possible vs taking longer to try to get something that is more polished but may not actually be a good fit once used widely.
Specifically with the audio output level here, for me the rush was concern with people not being able to use devices because the level was too high, so thats why I wanted to get the vol attentuation out asap. Now that is, I think its much less urgent to get a fix for too low level output as thats more an inconvenience vs something that stopped people even using it with some headphones/earbuds.
I'm perfectly ok with multiple revisions and pretty much think the same, but think I need to be careful to support features that people might come to expect and then removing them. In that sense, I think it's fine to iterate in the repo or some branch with half baked features, but the moment you make a release it might create expectations.
@democloid agreed people (including me!) get very annoyed when features are removed or get radically changed, the feedback I've had on replacing the import sample onscreen buttons with keys is as good an example of that as any!
So yes I think experimenting in the betas is ok for me and I'll try to make it clear here and on discord that thats what they are, experimental versions and not releases with features that will stay going forward.
OK here are my thoughts (and compromise):
- 3 gain settings, ok if we have 3 separate PIO files, but strive to unify later
- OK to use config.xml as backend for now, one more wont hurt
- settings should apply "on the fly", this is important IMO. it will necessarily require play/audition to be off
- OK to use project settings screen for it for the moment
need to define what bit shifting these gain settings correspond to
thoughts?
I think this is definitely workable..
3 seperate PIO files is easy enough to sort out.
Applying the settings on the fly is easy.. but yes, have to just check that the device isn't currently playing, etc before allowing a change.
Can pop this on the project settings screen..
I'm guessing the loudest setting will use the original code, but yes, @maks it would be good to get impact of what a low and medium setting would be?
If all of this is good, I can look at mocking up how the project settings screen would look with the options before building?
Thanks both for the great discussion on this!
This is just my 2c: yes loudest setting is original i2s code. The quiet setting is current master branch build 4bit offset setting.
The medium is the hard one, I think would be good to do 2bit and ask cTrix and others to compare with prev testing of the 3bit setting and see which one they prefer, I think this would be mainly for use with high impedance headphones as all line level use would be with the "full" setting.
Yeah, to me the obvious would be 0, 2 and 4. Potentially 1, 3 and 5 o 0, 2 or 3 and 5. A real low and a real high might be useful. The advantage of 0 is that it's true line level output that the amplifier outputs, so that's the only close to standard thing we have here.
I'm going to stick with the obvious settings for the moment, 0, 2, 4 defaulting to 4 (quietest).
Working on the display portion of this in project settings but reading/writing value from the Config.
@democloid instead of telling the user that the setting cannot be changed while playing? Would it be a better experience to stop audio if it's in use. Then change the value. Or do we want the user to actively think about the change?
I think it's important not to preempt playback. While this is not widely used now, say someone is playing this live or whatever, you don't want to perform an action that has unexpected consequences. Let's stick to displaying the same dialog that other project actions display.
@democloid Could you also assign this issue to me 🙏