fix pdm issue with #352 #409
Conversation
hathach
commented
Jan 8, 2020
hathach
commented
- also add define for PIN_PDM_DIN, PIN_PDM_CLK, PIN_PDM_PWR
- add PDM setPins() for custom board
- also add define for PIN_PDM_DIN, PIN_PDM_CLK, PIN_PDM_PWR - add PDM setPins() for custom board
| #define PIN_PDM_PWR -1 | ||
| #endif | ||
|
|
||
| PDMClass PDM(PIN_PDM_DIN, PIN_PDM_CLK, PIN_PDM_PWR); |
There was a problem hiding this comment.
PDM is declared within the class like Serial or Wire. It is consistent with current usage of PDM from Arduino nRF528x-mbedos. Making using 3rd party library easier. Also this help to prevent hardfault or linking error on IRQ if PDM is not defined in application sketch.
Note: it requires to remove this line from CPlay library https://github.com/adafruit/Adafruit_CircuitPlayground/blob/master/utility/Adafruit_CPlay_Mic.cpp#L30 . I will submit an PR for this after this once is merged.
| PDMClass(int dataPin, int clkPin, int pwrPin); | ||
| virtual ~PDMClass(); | ||
|
|
||
| void setPins(int dataPin, int clkPin, int pwrPin); |
There was a problem hiding this comment.
add setPins(), will be useful for custom nrf52 board.
| /* | ||
| * PDM Interfaces | ||
| */ | ||
| #define PIN_PDM_DIN 24 |
There was a problem hiding this comment.
currently these pin are defined for CPB
|
|
||
| // The default Circuit Playground Bluefruit pins | ||
| // data pin, clock pin, power pin (-1 if not used) | ||
| PDMClass PDM(24, 25, -1); |
There was a problem hiding this comment.
removed, we could use PDM directly like we did with Serial
henrygab
left a comment
henrygab
left a comment
There was a problem hiding this comment.
I think this may not result in a working PDM library.
It appears that buffers are provided (via the callback), before they are actually filled by the DMA engine.
I'd like to better understand how using the NVIC disable / enable for the public APIs will (or will not) risk losing samples.
|
|
||
|
|
||
| // set the buffer for transfer | ||
| // nrf_pdm_buffer_set((uint32_t*)_doubleBuffer.data(), _doubleBuffer.availableForWrite() / (sizeof(int16_t) * _channels)); |
There was a problem hiding this comment.
With this still commented, the call a few lines later to trigger the NRF_PDM_TASK_START task will cause the peripheral to work on the nullptr buffer. This doesn't seem right....
| size_t avail = _doubleBuffer.available(); | ||
|
|
||
| //NVIC_EnableIRQ(PDM_IRQn); | ||
| NVIC_EnableIRQ(PDM_IRQn); |
There was a problem hiding this comment.
By disabling the PDM in the NVIC, wouldn't this potentially cause the silent loss of samples of data?
Especially with softdevice enabled, and Bluetooth active, which can pre-empt the execution here and cause unexpectedly long delays while this is still disabled?
| // enable and trigger start task | ||
| nrf_pdm_enable(); | ||
| nrf_pdm_event_clear(NRF_PDM_EVENT_STARTED); | ||
| nrf_pdm_task_trigger(NRF_PDM_TASK_START); |
There was a problem hiding this comment.
The register buffer pointer is still set to nullptr. Can you help me understand why this might be intentional?
@henrygab thank you very much for analyzing the PDM implementation, unfortunately I am not capable of answering your question since I just learned about PDM recently. I mostly ste..l hmm use the code from Arduino repo + code comparing. You should really file issue there to get answer from PDM writer. |
