This post is older than 2 years and might not be relevant anymore
More Info: Consider searching for newer posts

Possible bug in the I2S driver (nrf_drv_i2s.c) up to SDK 13?

Hi,

when looking at nrf_drv_i2s.c in SDK v13.0.0, the description of nrf_drv_i2s_start() states that the parameter buffer_size is Size of the buffers (in 32-bit words).

I don't generally like mixing up size, length, count, and capacity, and I would agree with the selected best answer in this thread, concluding that when speaking of size (of buffers or variables in general), then it should mean the same as what sizeof() would return, that is, in bytes.

Since I have been struggling with I2S on nRF52 for quite a long time, I have looked into the source code of nrf_drv_i2s.c and I believe, there is an error attributable to a mix-up as described above. Within the function nrf_drv_i2s_start() there is the following section:

if (m_cb.p_tx_buffer != NULL)
{
    // Get from the application the first portion of data to be sent - we
    // need to have it in the transmit buffer before we start the transfer.
    // Unless the synchronized mode is active. In this mode we must wait
    // with this until the first portion of data is received, so here we
    // just make sure that there will be silence on the SDOUT line prior
    // to that moment.
    if (m_cb.synchronized_mode)
    {
        memset(m_cb.p_tx_buffer, 0, buffer_size);
    }
    else
    {
        m_cb.handler(NULL, m_cb.p_tx_buffer, m_cb.buffer_half_size);
    }
}

Variable buffer_size is holding the capacity of the TX buffer (in 32-bit words) at this point. Using a memset() with this number will zero out only one quarter of the allocated memory space, so that a click will most probably be heard at start.

This is -of course- no critical error, but it would be nice to fix.

And if the driver is going to be fixed, it would also be nice to change the expressions including sizeof(p_data_to_send) and sizeof(p_data_received), since p_data_to_send and p_data_received are defined as uint32_t*, so the results of sizeof() calls will have nothing to do with the underlying data type uint32_t (which the author of the code probably had in mind), but with the size of a pointer in the 32-bit ARM system. (Fortunately, both equals to 4 in the given case, so we don't have an error here.)

Thanks, Tamas

Related