drivers/periph/pdm: add PDM API and nrf52 driver - #22710
Maulbeere01 wants to merge 22 commits into
Conversation
Co-authored-by: Bahareh <baharehfatemi@gmail.com>
Co-authored-by: Bahareh <baharehfatemi@gmail.com>
replace the fixed pdm_sample_rate_t enum, which only offered a few hardcoded PDMCLKCTRL presets, with a variable rate in Hz. PDMCLKCTRL maps non-linearly on the resulting clock divisor, so add a LUT plus rounding logic to pick the bitfield that reaches the closest achievable divisor/ratio combination for the requested rate. Co-authored-by: MrKevinWeiss <weiss.kevin604@gmail.com>
tests that the pdm peripheral actually runs at the configured sample rate by comparing it against the physically measured rate. Add a pdm_to_wav tool to convert a recording into a plot + audio file. Co-authored-by: MrKevinWeiss <weiss.kevin604@gmail.com> Co-authored-by: Bahareh <baharehfatemi@gmail.com>
the LUT mapped each achievable divisor to its PDMCLKCTRL bitfield. From the nRF5340 PS we can derive the formula: bitfield = 1024 / divisor, which replaces the LUT, making the code easier to understand and reducing magic numbers. behaviour stays the same, confirmed via the sample rate test that measured rates are unchanged across the whole achievable range.
per PS, restarting before the STOPPED event is received after TASKS_STOP may result in unpredictable behaviour.
|
Hey @Maulbeere01, thank you for your first contribution! We really appreciate it! If you haven't already, please take a look at our contributing guidelines before the review process starts. Also, due to how the GitHub review system works, please avoid force-pushing or squashing your commits unless asked to by a maintainer (or unless your commit is still in "draft commit" stage). Lastly, make sure to comply with our AI Policy when using AI. Your pull request will be reviewed as soon as possible. |
github-actions
Bot
added
Platform: ARM
crasbe
added
CI: ready for build
|
Thank you for picking this up and thank you for your first contribution to RIOT! Is there a specific reason this is marked as Draft still? |
AnnsAnns
added
AI: Helped
|
Thanks! No real reason, was just being cautious since this is my first PR. Its ready from my side, marking it as ready for review now. |
crasbe
left a comment
crasbe
left a comment
There was a problem hiding this comment.
If you could add support for the seeedstudio-xiao-nrf52840-sense as well, that would be awesome. That's the board I have here which also has a PDM microphone.
This is the schematic: https://files.seeedstudio.com/wiki/XIAO-BLE/Seeed_Studio_XIAO_nRF52840_PDF.pdf
You'll have to extend boards/seeedstudio-xiao-nrf52840-sense/board.c like this:
diff --git a/boards/seeedstudio-xiao-nrf52840-sense/board.c b/boards/seeedstudio-xiao-nrf52840-sense/board.c
index e52ff54598..3cf23db72f 100644
--- a/boards/seeedstudio-xiao-nrf52840-sense/board.c
+++ b/boards/seeedstudio-xiao-nrf52840-sense/board.c
@@ -25,20 +25,32 @@
void board_init(void)
{
- /* The IMU is supplied through a GPIO Pin (P1.08), so it has to be set
- * to high power mode. */
- gpio_conf_t lsm6ds3_pwr_pin_conf = {0};
+ /* The IMU is supplied through a GPIO Pin (P1.08) and the microphone through
+ * Pin (P1.10), so they have to be set to high power mode. */
+ gpio_conf_t periph_pwr_pin_conf = {0};
- lsm6ds3_pwr_pin_conf.state = GPIO_OUTPUT_PUSH_PULL; /* Set the output to push pull */
- lsm6ds3_pwr_pin_conf.drive_strength = GPIO_DRIVE_STRONG; /* Enable high drive strength H0H1 */
+ periph_pwr_pin_conf.state = GPIO_OUTPUT_PUSH_PULL; /* Set the output to push pull */
+ periph_pwr_pin_conf.drive_strength = GPIO_DRIVE_STRONG; /* Enable high drive strength H0H1 */
/* Power on the IMU if used */
if (IS_USED(MODULE_LSM6DSXX)) {
- lsm6ds3_pwr_pin_conf.initial_value = true;
- } else {
- lsm6ds3_pwr_pin_conf.initial_value = false;
+ periph_pwr_pin_conf.initial_value = true;
+ }
+ else {
+ periph_pwr_pin_conf.initial_value = false;
}
gpio_ll_init(gpio_get_port(LSM6DS3_PWR_PIN),
- gpio_get_pin_num(LSM6DS3_PWR_PIN), lsm6ds3_pwr_pin_conf);
+ gpio_get_pin_num(LSM6DS3_PWR_PIN), periph_pwr_pin_conf);
+
+ /* Power on the PDM microphone if used */
+ if (IS_USED(MODULE_PERIPH_PDM)) {
+ periph_pwr_pin_conf.initial_value = true;
+ }
+ else {
+ periph_pwr_pin_conf.initial_value = false;
+ }
+
+ gpio_ll_init(gpio_get_port(PDM_PWR_PIN),
+ gpio_get_pin_num(PDM_PWR_PIN), periph_pwr_pin_conf);
}and add the following to boards/common/seeedstudio-xiao-nrf52840/include/board.h:
diff --git a/boards/common/seeedstudio-xiao-nrf52840/include/board.h b/boards/common/seeedstudio-xiao-nrf52840/include/board.h
index 04b924818b..a7ba8d9d5d 100644
--- a/boards/common/seeedstudio-xiao-nrf52840/include/board.h
+++ b/boards/common/seeedstudio-xiao-nrf52840/include/board.h
@@ -96,6 +96,34 @@ extern "C" {
/** @} */
#endif
+#if defined(BOARD_SEEEDSTUDIO_XIAO_NRF52840_SENSE) || defined(DOXYGEN)
+/**
+ * @name PDM configuration (Sense variant only!)
+ * @{
+ */
+/**
+ * @brief GPIO pin for the data line
+ */
+# ifndef PDM_DIN_PIN
+# define PDM_DIN_PIN GPIO_PIN(0, 16)
+# endif
+
+/**
+ * @brief GPIO pin for the clock
+ */
+# ifndef PDM_CLK_PIN
+# define PDM_CLK_PIN GPIO_PIN(1, 0)
+# endif
+
+/**
+ * @brief GPIO pin that powers the PDM microphone
+ */
+# ifndef PDM_PWR_PIN
+# define PDM_PWR_PIN GPIO_PIN(1, 10)
+# endif
+/** @} */
+#endif /* defined(BOARD_SEEEDSTUDIO_XIAO_NRF52840_SENSE) || defined(DOXYGEN) */
+
/**
* @name ztimer configuration values
* @{and this to the boards/common/seeedstudio-xiao-nrf52840-sense/periph_conf.h:
diff --git a/boards/common/seeedstudio-xiao-nrf52840/include/periph_conf.h b/boards/common/seeedstudio-xiao-nrf52840/include/periph_conf.h
index be43482393..de0f9547b3 100644
--- a/boards/common/seeedstudio-xiao-nrf52840/include/periph_conf.h
+++ b/boards/common/seeedstudio-xiao-nrf52840/include/periph_conf.h
@@ -95,6 +95,17 @@ static const i2c_conf_t i2c_config[] = {
#define I2C_NUMOF ARRAY_SIZE(i2c_config) /**< Number of (preconfigured) I2C Buses */
/** @} */
+#if defined(BOARD_SEEEDSTUDIO_XIAO_NRF52840_SENSE) || defined(DOXYGEN)
+/**
+ * @brief PDM pin configuration (Sense variant only!)
+ */
+static const pdm_conf_t pdm_config = {
+ .din_pin = PDM_DIN_PIN,
+ .clk_pin = PDM_CLK_PIN,
+};
+/** @} */
+#endif defined(BOARD_SEEEDSTUDIO_XIAO_NRF52840_SENSE) || defined(DOXYGEN)
+
#ifdef __cplusplus
}
#endif| /** | ||
| * @brief Structure for PDM configuration data | ||
| */ | ||
| typedef struct { | ||
| uint8_t din_pin; /**< DIN pin */ | ||
| uint8_t clk_pin; /**< CLK pin */ | ||
| } pdm_conf_t; |
There was a problem hiding this comment.
I'd move this part further up, below the UART_TXBUF_SIZE define.
This keeps the defines and typedefs together and the function prototypes together.
| #define ENABLE_DEBUG 0 | ||
| #include "debug.h" | ||
|
|
||
| #define ABS_DIFF(x, y) (((x) < (y)) ? ((y) - (x)) : ((x) - (y))) |
There was a problem hiding this comment.
There is probably a function for this somewhere already, I'd prefer to use something already existing than to create something new.
|
|
||
| #define ABS_DIFF(x, y) (((x) < (y)) ? ((y) - (x)) : ((x) - (y))) | ||
|
|
||
| #define PDM_SRC_CLOCK_HZ 32000000 |
There was a problem hiding this comment.
Is this fixed by the hardware or can it be configured?
In any case, a documentation string would be good.
| /* PDM clock = PDM_SRC_CLOCK_HZ / divisor. Keeps the clock inside a typical | ||
| * PDM MEMS mic clock range of [1.0, 3.25] MHz */ |
There was a problem hiding this comment.
What is the "typical clock range" based on?
There was a problem hiding this comment.
This is a mistake on my side. The range was based on the mics I looked at (feather-sense, clue, my external mics) and they all had this range. Looking at your seeedstudio-sense mic datasheet, its actually 1.3-4.8 MHz for this one.
How should I fix this? I see 2 possible solutions:
-
Implement a configurable mic range per board and derive the divisor range from that.
-
If you could test whether the seeedstudio-sense works for the current range of 1.0-3.25 MHz, then it could stay like that and I just change the documentation. Context being, I tested beyond the documented range on the feather-sense and the microphone worked from 0.8-4 MHz. If the seeedstudio-sense mic has similar margin it should be fine with the current 1 MHz floor even though that is below its 1.3 MHz datasheet minimum.
Interestingly as I tested divisor 7 (on the feather-sense), which results in 4.57 MHz, the board wouldnt respond (no data at all), while divisor 8 (4.0 MHz) still works fine. Not sure if thats a PDM module limit or specific to my mic though. If its the limit of the PDM module, then the full range of the seeedstudio-sense mic cannot be supported anyways. If I run into the limit on the lower bound (lower than 0.8 MHz) there is no hard lock. The test just fails because the configured and measured rate fall out of tolerance.
| * @param[in] arg context passed to the callback function | ||
| * | ||
| * @retval >0 actual configured PDM sample rate in Hz on success | ||
| * @retval <0 on error |
There was a problem hiding this comment.
It would be good to mention the actual errnos returned, such as -ENOTSUP for example.
| make flash test > /tmp/pdm && \ | ||
| ../../../dist/tools/pdm_to_wav/pdm_to_wav.py \ | ||
| /tmp/pdm \ | ||
| --output-file /tmp/output |
There was a problem hiding this comment.
| make flash test > /tmp/pdm && \ | |
| ../../../dist/tools/pdm_to_wav/pdm_to_wav.py \ | |
| /tmp/pdm \ | |
| --output-file /tmp/output | |
| make flash test > /tmp/pdm | |
| ../../../dist/tools/pdm_to_wav/pdm_to_wav.py /tmp/pdm --output-file /tmp/output |
Did you execute it like that? Because that will also put all the make output into /tmp/pdm, which might confuse the JSON parser I guess?
There was a problem hiding this comment.
Yes, always worked for me. Looking at pdm_to_wav.py it parses line by line and skips anything that isnt valid JSON, so make output mixed into the file shouldnt cause a problem.
| then open the `/tmp/output.wav` to verify, if `matplotlib` is installed you | ||
| can also check `/tmp/output.png` |
There was a problem hiding this comment.
| then open the `/tmp/output.wav` to verify, if `matplotlib` is installed you | |
| can also check `/tmp/output.png` | |
| After that you can open the `/tmp/output.wav` file to verify the recording and | |
| if `matplotlib` is installed you can also check `/tmp/output.png` to see a | |
| visual plot of the data. |
| #ifndef PDM_DIN_PIN | ||
| /** | ||
| * @brief GPIO pin for the data line | ||
| */ | ||
| # define PDM_DIN_PIN GPIO_PIN(0, 0) | ||
| #endif |
There was a problem hiding this comment.
| #ifndef PDM_DIN_PIN | |
| /** | |
| * @brief GPIO pin for the data line | |
| */ | |
| # define PDM_DIN_PIN GPIO_PIN(0, 0) | |
| #endif | |
| /** | |
| * @brief GPIO pin for the data line | |
| */ | |
| #ifndef PDM_DIN_PIN | |
| # define PDM_DIN_PIN GPIO_PIN(0, 0) | |
| #endif |
Doxygen runs a preprocessor similar to what GCC or LLVM would do. The PDM_DIN_PIN would not be defined for this file, because only the only defines that Doxygen cares about are the ones set in this file or the ones set gloablly in doc/doxygen/riot.doxyfile.
Therefore, the preprocessor will reduce this to #define PDM_DIN_PIN GPIO_PIN(0,0), which can be evaluated by Doxygen no problem.
Long story short: it's cleaner to have the documentation block outside of the #ifndef.
Please also adapt the lines below as well as the lines for the adafruit-feather-nrf52840-sense.
There was a problem hiding this comment.
Also, the pin definitions are usually in boards/.../include/board.h, so please move them there. The pdm_config structure can (and should) remain in periph_conf.h.
There was a problem hiding this comment.
Thanks for the review and the explanation on Doxygens inner workings! Moving the pin definitions caused a compile error (PDM_DIN_PIN undeclared), board.h included periph/gpio.h, which pulls in periph_conf.h before the pin values were defined further down in board.h. I removed periph/gpio.h from board.h since it looked unused there. Had to do this for the seeedstudio-sense and feather-sense. Murdock is green on both. Fine as it is now or would you do it differently?
Co-authored-by: crasbe <crasbe@gmail.com>
6 participants
Contribution description
This proposes an API for PDM (Pulse density modulation) peripherals and an implementation for nrf52 MCUs. The PDM module on the nrf52 takes in a 1-bit density-modulated bitstream, in practice mostly from a digital microphone, and uses hardware decimation filters to produce PCM samples, supporting mono or stereo. The samples are then streamed to RAM via EasyDMA.
The driver implementation approximates the requested sample rate by choosing the PDM clock divisor and decimation ratio closest to it and returns the rate it actually configured.
The feature is enabled for the feather-sense and clue, but I have only tested on the feather-sense. The clue was enabled in a prior PR that this PR is based on and I have since made adjustments to the driver. It should still work on the clue, but I cant test it.
About the startup artifact...
Every recording begins with a startup transient before the signal settles. This artifact can last up to a few hundred ms, with its length and form depending on the config. I will share here what I have found/tried out so far, but I dont think there is much that can be done in software to prevent this. I have also verified this against the Adafruit/Arduino PDM library, it produces the exact same artifact.
First, I tied DIN to GND, so what you see below is only the PDM peripherals filter behavior, without any microphone signal.
The artifact in the first 50 samples is the filter starting up. Per PS these initial 50 samples should be discarded, so I implemented a backoff with a default of 5 ms, which covers that.
The slow curve back to zero afterwards is the filter finding its baseline. The peripheral keeps the audio centred on zero, meaning it removes any constant offset. None of it is actual audio, just the filter settling, so it is harmless. A software high-pass could remove this later at the application level if wanted.
But in combination with the microphone startup:
Before the spike the output is identical to the DIN to GND recording. After the spike the output settles back to baseline with the same shape, just mirrored. This is the filter settling again, not real audio yet. The drop at around 160 ms marks the point where real audio data starts coming in.
Also higher gains can run the spike into clipping of the 16 bit values, resulting in an even longer artifact.
So there is no single optimal default backoff for every possible config.
I have tested on multiple feather-sense boards, they all produce the same artifact. Also my stereo setup uses two external microphones and both have the same artifact. I think the problem is that the microphone startup falls into the filter settling window. The microphone only wakes up after receiving the clock pulse and that is only created after TASKS_START, which also turns on the filter, so the two overlap.
To be clear, I think this is a hardware limitation. The Adafruit/Arduino PDM library shows the exact same startup behavior, and it uses Nordics own nrfx driver underneath.
For applications that need clean samples from the start, the backoff default can be overridden to fit their config.
But if anyone has a better understanding of whats going on here, or an idea what I could still try, let me know!
Testing procedure
I have tested on the adafruit-feather-nrf52840-sense.
Mono, onboard mic:
make -C tests/periph/pdm flash testRecords, prints the samples and then sweeps the sample rate and asserts the configured rate matches the measured one.
Output of the rate sweep after the recording dump should look like this:
To actually listen to and view the recording:
make -C tests/periph/pdm flash test | tee /tmp/pdm && dist/tools/pdm_to_wav/pdm_to_wav.py /tmp/pdm --output-file /tmp/out/tmp/out.wavto listen,/tmp/out.png(needs matplotlib) to view.I also tested stereo with two external PDM MEMS Breakout Microphones, works:
Two recordings, first I covered the left microphone and then the right one.
Issues/PRs references
Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are:
The API and driver implementation itself was written by the previous authors and me.