Skip to content

fix pdm issue with #352 #409

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.

Sign up for GitHub

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

Merged
ladyada merged 2 commits into from
Jan 10, 2020
Merged

fix pdm issue with #352 #409

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6f7e41
fix pdm issue with #352
hathach Jan 8, 2020
f132517
fix build error with 52832 (no PDM Ratio register)
hathach Jan 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions libraries/PDM/examples/PDMSerialPlotter/PDMSerialPlotter.ino
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@

#include <PDM.h>

// The default Circuit Playground Bluefruit pins
// data pin, clock pin, power pin (-1 if not used)
PDMClass PDM(24, 25, -1);
Copy link
Member Author

@hathach hathach Jan 8, 2020

Choose a reason for hiding this comment

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

removed, we could use PDM directly like we did with Serial


// buffer to read samples into, each sample is 16-bits
short sampleBuffer[256];

Expand Down Expand Up @@ -41,8 +37,6 @@ void setup() {

void loop() {
// wait for samples to be read
PDM.IrqHandler();

if (samplesRead) {
// print samples to the serial monitor or plotter
for (int i = 0; i < samplesRead; i++) {
Expand All @@ -62,4 +56,5 @@ void onPDMdata() {

// 16-bit, 2 bytes per sample
samplesRead = bytesAvailable / 2;
}
}

56 changes: 39 additions & 17 deletions libraries/PDM/src/PDM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,23 +44,35 @@ PDMClass::~PDMClass()
{
}

void PDMClass::setPins(int dataPin, int clkPin, int pwrPin)
{
_dinPin = dataPin;
_clkPin = clkPin;
_pwrPin = pwrPin;
}

int PDMClass::begin(int channels, long sampleRate)
{
_channels = channels;

/*
// Enable high frequency oscillator if not already enabled
if (NRF_CLOCK->EVENTS_HFCLKSTARTED == 0) {
NRF_CLOCK->TASKS_HFCLKSTART = 1;
while (NRF_CLOCK->EVENTS_HFCLKSTARTED == 0) { }
}

// configure the sample rate and channels
switch (sampleRate) {
case 16000:
nrf_pdm_clock_set(NRF_PDM_FREQ_1032K);
NRF_PDM->RATIO = ((PDM_RATIO_RATIO_Ratio80 << PDM_RATIO_RATIO_Pos) & PDM_RATIO_RATIO_Msk);
nrf_pdm_clock_set(NRF_PDM_FREQ_1280K);
break;
case 41667:
nrf_pdm_clock_set(NRF_PDM_FREQ_2667K);
break;
default:
return 0; // unsupported
}
*/

switch (channels) {
case 2:
Expand All @@ -74,6 +86,7 @@ int PDMClass::begin(int channels, long sampleRate)
default:
return 0; // unsupported
}

setGain(DEFAULT_PDM_GAIN);

// configure the I/O and mux
Expand All @@ -82,8 +95,6 @@ int PDMClass::begin(int channels, long sampleRate)

pinMode(_dinPin, INPUT);

Serial.printf("Clock P %d Data P %d\n", digitalPinToPinName(_clkPin), digitalPinToPinName(_dinPin));

nrf_pdm_psel_connect(digitalPinToPinName(_clkPin), digitalPinToPinName(_dinPin));

// clear events and enable PDM interrupts
Expand All @@ -98,14 +109,13 @@ int PDMClass::begin(int channels, long sampleRate)
digitalWrite(_pwrPin, HIGH);
}


// clear the buffer
_doubleBuffer.reset();

// set the PDM IRQ priority and enable
NVIC_SetPriority(PDM_IRQn, PDM_IRQ_PRIORITY);
NVIC_ClearPendingIRQ(PDM_IRQn);
NVIC_EnableIRQ(PDM_IRQn);


// set the buffer for transfer
// nrf_pdm_buffer_set((uint32_t*)_doubleBuffer.data(), _doubleBuffer.availableForWrite() / (sizeof(int16_t) * _channels));
Copy link
Collaborator

@henrygab henrygab Jan 11, 2020

Choose a reason for hiding this comment

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

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....

Expand All @@ -116,7 +126,6 @@ int PDMClass::begin(int channels, long sampleRate)
nrf_pdm_event_clear(NRF_PDM_EVENT_STARTED);
nrf_pdm_task_trigger(NRF_PDM_TASK_START);
Copy link
Collaborator

@henrygab henrygab Jan 14, 2020

Choose a reason for hiding this comment

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

The register buffer pointer is still set to nullptr. Can you help me understand why this might be intentional?



return 1;
}

Expand All @@ -133,6 +142,8 @@ void PDMClass::end()
pinMode(_pwrPin, INPUT);
}

// Don't disable high frequency oscillator since it could be in use by RADIO

// unconfigure the I/O and un-mux
nrf_pdm_psel_disconnect();

Expand All @@ -145,7 +156,7 @@ int PDMClass::available()

size_t avail = _doubleBuffer.available();

//NVIC_EnableIRQ(PDM_IRQn);
NVIC_EnableIRQ(PDM_IRQn);
Copy link
Collaborator

@henrygab henrygab Jan 14, 2020

Choose a reason for hiding this comment

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

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?


return avail;
}
Expand All @@ -156,7 +167,7 @@ int PDMClass::read(void* buffer, size_t size)

