Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability save a wav recording #13299

Closed
wants to merge 1 commit into from

Conversation

lovettchris
Copy link

@lovettchris lovettchris commented Nov 30, 2023

This PR adds a AUDIO_SAVE_WAV that can be used to change the behavior of BUTTON_5 so that it saves a 10 second wav file recording of the PCM data being received over LE audio stream to the SD card.

This turned out to be a handy debugging tool when I was trying to record the exact PCM data without any microphone line noise that you get when you use an audio jack on the nrf5340 audio DK for recording.

It uses 2 1 second buffers so it can save from a thread in parallel with receiving new data over LE audio. This way it could save an unlimited amount of audio, currently hard coded to 10 seconds which is sufficient for most quick tests. Which buffer to save is communicated in a new zbus message to ensure that the task saves the right buffer.

You get the following logging output when you press the button:

HL [00:00:31.748,443] <inf> audio_datapath: Saved 320000 bytes to wav file.
HL [00:00:37.205,871] <inf> sd_card: abs path name:     /SD:/test.wav

There are a couple of open issues I'd love some help with:

  1. See bugbug in audio_datapath.c where I'm finding that "fs_seek" is not working. What I really want to do in write_wav_header is seek to the beginning of the file with fs_seek(wav_file, 0, FS_SEEK_SET); and that way I can "patch" the wav header at the end before closing the file because it is only really at that point that we know for sure how many bytes were written which is needed in the wav_size and data_bytes fields of the wave header. When I tried this code, the last call to write_wav_header did not modify the header instead it wrote another header to the end of the file. Is there some problem with fs_seek? There were no seek errors in the logs...

  2. I'm saving the output of sw_codec_decode but to be honest I didn't fully understand lines 1037-1052 in audio_datapath.c (some comments would be nice). This seems to be breaking up the resulting ctrl_blk.decoded_data into chunks. Not sure why, maybe that's just in preparation for how the i2s output to the headphones works? But when I save the ctrl_blk.decoded_data directly to the wav file I end up with twice as much data as expected and it plays back super slowly at very low pitch. I had to double the wav_file_hedaer.sample_rate for the audio to sound correct. I'm not sure why. For example, in this branch I'm recording at 16kHz, I had to wite sample_rate 32khz to my wav file. Perhaps sw_codec_decode returns stereo in a weird way even though this is a mono-channel at this point, and that's what lines 1037-1052 is dealing with? I don't know... any pointers would be appreciated.

  3. I see that the "main" branch seems to have moved to a new version of Zephyr, so I can't build main right now using the version that comes with v2.5.0. A pristine build of main gives me the error:

    warning: ZBUS_MSG_SUBSCRIBER (defined at F:/git/sdk-nrf/applications/nrf5340_audio\Kconfig.defaults:56) 
    defined without a type
    

    So where do I find the build/test instructions for the main branch?

Attached is a zip file containing the resulting wav files, the original without the sample_rate * 2 fix, then the fixed version. The fixed version then is only 5 seconds, whereas I was supposed to be saving 10 seconds of audio. Note that during this recording I was listening to the ear buds connected to the headset jack and the audio sounded great, so I know the LE audio was correct.

test.zip

@CLAassistant
Copy link

CLAassistant commented Nov 30, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 30, 2023
@NordicBuilder
Copy link
Contributor

Thank you for your contribution!
It seems you are not a member of the nrfconnect GitHub organization. External contributions are handled as follows:
Large contributions, affecting multiple subsystems for example, may be rejected if they are complex, may introduce regressions due to lack of test coverage, or if they are not consistent with the architecture of nRF Connect SDK.
PRs will be run in our continuous integration (CI) test system.
If CI passes, PRs will be tagged for review and merged on successful completion of review. You may be asked to make some modifications to your contribution during review.
If CI fails, PRs may be rejected or may be tagged for review and rework.
PRs that become outdated due to other changes in the repository may be rejected or rework requested.
External contributions will be prioritized for review based on the relevance to current development efforts in nRF Connect SDK. Bug fix PRs will be prioritized.
You may raise issues or ask for help from our Technical Support team by visiting https://devzone.nordicsemi.com/.

Note: This comment is automatically posted and updated by the Contribs GitHub Action.

@NordicBuilder NordicBuilder added the external External contribution label Nov 30, 2023
@koffes
Copy link
Contributor

koffes commented Dec 4, 2023

@lovettchris: Thanks a lot for this interesting and valuable feature!
We appreciate when external parties want to contribute :)

To be frank and set the expectations: This is a sizable new feature, and we must discuss internally if this is something we want to add, as well as support and maintain. There is a lot more to it than the code, as we must set up systems in CI to ensure that this feature does indeed write to the SD card and is not broken some time in the future.

I have started a CI run on this PR, and the results will be shown here.
Right off the bat, I see that some default Kconfig values are changed such as watchdog to "no", main stack size reduced, default sample rate etc. which must remain as they were.

@lovettchris
Copy link
Author

lovettchris commented Dec 4, 2023

Woops, I reverted the config changes I made while debugging, and hooked up the new read_wav_header helper function, and removing the duplicate struct wav_header definition. Happy to brainstorm on how to setup a test for this, I was thinking being able to save a wav file could be a handy way to setup whatever test you have for sd_card_playback_play_wav.

Copy link

github-actions bot commented Feb 5, 2024

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Feb 5, 2024
@github-actions github-actions bot closed this Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. external External contribution Stale
Projects
None yet
4 participants