Fix test_i2s_basic_slave and test_slave_bclk_invert tests #103
Conversation
| static const unsigned bclk_freq_lut[NUM_BCLKS] = { | ||
| 1228800 | ||
| static const unsigned lr_freq_lut[NUM_BCLKS] = { | ||
| 192000 | ||
| }; | ||
| #else | ||
| #define NUM_BCLKS 12 |
There was a problem hiding this comment.
These two macros need updating and renaming
| @@ -10,6 +10,7 @@ | |||
|
|
|||
| num_in_out_args = { | |||
| "4ch_in,4ch_out": (4, 4), | |||
| "2ch_in,2ch_out": (2, 2), | |||
There was a problem hiding this comment.
I think you'll also need to update the CMakeLists to get these to build properly, as well as the .sh files that actually do the building
There was a problem hiding this comment.
The 2,2 configuration is already present in the makefile and the build_lib_i2s_tests.sh. I think they were being tested at some point before getting mysteriously removed?
There was a problem hiding this comment.
Oh that's exciting! Carry on then 😂
test/lib_i2s/i2s_slave_checker.py
Outdated
| @@ -173,6 +176,8 @@ def run(self): | |||
| xsi.drive_port_pins(self._bclk, bclk1) | |||
| time = self.wait_until_ret(time + clock_half_period) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Unnecessary line breaks. I might actually suggest that at this point, any Python files committed here should have Black run on them first to unify code style - I started doing this myself
| @@ -16,6 +16,10 @@ set(APP_LINK_OPTIONS | |||
| -target=XCORE-AI-EXPLORER | |||
| ) | |||
|
|
|||
| # Compile main.c which contains the i2s_callback_group_t functions in O3 mode. Needed for passing the | |||
| # test_i2s_basic_slave[4ch_in,4ch_out-32b] and test_i2s_basic_slave[4ch_in,4ch_out-16b] tests | |||
| set_source_files_properties(${CMAKE_CURRENT_LIST_DIR}/src/main.c PROPERTIES COMPILE_FLAGS "-O3") | |||
There was a problem hiding this comment.
This test is intended to check that the component is capable of operating using these settings, which it does - which is good. I would caution that this does highlight that the backpressure tolerance for the component in these settings is lower than we expected it to be - it would be useful to be able to profile these in the backpressure "test".
| 1228800, 614400, 384000, 192000, 44100, | ||
| 22050, 96000, 176400, 88200, 48000, 24000, 352800 | ||
| #define NUM_LRCLKS 12 | ||
| #define NUM_LRCLKS_TO_CHECK 3 |
There was a problem hiding this comment.
Approved, but, why are these 12 and 3? To be frank we could just make them 6 and 6, no point having clocks if we're not going to use them
https://xmosjira.atlassian.net/browse/AP-371
I've tested both smoke and nightly and the tests pass on my local setup (MacOS).
pytest lib_i2s/test_i2s_basic_slave.py -s
pytest lib_i2s/test_i2s_basic_slave.py --nightly -s