adafruit / circuitpython

CircuitPython - a Python implementation for teaching coding with microcontrollers
https://circuitpython.org
Other
4.12k stars 1.22k forks source link

Add audiodelays & audioeffects to unix coverage port #9793

Closed jepler closed 2 weeks ago

jepler commented 2 weeks ago

.. and add a very basic audioeffects test, showing that it plausibly is working

I had to address several build errors that occurred in the Unix build, mostly related to conversion from FP types to integral types (replaced by explicit casts) and by accidental mixing of regular & f-suffixed floating constants (replaced with the MICROPY_FLOAT_CONST macro)

Particularly this change could use consideration:

-    self->max_echo_buffer_len = self->sample_rate / 1000.0f * max_delay_ms * (self->channel_count * sizeof(uint16_t)); // bytes
+    self->max_echo_buffer_len = (uint32_t)(self->sample_rate / 1000.0f * max_delay_ms) * (self->channel_count * sizeof(uint16_t)); // bytes

The buffer length is being calculated in floating point based on the millisecond delay & the sample rate. The result could then be a fractional number such as 529.2 for a 12ms delay at 44.1kHz. Multiplying a floating number by the size required for each echo buffer item ((self->channel_count * sizeof(uint16_t))) could yield a number of bytes that doesn't correspond to an integral number of buffer items. I grouped the float->int conversion so that it converts the number of echo buffer items to an integer and then multiplies by the size of the item.

ping @relic-se -- I've found it helpful to have host-based tests for synthio. this enables the same thing for filters and echo. Also, see above, maybe I found a bug?

relic-se commented 2 weeks ago

I think your solution to fix the implicit conversion will do the trick. There will be some imperceptible deviation from the actual ms value, but I think that's inevitable. Thanks for scanning through both modules and making those updates throughout. I'll try the filter tests out when I get the chance.

I see you haven't created a test for the audiodelays.Echo yet. When you do, could you make sure that it's testing in stereo? (ie: hard panned sine waves at different intervals) I'm worried that there might be some buffer mishandling when freq_shift=True that I haven't gotten around to investigating. Generating the test output may reveal that error.

jepler commented 2 weeks ago

I only created one test as an example. I'd hope that after this PR is merged you'd consider creating more tests; you're likely to do better at identifying what's important to test than me, given your familiarity with the code.

FoamyGuy commented 2 weeks ago

@jepler I tried testing this but I'm not sure that I've figured out the correct way to run it.

make test and make test_full inside of ports/unix/ runs many tests including this new one, but the new one is one of 42 tests that fail for me when I run them this way.

I briefly tried running the new test on it's own with

build-coverage/micropython ../../tests/circuitpython/audiofilter_filter.py

But that raises an ImportError not being able to find audiofilterhelper, which I see is commited along with this PR in testlib folder. I'm not sure how to get the micropython binary to find it there when run like this. As a workaround I tried copying that file into build-coverage/ but that still results in the same error.

jepler commented 2 weeks ago

You can run a single test with make VARIANT=coverage TEST_EXTRA=circuitpython/audiofilter_filter.py. Or, you can manually include testlib in the import path with MICROPYPATH=../../tests/testlib build-coverage/micropython ../../tests/circuitpython/audiofilter_filter.py