SDK's FLEXCOMM0_DriverIRQHandler: strange code

[danielholala]

Hi,

I'm using SDK 2.10.1 for LPC5526. While struggling with code for USART (managed by Peripherals Tools), I checked the code flow when characters are received in an IRQ based configuration.

On receive, the IRQ handler FLEXCOMM0_DriverIRQHandler is called

fsl_flexcomm.c:

void FLEXCOMM0_DriverIRQHandler(void)
{
    uint32_t instance;

    /* Look up instance number */
    instance = FLEXCOMM_GetInstance(FLEXCOMM0);
    assert(s_flexcommIrqHandler[instance] != NULL);
    s_flexcommIrqHandler[instance]((uint32_t *)s_flexcommBaseAddrs[instance], s_flexcommHandle[instance]);
    SDK_ISR_EXIT_BARRIER;
}

FLEXCOMM_GetInstance() iterates over a static array 

fsl_flexcomm.c:

static const uint32_t s_flexcommBaseAddrs[] = FLEXCOMM_BASE_ADDRS;

that contains all base addresses in natural order

LPC5526.h:

#define FLEXCOMM_BASE_ADDRS    { 
FLEXCOMM0_BASE, 
FLEXCOMM1_BASE,
FLEXCOMM2_BASE,
FLEXCOMM3_BASE,
FLEXCOMM4_BASE,
FLEXCOMM5_BASE,
FLEXCOMM6_BASE,
FLEXCOMM7_BASE,
FLEXCOMM8_BASE }

As you can see, the handler contains the base address of flexcom instance 0 hard coded and iterates over all possible base addresses to figure out the instance of this handler. 

I think this violates the guideline to keep handler code short and concise.

Most of all, as the instance number should be well known at compile time. For example, there could be a define #define FLEXCOMM0_INSTANCE 0 to avoid the iteration. 

I wonder why the code is so odd?

[xiangjun_rong]

Hi, Daniel,

As you know that the void FLEXCOMM0_DriverIRQHandler(void) is an ISR(Interrupt Service Routine), when the FlexComm0 interrupt is triggered, the ISR is called automatically.

why the following two lines are inserted is that it must be make sure that the s_flexcommIrqHandler[instance] function is initialized, in other words, the body of s_flexcommIrqHandler[instance] function must exist, otherwise, when the function does not exist, but you call it, it is a fatal error, the system may be collapsed.

instance = FLEXCOMM_GetInstance(FLEXCOMM0);
assert(s_flexcommIrqHandler[instance] != NULL); 

Hope it can help you

void FLEXCOMM0_DriverIRQHandler(void)
{
    uint32_t instance;

    /* Look up instance number */
    instance = FLEXCOMM_GetInstance(FLEXCOMM0);
    assert(s_flexcommIrqHandler[instance] != NULL);
    s_flexcommIrqHandler[instance]((uint32_t *)s_flexcommBaseAddrs[instance], s_flexcommHandle[instance]);
    SDK_ISR_EXIT_BARRIER;
}

[danielholala]

Hi @xiangjun_rong ,

I understand. Thanks for the explanation.

All I'm saying is that looking at this ISR and based on the fact that part of the code is generated automatically, there's no need to calculate the instance number during runtime. It should be computed or defined at compile time, e.g., in 

LPC5526.h:
#define FLEXCOMM0_INSTANCE 0
#define FLEXCOMM1_INSTANCE 1
// (etc...)

and then  FLEXCOMM0_DriverIRQHandler() could avoid the call to lookup instance number and instead just contain:

instance = FLEXCOMM0_INSTANCE;

or even replace the variable completely. Maybe you don't even need to define this at all when for Flexcom 0 the handler is always at index 0, for Flexcom 1 the handler is always at index 1, etc.

assert(s_flexcommIrqHandler[0] != NULL);
s_flexcommIrqHandler[0]((uint32_t *)s_flexcommBaseAddrs[0], s_flexcommHandle[0]);

On a side note, I already suggested to move away from base addresses to instance numbers of peripherals when calling API functions.

Best regards,
Daniel

[xiangjun_rong]

Hi, Daniel,

I agree with you that getting instance variable in real time is unnecessary and superfluous.

BR

XiangJun Rong