ASF: SAMD ADC Callback Driver Risky Bug with Jobs

I encountered a rather dangerous bug when using a SAMD21 with the ASF ADC Callback driver as of ASF 3.20.1

In the callback driver, to start interrupts you would do

adc_register_callback(&adc_instance, adc_complete_callback, ADC_CALLBACK_READ_BUFFER);
adc_enable_callback(&adc_instance, ADC_CALLBACK_READ_BUFFER);

uint16_t adc_buffer[8] = {0};
adc_read_buffer_job(&adc_instance,adc_buffer,8);

after configuration of the ADC of course.

Then the ADC callback will be

/**
 * \brief ADC complete callback, stores ADC measurement
 *
 *	\param module	ADC Module
 */
void adc_complete_callback(const struct adc_module *const module)
{
	measurement = adc_buffer[0];
}

This will work just fine.

The bug comes into play with adc_abort_job

	adc_abort_job(&adc_instance,ADC_JOB_READ_BUFFER);

This will disable the callback and ADC interrupts like we want. Except if you look into it

void adc_abort_job(
		struct adc_module *module_inst,
		enum adc_job_type type)
{
	/* Sanity check arguments */
	Assert(module_inst);

	if (type == ADC_JOB_READ_BUFFER) {
		/* Disable interrupt */
		adc_disable_interrupt(module_inst, ADC_INTERRUPT_RESULT_READY);
		/* Mark job as aborted */
		module_inst->job_status = STATUS_ABORTED;
		module_inst->remaining_conversions = 0;
	}
}

It does not appear to cancel running conversions (which isn’t possible anyway).

Now why are running conversions important? Because they set the INTFLAG.RESRDY bit when finished. This bit does not require interrupts turned on to get set, it allows for polling based conversions instead of interrupts.

Now to continue.

static inline void adc_disable_interrupt(struct adc_module *const module_inst,
		enum adc_interrupt_flag interrupt)
{
	/* Sanity check arguments */
	Assert(module_inst);
	Assert(module_inst->hw);

	Adc *const adc_module = module_inst->hw;
	/* Enable interrupt */
	adc_module->INTENCLR.reg = interrupt;
}

Only the INTEN register gets modified in disable_interrupt which means interrupts will no longer generate which is okay.

Now the problem occurs when adc_read_buffer_job is called to start our ADC measurements again.

enum status_code adc_read_buffer_job(
....
	adc_enable_interrupt(module_inst, ADC_INTERRUPT_RESULT_READY);

	if(module_inst->software_trigger == true) {
		adc_start_conversion(module_inst);
	}

	return STATUS_OK;
}

adc_enable_interrupt only changes INTEN to turn on RESRDY:

adc_module->INTENSET.reg = interrupt;

adc_start_conversion only does:

adc_module->SWTRIG.reg |= ADC_SWTRIG_START;

And that’s how we have a bug.

The issue is, if

1. Conversion is running
2. adc_abort_job is called while a conversion is running, interrupts get disabled (that’s ok)
3. INTFLAG.RESRDY gets set because the conversion completed sometime later after #2
4. adc_read_buffer_job is called later in the code and enables interrupts
5. NVIC does not generate an interrupt as desired

Why does #5 happen? Because INTFLAG.RESRDY must be cleared before the NVIC can see the interrupt occur.

Fixing this right now is easy using ASF routines:

adc_clear_status(&adc_instance, ADC_STATUS_RESULT_READY);

which will gurantee RESRDY is cleared before starting a conversion.

before calling

adc_read_buffer_job(&adc_instance,adc_buffer,8);

I tried arguing with Atmel support with it but the current support rep doesn’t understand INTFLAG.RESRDY will be always be set regardless of INTEN which is just for the NVIC to generate interrupts.