search

freeDSP / freeDSP-aurora

freeDSP ADAU1452 with 8 analog input, 8 analog outputs, S/P-DIF I/O, ADAT I/O, USB Audio Class2, WiFi, Bluetooth
Creative Commons Attribution Share Alike 4.0 International
176 stars 55 forks source link

fix issue #16 by adding a 250ms delay #56

Closed archi closed 3 years ago

zas commented 4 years ago

I still don't get why it works. The delay is before unmute command, but after something, imho the right place isn't here. Since the delay is short, it doesn't impact user experience much, so that's fine, but I'd rather look for the real cause of the issue.

@archi : you should squash those 2 commits (keeping commit message of the first one, but the change of the second one)

archi commented 4 years ago

@zas I am not a fan of squashing: Partly because sometimes it hides how code developed over time, e.g. it tends to hide the author's thought process. In this instance I was voice chatting with @dspverden while authoring the commit and wasn't really focused (resulting in the first commit being bad). Same goes for rebasing over merge. But this is kind of a philosophical/religious issue in my eyes, and if others prefer to squash their commits or rebase then that's fine with me :)

For review, you can use the above "Files changed" button, which shows the same as a squashed commit.

Regarding the real topic here: It works because the PEQ's transfer function's parameters seem to cause awkward artefacts, which then decay over time (I think they're essentially a FIR? or IIR?) - and this change gives enough time for that decay to happen. Until someone found the "real cause" (which I suppose is "garbage parameters in, garbage signal out"), this is the best I can do about it. In my case there are >200 Euro of tweeters on the line (and I'm sure they're still pretty cheap compared to other people's speakers), so I'm really interested in having this in master until the "real cause" is found & fixed ;-)

Also, I'm afraid that this "someone" to find the "real cause" isn't me, since I did not take any signal processing lectures and wouldn't even know where to start ;-)

zas commented 4 years ago

@zas I am not a fan of squashing: Partly because sometimes it hides how code developed over time, e.g. it tends to hide the author's thought process. In this instance I was voice chatting with @dspverden while authoring the commit and wasn't really focused (resulting in the first commit being bad). Same goes for rebasing over merge. But this is kind of a philosophical/religious issue in my eyes, and if others prefer to squash their commits or rebase then that's fine with me :)

In general, I wouldn't ask for squashing commits that make sense, but in this case, there's absolutely no value in keeping 2 commits for such a trivial change. Keeping main branches history clean help a lot, and in this case you shouldn't have 2 commits to start with. Especially since the first one has zero effect on the issue this PR is supposed to solve.

Also you should rather make PR title clear about what it does, and use useful commit messages (the second one gives no info about what the change does).

But it's up to you, and I'm not maintainer of this project, just sharing experience.

For review, you can use the above "Files changed" button, which shows the same as a squashed commit.

Ahah, thanks for the advice, but I'm aware of this feature being a daily git and github user since they appeared (https://repo.or.cz/elinks.git/commit/67030f03404f3d9ebe0695b209d87054026781c8 proves I was using git back to 2005, even before git 1.0).

I perfectly understood the patch "protects" from bad things, and since it doesn't impact user too much it is fine for me, but it doesn't address the core issue, which is yet to be determined. I would name it a "workaround" instead of a "fix"' for this reason.

@dspverden : any idea about how to identify the actual source of the issue? It seems to me delay before unmuting shouldn't be needed.