Understanding correct ISR implementation

The hardware manual for the device will details the peripheral registers, and any bits that need to be cleared during interrupt handling.
Are you writing your own interrupt handler, or using a callback that called from an existing interrupt handler in the SSP?

In reply to Jeremy:

Hi Jeremy,

yes I'm talking about ISRs, not callbacks from Synergy.

I'm using a timer and the DMAC transfer driver callback, which I think is an IRQ as well, by looking at r_dmac.c, R_DMAC_Open():

    if (NULL != p_cfg->p_callback)
    {
        ssp_vector_info_t * p_vector_info;
        fmi_event_info_t event_info = {(IRQn_Type) 0U};
        err = g_fmi_on_fmi.eventInfoGet(&feature, SSP_SIGNAL_DMAC_INT, &event_info);
        p_ctrl->irq = event_info.irq;
        DMAC_ERROR_RETURN(SSP_INVALID_VECTOR != p_ctrl->irq, SSP_ERR_IRQ_BSP_DISABLED);
        R_SSP_VectorInfoGet(p_ctrl->irq, &p_vector_info);
        NVIC_SetPriority(p_ctrl->irq, p_cfg->irq_ipl);
        *(p_vector_info->pp_ctrl) = p_ctrl;
    }

 

For the timer I don't set any register flags in the Interrupt handler. For the DMAC, my code currently looks like this:

 

void dmac_adc_measurement_callback(transfer_callback_args_t * p_args)
{
    SF_CONTEXT_SAVE;
    SSP_PARAMETER_NOT_USED(p_args);

    g_gpt0.p_api->stop(g_gpt0.p_ctrl);

    /* Count blocks and store adc_buffer.buffer index of the next block */
    adc_device.interrupt_count = (adc_device.interrupt_count + 1) % DMAC_NUM_BLOCKS;
    adc_device.current_block_index = adc_device.interrupt_count * DMAC_NUM_TRANSFERS;

    /* Check if measurement is completed (= buffer is filled once since trigger_index)*/
    if(adc_device.state == ADCDevice::State::MEASURING
        && adc_device.current_block_index == adc_device.trigger_index)
        adc_device.set_state(ADCDevice::State::MEASUREMENT_END);
    else
    {
        /* Set the number of transfers for the next block */
        R_DMAC0->DMCRB = DMAC_NUM_TRANSFERS;

        /* If we start again at the beginning */
        if(adc_device.interrupt_count == 0)
        {
            /* Reset the DMAC for the next capture */
            /* Reset the source, destination address */
            g_transfer0.p_cfg->p_info->p_dest = static_cast<void*>(adc_device.buffer.data());

            /* Reset the DMAC.
             * Source address will be the same.
             * New Destination address.
             * Refresh the block count
             */
            g_transfer0.p_api->reset(g_transfer0.p_ctrl,
                                               g_transfer0.p_cfg->p_info->p_src,
                                               g_transfer0.p_cfg->p_info->p_dest,
                                               DMAC_NUM_TRANSFERS );
        }
        g_transfer0.p_api->enable(g_transfer0.p_ctrl);

        g_gpt0.p_api->start(g_gpt0.p_ctrl);
    }

    /* Calibrate the sensor channels while idle */
    //double FM_idle_time = static_cast<double>(tx_time_get() - FM::instance().idle_since) / static_cast<double>(TX_TIMER_TICKS_PER_SECOND);
    if(adc_device.state == ADCDevice::State::IDLE && (tx_time_get() - FM::instance().idle_since) > DMAC_NUM_TRANSFERS * ADC_SPEED * TX_TIMER_TICKS_PER_SECOND)
        calibration_step();

    SF_CONTEXT_RESTORE;
}

The code is used to implement a multi-DMAC block ring buffer with a pre/post-triggering system for short time measurements, somewhat similar to an oscilloscope. The code is (mostly) working as expected. so I believe that I don't need to clear any additional registers.

