Beware that this post is related to an SDK in maintenance mode
More Info: Consider nRF Connect SDK for new designs
This post is older than 2 years and might not be relevant anymore
More Info: Consider searching for newer posts

nrfx_saadc_abort() seems to be broken

Hi,

I'm new to nRF52 and the SDK, trying a few things to learn, so might be missing something obvious, but I've been looking at this apparent issue for a few days now.

My test setup:

  • nRF52840 DK
  • SDK v16.0.0, 15.3.0 and 15.2.0 (see below)
  • no soft device

I'd put together a simple program based on the SDK 16 SAADC example, with a timer driving sampling via PPI.  All working well.  But then I thought I'd add ADC calibration, which seems to be very sparingly documented in the PS and SDK (burst mode is pretty light too, by the way...).  So I wasn't surprised when my code didn't initially work.  Google to the rescue, found the saadc_low_power example at https://github.com/NordicPlayground.  Excellent, just what I was looking for.  Studied the code, thought I understood it, and was then a bit frustrated when I kept getting an assert failure from nrfx_saadc_abort(), which I was running prior to calibration, as is done in Nordic's saadc_low_power example.

Ok, so maybe something I didn't understand after all, so I grabbed the saadc_low_power example and built it for SDK 16.0.0, basing it on the sdk_5.2.0_migration branch.

But still the same issue, the assert was failing in:

void nrfx_saadc_abort(void)
{
    if (nrfx_saadc_is_busy())
    {
        nrf_saadc_event_clear(NRF_SAADC_EVENT_STOPPED);
        nrf_saadc_int_enable(NRF_SAADC_INT_STOPPED);
        nrf_saadc_task_trigger(NRF_SAADC_TASK_STOP);

        if (m_cb.adc_state == NRF_SAADC_STATE_CALIBRATION)
        {
            m_cb.adc_state = NRF_SAADC_STATE_IDLE;
        }
        else
        {
            // Wait for ADC being stopped.
            bool result;
            NRFX_WAIT_FOR((m_cb.adc_state == NRF_SAADC_STATE_IDLE), HW_TIMEOUT, 0, result);
            NRFX_ASSERT(result);
        }

        nrf_saadc_int_disable(NRF_SAADC_INT_STOPPED);

        m_cb.p_buffer           = 0;
        m_cb.p_secondary_buffer = 0;
        NRFX_LOG_INFO("Conversion aborted.");
    }
}

This indicated that m_cb.adc_state == NRF_SAADC_STATE_IDLE failed to become true, after checking HW_TIMEOUT times.

But it's supposed to, because we've triggered a NRF_SAADC_TASK_STOP task, and enabled the SAADC interrupt on the stop event.  The IRQ then updates m_cb.adc_state to indicate that the SAADC is no longer busy and we're ok to start a calibration.  That's the theory, right?  But it wasn't happening.  Back to Google.

There's at least one ticket similar to this, mentioning compiler optimisation and the need to ensure that m_cb.adc_state is volatile.  Yes, it's declared as volatile, and in any case I was running in debug mode, so the code can't have been optimised out, right?  Sure, I could check the disassembly, but just for kicks I tried a release build.  I was very surprised to find that it ran fine!  Not only no violated asserts, but the calibration was happening.  How do I know?  My SAADC event callback was being called with NRF_DRV_SAADC_EVT_CALIBRATEDONE, which shows that calibration was triggered and ran.

Ok, so more digging.  I wasn't seeing an assert failure in release mode, only because asserts are only active in debug builds.  By adding some logging, I was able to demonstrate that

NRFX_WAIT_FOR((m_cb.adc_state == NRF_SAADC_STATE_IDLE), HW_TIMEOUT, 0, result);

was still timing out.  It's just that, although the SAADC abort appeared to have failed, it actually worked.  Or wan't necessary (because maybe by the time the calibration was started by nrf_drv_saadc_calibrate_offset(), the SAADC was idle anyway.

So what's going on?  Maybe the timeout is too short?  I tried bumping up the '0' in the NRFX_WAIT_FOR() macro to 100, to give it heaps of time to complete.  And I set a breakpoint on the SAADC IRQ.  Turns out that the interrupt is never being triggered!  So m_cb.adc_state doesn't get updated, and the wait always times out.

And yet, it looks like the appropriate interrupt is being enabled.  Weird.

So, I'd built this with SDK 16.0.0, but was testing code written for 15.2.0, so the obvious thing was to try building with 15.2.0.  And it worked, even in debug mode!

Ah, but not so simple...  Why did it work, what's different?

It turns out that the corresponding code in nrfx_saadc_abort() in 15.2.0 looks like:

NRFX_WAIT_FOR((m_cb.adc_state != NRF_SAADC_STATE_IDLE), HW_TIMEOUT, 0, result);

The condition is the opposite!  That code is waiting for the ADC to be not idle, which of course it initially is (because we only hit this code if the ADC is busy), so it's always successful.  That line of code does nothing and seems to have been a bug that was fixed - in SDK 15.3.0 as it turns out.  When built with SDK 15.3.0, the code fails in exactly the same way as with 16.0.0, when running a debug build.

We're left with the SAADC interrupt not firing.  I'll investigate further, but it sure looks like a bug in the SDK.

Any ideas?

Parents
  • This seems like a bug, since nrfx_saadc_irq_handler()-->if (nrf_saadc_event_check(NRF_SAADC_EVENT_STOPPED))-->m_cb.adc_state = NRF_SAADC_STATE_IDLE is never triggered

    I have reported the issue and I'm currently waiting for an answer. However, if nrfx_saadc_abort() is modified accordingly it works fine:

    void nrfx_saadc_abort(void)
    {
        .
        .
        .
            {
                // Wait for ADC being stopped.
                bool result;
                //NRFX_WAIT_FOR((m_cb.adc_state == NRF_SAADC_STATE_IDLE), HW_TIMEOUT, 0, result);
                //NRFX_ASSERT(result);
                NRFX_WAIT_FOR(nrf_saadc_event_check(NRF_SAADC_EVENT_STOPPED), HW_TIMEOUT, 0, result);
                NRFX_ASSERT(result);
            }
        .
        .
    }

    Best regards,

    Simon

  • Thanks Simon.

    I hesitate to modify the SDK code (messy when it comes to version control and tracking dependencies), but I could certainly make my own version of nrfx_saadc_abort() based on this and use that.

    Thanks again.

Reply Children
Related