Skip to content

Fix audiomixer.Mixer regressions on SAMD51#10906

Draft
relic-se wants to merge 3 commits intoadafruit:mainfrom
relic-se:audiomixer-samd-fix
Draft

Fix audiomixer.Mixer regressions on SAMD51#10906
relic-se wants to merge 3 commits intoadafruit:mainfrom
relic-se:audiomixer-samd-fix

Conversation

@relic-se
Copy link
Copy Markdown

Updates made within #10529 may have caused audio corruption on the SAMD51 platform as reported by #10867. This update adds ARM-specific operations to copy16lsb and copy16msb to attempt to fix this regression.

This update has not yet been tested on the target hardware. Any assistance from @todbot or @gamblor21 would be greatly appreciated.

Fixes #10867

@gamblor21
Copy link
Copy Markdown
Member

Thinking out loud a bit here. I don't see anything wrong in your change, but I'm not sure the distortion would be fixed by just a speed up of what amounts to a basic bit copy (but it may).

I think there is an issue in the SAMD51 ASM code, with mult16signed. The old code would take a 32bit value HHHHLLLL and multiply both halves by loudness returning a 32-bit result.

The new code gets HHHHHHHH and then LLLLLLLL as two values (because of the copy16lsb and msb). So the result is HHHHloudness and HHHHloudness as a 32-bit value. and then the same for the LSB. So now we get HHHHHHHH and then LLLLLLLL.

I'm fighting a cold so maybe I missed something but I think the C code works but the ASM has something off in it.

That said I haven't had a chance to test your change on h/w and maybe my brain is making things up at the moment :)

@relic-se
Copy link
Copy Markdown
Author

Thinking out loud a bit here. I don't see anything wrong in your change, but I'm not sure the distortion would be fixed by just a speed up of what amounts to a basic bit copy (but it may).

Okay, that makes sense. I was unsure if there might be some issues with bitwise C operations on that platform that I wasn't aware of. I was able to test the pkhbt and pkhtb operands in an emulator with the desired result, so it probably won't hurt to keep that in.

I think there is an issue in the SAMD51 ASM code, with mult16signed. The old code would take a 32bit value HHHHLLLL and multiply both halves by loudness returning a 32-bit result.

The new code gets HHHHHHHH and then LLLLLLLL as two values (because of the copy16lsb and msb). So the result is HHHHloudness and HHHHloudness as a 32-bit value. and then the same for the LSB. So now we get HHHHHHHH and then LLLLLLLL.

I think you're on to something. I had forgotten that I modified mult16signed in c92efd6.

I'm fighting a cold so maybe I missed something but I think the C code works but the ASM has something off in it.

That said I haven't had a chance to test your change on h/w and maybe my brain is making things up at the moment :)

Sorry to hear that you're under the weather. 😿 As I said before, I did run the operation through an emulator, but I'm definitely no expert in ARM assembly. I still don't have hardware to test this, so whenever you get the chance, that would be awesome. Until then, I'll look into mult16signed.

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.

AudioMixer bad on Pygamer / SAMD51 on CirPy 10.x

2 participants