However, I sometimes see an erroneous data point at the block boundary, which I assume comes from timing problems in this fucntion. I've written some different variants which will be tested shortly:

1. Not stopping the timer
2. Resetting the timer after stopping
3. Increasing interrupt priority
4. Clearing pending IRQ (Shouldn't do anything here as this IRQ triggers once every 5 ms)

I hope that one of these will help with this problem, otherwise I'll have to do some signal processing to filter out invalid data points.

In reply to ChrisS:

Can you detail, perhaps including a timing diagram, of what it is you're trying to achieve.
The interaction between GPT, ADC and DMAC is very configurable (not directly via the SSP, a bit of register manipulation is required) so I am confident that a solution can be found to eliminate the glitches that you are seeing.
I have a few ADC/GPT/DMA examples that have the DMA reading the ADC continuously. For example, DMA extended repeat mode can be used and when implemented correctly, there is no need for any ISRs to reconfigure and restart the DMA. It this what you are trying to achieve?
As I said, if you can provide a bit more info then I'm confident a solution can be provided.

In reply to Richard:

Hi Richard,

I think we actually talked about this before and you gave me an example.

We're measuring a signal with the ADC with a very high sampling rate of 500 kHz, so very close to the limit of the S5D5. The measuring time is 1 s and the data is saved continously in a ring buffer into an external SDRAM. The DMAC is triggered by the ADC Scan End event to copy the measured data points (4 channels every 2 µs). However, as the DMAC can only transfer up to 65536 times it needs to be reconfigured repeatedly. This is done in the previous code by resetting the number of transfers (R_DMAC0->DMCRB = DMAC_NUM_TRANSFERS;) and by setting the destination address to the start of the new block (g_transfer0.p_cfg->p_info->p_dest = static_cast<void*>(adc_device.buffer.data());). Afterwards the DMAC is reset and enabled and the timer which triggers the ADC measurements is started again.

Below is the code which I used to initialize the peripherals and to reset it after an event has been measured:

void ADCDevice::initialize()
{
    ssp_err_t ssp_err;
    /* Open the ADC Unit 0 - Channels AN003, AN004, AN005, AN006 are enabled */
    ssp_err = g_adc0.p_api->open(g_adc0.p_ctrl, g_adc0.p_cfg);
    ASSERT_STATUS(ssp_err);

    /* Configure the ADC Unit 0 - Channels AN003, AN004, AN005, AN006 are enabled */
    ssp_err = g_adc0.p_api->scanCfg(g_adc0.p_ctrl, g_adc0.p_channel_cfg);
    ASSERT_STATUS(ssp_err);

    /* Open the GPT that will trigger the ADC every 2us */
    ssp_err = g_gpt0.p_api->open(g_gpt0.p_ctrl, g_gpt0.p_cfg);
    ASSERT_STATUS(ssp_err);

    /* Link the GPT5 Overflow event to trigger the ADC UNIT 0 */
    ssp_err = g_elc.p_api->linkSet(this->elc_peripheral, this->elc_event);
    ASSERT_STATUS(ssp_err);

    /* Set the DMAC source and destination addresses
     * All other parameters have been set in the configuration
     */
    g_transfer0.p_cfg->p_info->p_src = (void*)&R_S12ADC0->ADDRn[this->sensor_channels[0]];
    g_transfer0.p_cfg->p_info->p_dest = (void*)&buffer[0][0];

    /* Open the DMAC */
    ssp_err = g_transfer0.p_api->open(g_transfer0.p_ctrl, g_transfer0.p_cfg);
    ASSERT_STATUS(ssp_err);

    /* Direct register access of DMAC0 to enable the correct interrupt */
    /* By default, if DMAC interrupts are enabled then DTIE, ESIE and RPTIE are enabled
     * We only want DTIE, so rewrite DMIT from 0x1C to 0x10
     * Refer to section 17.2.6 of the Hardware User's Manual for more information
     */
    R_DMAC0->DMINT = 0x10;

    /* Set the ADC ready for triggering */
    ssp_err = g_adc0.p_api->scanStart(g_adc0.p_ctrl);
    ASSERT_STATUS(ssp_err);

    //Open the IRQs to enable triggering
    uint32_t i = 0;
    for(auto irq : this->surge_irqs)
    {
        const_cast<external_irq_cfg_t *>(irq->p_cfg)->p_context = &surge_trigger_irq_counts[i];
        ssp_err = irq->p_api->open(irq->p_ctrl, irq->p_cfg);
        ASSERT_STATUS(ssp_err);
        i++;
    }

    return;
}

void ADCDevice::reset()
{
    uint32_t status = SSP_SUCCESS;
    /* Reset the DMAC for the next capture */
    /* Reset the source, destination address */
    g_transfer0.p_cfg->p_info->p_dest = (void*)&buffer[0][0];

    /*Also reset the current index in the buffer */
    current_block_index       = 0;
    interrupt_count = 0;

    /* Reset the DMAC.
     * Source address will be the same.
     * New Destination address.
     * Refresh the block count
     */
    status = g_transfer0.p_api->reset(g_transfer0.p_ctrl,
                                      g_transfer0.p_cfg->p_info->p_src,
                                      g_transfer0.p_cfg->p_info->p_dest,
                                      DMAC_NUM_TRANSFERS );
    ASSERT_STATUS(status);

    /* Enable the DMAC, ready for trigger */
    status = g_transfer0.p_api->enable(g_transfer0.p_ctrl);
    ASSERT_STATUS(status);

    /* Start continuous measurement by starting GPT0 that will start the "FAST" ADC, DMAC process */
    status = g_gpt0.p_api->start(g_gpt0.p_ctrl);
    ASSERT_STATUS(status);

    //Enable interrupts to enable the measurement triggers
    for(auto irq : surge_irqs)
    {
        status = irq->p_api->enable(irq->p_ctrl);
        ASSERT_STATUS(status);
    }
}

The constants are defined as follows:

const uint16_t      DMAC_TRANSFER_SIZE          = 4;
const uint16_t      DMAC_NUM_TRANSFERS          = 2500;
const uint16_t      DMAC_BLOCK_SIZE             = DMAC_TRANSFER_SIZE * DMAC_NUM_TRANSFERS;

This means that the DMAC reconfiguration will happen every 2 µs * 2500 = 5 ms. At this block boundary I'm getting nonsensical values sometimes. They can be larger than the 12 bit range of the ADC.

In reply to ChrisS:

Chris,
That is very possible!

I had one immediate thought.
Could you use 2 DMA channels.
DMAC0 performs its number of transfers, filling the buffer, at which point you will get the DMAC end ISR.
This ISR could enable DMAC1 which could then be handling the ADC and filling the buffer, giving the application time to reset DMAC0, ready for when DMAC1 completes and hands over to DMAC0.
Have you considered this already? Is this possible in your system?
Regards,
Richard

In reply to Richard:

Hi Richard,

I think that's a great idea, I will try to implement and test this tomorrow.
Is it possible to setup an ELC link to have it start automatically? My knowledge about the ELC system is very limited unfortunately. Otherwise the start of the second DMA channel needs to be fast enough to continue the measurement here. If necessary, I could also tolerate a somewhat longer time between the data points at this boundary, since the point should be deterministic and could be considered in later signal processing.

Best Regards,
Chris

 

Edit: I found a diagram in the µC manual that seems to indicate that only the DTC can be started by the ELC, so this does not seem to be an option.

In reply to ChrisS:

Chris,

I did look at that and there doesn't seem to be an easy way.

However, my thought is this. Prior to starting the ADC that the DMA's will be all configured, ready to take the ADC interrupt event and transfer data accordingly.
Once configured, to enable a DMA is one line of code, setting the DTE bit in the corresponding DMCNT register.
i.e. R_DMAC0->DMCNT = 0x01; or R_DMAC1->DMCNT = 0x01;
If this is the first line of code in the DMAC ISR then I think you will have enough time.
You say that the ADC (& DMAC) is being triggered every 2us. With the S5D5 ICLK running at 120MHz, that gives you 240 CPU clocks to take the DMAC ISR, and execute the code R_DMAC0->DMCNT = 0x01;
If may be worth while considering using a custom DMAC ISR as the SSP ISR handlers can be rather "code heavy" and could slow the response time.
If you think that you system may not be able to repsond in time (the CPU is doing other system critical tasks that can't be interrupted) then the DMAC ISR can trigger the DTC. The DTC could be set up to write the value 0x01 to the corresponding DMCNT register.
Let me know your thoughts and if required I can put together a proof on concept?

Regards,
Richard

In reply to Richard:

If may be worth while considering using a custom DMAC ISR as the SSP ISR handlers can be rather "code heavy" and could slow the response time.

Do you mean replacing g_transfer0.p_api->enable() with a custom implementation?

We cannot (or probably shouldn't) start it directly at the ISR start since we may have to stop the measurement at the post triggering time. It may be possible to use a slightly larger buffer to keep it running before doing the check at the cost of some additional complexity.

I think there isn't too much time critical other processes going on. We have some ethernet communication and a much slower ADC measurement on ADC1. There's also some controlling done in the ISR I showed above, but that can happen after reconfiguration. There are however some IRQ based triggers for the measurement with higher or equal priority as the DMAC ISR.

In reply to ChrisS:

You can do that. Everything that eliminates code execution time.
But I was actually thinking of the DMA ISR. From the code you have posted it looks as if you are using the DMA callback function. " void dmac_adc_measurement_callback(transfer_callback_args_t * p_args)"
This callback is called as part of the DMA ISR.
Whilst its functionality is absolutely fine, it may be possible to save a few CPU clocks by writing your own.

In reply to Richard:

Ah yes you're right, I didn't look too far into the DMA transfer driver code. That function could be simplified a bit I think. It's probably easiest to exchange it during linking with -Wl,--wrap=dmac_int_isr or to define SSP_SUPPRESS_ISR_g_transfer0 and set SSP_VECTOR_DEFINE_CHAN(dmac_int_isr, DMAC, INT, 0); somewhere else.

In reply to ChrisS:

I've started some optimizations for the ISR:

void dmac_int_isr (void)
{
    /* Stop the timer that triggers the ADC */
    g_gpt0.p_api->stop(g_gpt0.p_ctrl);

    /* Count blocks and store adc_buffer.buffer index of the next block */
    adc_device.interrupt_count = (adc_device.interrupt_count + 1) % DMAC_NUM_BLOCKS;

    /* Check if measurement is completed (= buffer is filled once since trigger_index)*/
    if(adc_device.state != ADCDevice::State::MEASURING
        || adc_device.interrupt_count != adc_device.trigger_interrupt_count)
    {
        /* Set the number of transfers for the next block */
        R_DMAC0->DMCRB = DMAC_NUM_TRANSFERS;

        /* If we start again at the beginning */
        if(adc_device.interrupt_count == 0)
        {
            /* Reset the DMAC for the next capture */
            /* Reset the source, destination address */
            g_transfer0.p_cfg->p_info->p_dest = static_cast<void*>(adc_device.buffer.data());

            /* Reset the DMAC.
             * Source address will be the same.
             * New Destination address.
             * Refresh the block count
             */
            g_transfer0.p_api->reset(g_transfer0.p_ctrl,
                                               g_transfer0.p_cfg->p_info->p_src,
                                               g_transfer0.p_cfg->p_info->p_dest,
                                               DMAC_NUM_TRANSFERS );
        }

        /* Transfer is disabled during the interrupt if an interrupt is requested after each block. If not all transfers
         * are complete, reenable transfer here. */
        auto p_ctrl = static_cast<dmac_instance_ctrl_t*>(g_transfer0.p_ctrl);
        static_cast<R_DMAC0_Type*>(p_ctrl->p_reg)->DMCNT_b.DTE = true;
        extern R_ICU_Type * gp_icu_regs;
        gp_icu_regs->DELSRn[p_ctrl->channel].DELSRn_b.DELS = p_ctrl->trigger;

        /* Start the ADC timer again to resume measurement */
        g_gpt0.p_api->start(g_gpt0.p_ctrl);
    }
    else
        adc_device.set_state(ADCDevice::State::MEASUREMENT_END);
    adc_device.current_block_index = adc_device.interrupt_count * DMAC_NUM_TRANSFERS;

    /* Calibrate the sensor channels while idle */
    //double FM_idle_time = static_cast<double>(tx_time_get() - FM::instance().idle_since) / static_cast<double>(TX_TIMER_TICKS_PER_SECOND);
    if(adc_device.state == ADCDevice::State::IDLE && (tx_time_get() - FM::instance().idle_since) > DMAC_NUM_TRANSFERS * ADC_SPEED * TX_TIMER_TICKS_PER_SECOND)
        calibration_step();

    /** Clear pending IRQ to make sure it doesn't fire again after exiting */
    R_BSP_IrqStatusClear(R_SSP_CurrentIrqGet());
} /* End of function dmac_int_isr */

It might be possible to optimize away the stopping of the ADC timer (if it's fast enough) and the check for ADCDevice::State::MEASURING. I'm not sure if the gp_icu_regs->DELSRn[p_ctrl->channel].DELSRn_b.DELS = p_ctrl->trigger; line is needed.

In the next step I'll try to use a second DMA channel. If that is not enough I might add one more block to the rin buffer to directly resume the measurement without checking for the stop condition. If that is still not sufficient the DTC approach might be needed. Do you think this is a valid approach?

In reply to ChrisS:

Chris,
I have just uploaded a project to the Media Gallery (S5D5_TB_ADC_DUAL_DMAC.zip) which I hope will give you some pointers.

The example that uses 2 DMA channels to read the ADC and write data into an incrementing buffer
DMA0 writes to the buffer 10000 results and then DMA1 takes over
When DMA1 has written 10000 results, DMA0 takes over.

When the buffer is full, the buffer size is 100000 elements in size, the buffer wraps around

The DMAs are configured for block mode, reading 4 ADC channels, and the block transfer number is 2500
The DMA ISRs enable the next DMA channel.
I hope this provides glitch free ADC readings.

Regards,
Richard

In reply to Richard:

Hi Richard,

thanks for the example! I actually went ahead and wrote an implementation myself in parallel. I will check yours out and compare it against mine. I'll report back with test results tomorrow.

One question: Is it necessary to move the resetting of the DMAC channel that recently finished into the main loop / a thread or can it remain in the ISR after starting the new one?

I more or less have the same implementation, except that I haven't migrated to my own ISR yet and still rely on the callback, in which I also setup the DMAC channel that will be needed next time. I've also used pointers to the transfer structures and registers which I swap on each invocation of the callback.

Best Regards,
Chris

In reply to ChrisS:

Chris,
One final question. What size is the circular buffer that you are using?
If you can use a circular buffer of 2^n (n=1->27) then a solution using DMA Extended repeat mode could be used.
(The sizes are detailed in Table 17.2 of the Hardware Users Manual.)

This extended repeat area solution uses minimal interrupts, just an occasional interrupt to rewrite the DMCRB so that it never gets to 0 and stops the DMA.
The reconfiguration of the DMA destination address is performed automatically. The only limitation is that you buffer would have to be of a specific size.

I can provide further info tomorrow if this is of interest?
Regards,
Richard

In reply to Richard:

I think this might be an option. Right now I'm measuring 4 channels, 12 bit, 500 kHz for 1 s, so 4*2*500000=4000000 bytes. A buffer of 2^22=4194304 might thus be a good fit. I'll check out this implementation first though, maybe it's already sufficient.