int read = _doubleBuffer.read(buffer, size);

//NVIC_EnableIRQ(PDM_IRQn);
NVIC_EnableIRQ(PDM_IRQn);

return read;
}
Expand Down Expand Up @@ -203,13 +214,24 @@ void PDMClass::IrqHandler()
}
}

extern "C" {
void PDM_IRQHandler(void);
}

void PDM_IRQHandler(void)
extern "C"
{
PDM.IrqHandler();
void PDM_IRQHandler(void)
{
PDM.IrqHandler();
}
}

extern PDMClass PDM;
#ifndef PIN_PDM_DIN
#define PIN_PDM_DIN -1
#endif

#ifndef PIN_PDM_CLK
#define PIN_PDM_CLK -1
#endif

#ifndef PIN_PDM_PWR
#define PIN_PDM_PWR -1
#endif

PDMClass PDM(PIN_PDM_DIN, PIN_PDM_CLK, PIN_PDM_PWR);
Copy link
Member Author

@hathach hathach Jan 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

1 change: 1 addition & 0 deletions libraries/PDM/src/PDM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class PDMClass
PDMClass(int dataPin, int clkPin, int pwrPin);
virtual ~PDMClass();

void setPins(int dataPin, int clkPin, int pwrPin);
Copy link
Member Author

@hathach hathach Jan 8, 2020

Choose a reason for hiding this comment

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

add setPins(), will be useful for custom nrf52 board.

int begin(int channels, long sampleRate);
void end();

Expand Down
15 changes: 12 additions & 3 deletions variants/circuitplayground_nrf52840/variant.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ extern "C"
#define LED_STATE_ON 1 // State when LED is litted

// Buttons
#define PIN_BUTTON1 (4)
#define PIN_BUTTON2 (5)
#define PIN_BUTTON1 (4)
#define PIN_BUTTON2 (5)

// Microphone
#define PIN_PDM_DIN 24
Expand Down Expand Up @@ -120,7 +120,9 @@ static const uint8_t SCK = PIN_SPI_SCK ;
#define PIN_WIRE1_SDA (28)
#define PIN_WIRE1_SCL (26)

// QSPI Pins
/*
* QSPI Interfaces
*/
#define PIN_QSPI_SCK 29
#define PIN_QSPI_CS 30
#define PIN_QSPI_IO0 31
Expand All @@ -134,6 +136,13 @@ static const uint8_t SCK = PIN_SPI_SCK ;
#define USB_MSC_BLOCK_SIZE 512
#define USB_MSC_BLOCK_COUNT ((2*1024*1024) / USB_MSC_BLOCK_SIZE)

/*
* PDM Interfaces
*/
#define PIN_PDM_DIN 24
Copy link
Member Author

@hathach hathach Jan 8, 2020

Choose a reason for hiding this comment

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

currently these pin are defined for CPB

#define PIN_PDM_CLK 25
#define PIN_PDM_PWR -1 // not used

#ifdef __cplusplus
}
#endif
Expand Down