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

drivers/periph/pdm: initial pdm implementation #20509

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

baharehf
Copy link

@baharehf baharehf commented Mar 26, 2024

Contribution Description

The pulse density modulation (PDM) module enables input of pulse density modulated signals from external audio frontends, for example, digital microphones. The PDM module generates the PDM clock and supports single-channel or dual-channel (left and right) data input. Data is transferred directly to RAM buffers using EasyDMA.

Testing procedure

I was working on PDM peripheral and utilized for Feather nRF52840 Sense board, conducting comprehensive some tests to validate itsperformance. after thorough examination, I successfully recorded from the PDM on the board and saw the result in the chart of python code and compared with Audacity application output.

  • Synchronization PDM testing in different buffer sizes
  • Record different frequencies and see the result in python chart
  • The expected success test output in Audacity

Issues/PRs references

based on #16125

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports labels Mar 26, 2024
@benpicco benpicco requested a review from aabadie March 26, 2024 23:17
Comment on lines 1 to 12
import matplotlib.pyplot as plt
import matplotlib.patches as mpatches
import re
import numpy as np
import os
import csv
from itertools import groupby
from operator import itemgetter
import serial
import struct
import wave
import time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are providing a script, maybe it is better to provide it in a tests sub-folder or maybe even somewhere in tools/dist?

It should also have some sort of requirements.txt so others can easily setup the and run it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Kevin for your guidance and assistance.
I will move the python script to the dist/tools folder and include the requirements.txt and readme.md files as well.

@MrKevinWeiss MrKevinWeiss self-assigned this Mar 28, 2024
@github-actions github-actions bot added Area: doc Area: Documentation Area: tools Area: Supplementary tools labels Mar 28, 2024
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a new naming convention for our test folder.

tests/periph_pdm -> tests/periph/pdm

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should also be a README.md in the test folder... even if it just points to the dist/tools/pdm_check tool.

@MrKevinWeiss
Copy link
Contributor

OK. So there were many thing that should still be looked at here. I push my work so people can see, cleanup is needed. Also @baharehf can drop my commits if needed.

The test that I adapted now tests different sample rates and it appears that they values are not that correct. Probably something to do with the ratio setting. This drove us to a rabbit hole of how to handle sample rates, wouldn't it be nice to get a way to iterate through possible sample rates (for testing purposes) or set arbitrary ones. We have this type of issue with the SPI driver as well.

Where we left it is (despite Nordics poor documentation) we should be able to set arbitrary sample rates and read back the real sample rate. This means extending the API.

So many things were found and fixed...
I changed the test application so we could verify sample rates.
I added tests that check the values of the sample rates and some sanity check of the actual samples
Added a few changes to the API to make it more testable, such as the variable sample rate and checking the real sample rate.
Implemented variable sample rate.
Cleaned up the python application to only use the output files and removed unneeded dependencies.
Updated the documentation...
@MrKevinWeiss
Copy link
Contributor

So I did some work on it. The commits will need to be cleaned up. There is (at least) one remaining point that I would like solved, there seems to be a recording artifact at the start of the recording. @MichelRottleuthner mentioned something about this, maybe we need a backoff value or something like that in the API?

I have also only tested on the feather-nrf52840-sense board... I don't know if other boards will behave in a different way (especially for the LUT). I also haven't tested stereo mode. Some arguments could also be made about best approximation of the sample rate implementation it is now.

As well as some cleanup and documentation of the variables (maybe too much magic number usage).

@github-actions github-actions bot removed the Area: build system Area: Build system label Apr 25, 2024
@github-actions github-actions bot added the Area: build system Area: Build system label Apr 30, 2024
@MrKevinWeiss
Copy link
Contributor

It would be nice to finish this off...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: drivers Area: Device drivers Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants