MQX 4.0 BSP Bug: twr-mcf54418 (_mcf5441_int_init - _bsp_int_init)

[stevejanisch]

I have discovered a bug in the _mcf5441_int_init routine (aka _bsp_int_init via macro definition).  In fact I note the _bsp_int_init macro just in case it carries over to other BSPs.

The issue is with the incoming argument PSP_INTERRUPT_TABLE_INDEX irq.

When I called the routine from my application, the passed in irq was being interpreted as a signed 8-bit number, so any irq > 127 (the mcf5441x has interrupt vectors up to 255) was being interpreted as a negative number.  The function would fail with the return code MQX_INVALID_PARAMETER.

My quick fix was to change the name of the argument from irq to irq_in and then add the line

uchar irq = irq_in ;

Header 1

/*FUNCTION*----------------------------------------------------------------- * * Function Name   : _mcf5441_int_init * Returned Value  : uint_32 * Comments        : *   Initialize a specific interrupt in the proper interrupt controller *   SEJ 26-APR-2013 : Revised to quick fix incoming irq as unsigned char. * *END*---------------------------------------------------------------------*/ uint_32 _mcf5441_int_init    (       // [IN} Interrupt number     //PSP_INTERRUPT_TABLE_INDEX irq,       PSP_INTERRUPT_TABLE_INDEX irq_in, // SEJ - this is being case as signed 8-bit       // [IN} Interrupt priority level       _int_level                level,       // [IN} Unmask the interrupt now?       boolean                   unmask    ) {     _mqx_int idx;     uint_32 temp;     /* This function had some issues... at least when called from an external application.      * The passed PSP_INTERRUPT_TABLE_INDEX irq was interpreted as a signed 8-bit number,      * so any irq > 127 came in as a negative number.  Perhaps this issue is deeper than      * this, but to quick fix I changed the passed in argument 'irq' to 'irq_in' and then      * declare the local uchar irq here.     **/     uchar irq = irq_in ;     if (irq >= PSP_INT_FIRST_EXTERNAL) {         idx = irq - PSP_INT_FIRST_EXTERNAL;         temp = _psp_get_sr();         _psp_set_sr(temp | 0x0700);         if (idx < 64) {             PSP_GET_ICTRL0_BASE()->ICR[idx] = level & 7;             if (unmask)                 PSP_GET_ICTRL0_BASE()->CIMR = MCF54XX_ICTRL_IMR_N(idx);             else                 PSP_GET_ICTRL0_BASE()->SIMR = MCF54XX_ICTRL_IMR_N(idx);         }         else if (idx < 128) {             idx -= 64;             PSP_GET_ICTRL1_BASE()->ICR[idx] = level & 7;             if (unmask)                 PSP_GET_ICTRL1_BASE()->CIMR = MCF54XX_ICTRL_IMR_N(idx);             else                 PSP_GET_ICTRL1_BASE()->SIMR = MCF54XX_ICTRL_IMR_N(idx);         }         else {             idx -= 128;             PSP_GET_ICTRL2_BASE()->ICR[idx] = level & 7;             if (unmask)                 PSP_GET_ICTRL2_BASE()->CIMR = MCF54XX_ICTRL_IMR_N(idx);             else                 PSP_GET_ICTRL2_BASE()->SIMR = MCF54XX_ICTRL_IMR_N(idx);         }         _psp_set_sr(temp);         return MQX_OK;     }     return MQX_INVALID_PARAMETER; }

[c0170]

Hello Steve Janisch,

I glanced into the code, there's enum MCF5441_INTERRUPT_TABLE_INDEX which should be used (it will be type int anyway). What I can't answer straightaway it's why in the end of the enum it's psp name commented out:

MCF5441_INTERRUPT_TABLE_INDEX;//, PSP_INTERRUPT_TABLE_INDEX

I can't find typedef or macro redefinition between this enum and PSP_INTERRUPT_TABLE_INDEX. Where did you find that PSP_INTERRUPT_TABLE_INDEX is signed char?

Thank you for sharing with us !! I'll fill in request to review this.

Regards,

c0170

[stevejanisch]

I didn't see the enum type defined as a signed char anywhere. 

You might recall I've been working on a application for a motor control and have been spending some time with the mcPWM module.  I wanted to assign the reload interrupt so I could count the number of pulses and transfer the counts to a lower priority thread but the function _mcf5441_int_init (or _bsp_int_init) failed and returned the MQX_INVALID_PARAMETER.

When I debugged the function I saw that the passed in irq from the enum (MCF5441_INT_PWM_SM0SR_RF - which is 197d, or 0xC5) was being passed as 0xC5, but -59d rather than 197d.  Ergo, my interpretation that the value was being interpreted as a signed char rather than an unsigned char.

Note that I am calling the function from my application, not within any BSP function.  Perhaps if I initialized the interrupt within the BSP code itself some other macro or cast would take place and the value would be interpreted correctly.  I only know that it failed when called from the external application and appears to have been cast as a signed char.

[c0170]

Hello Steve Janisch,

I remember your project, I hope you are going to share the result with the community :smileywink: at least some snippets which might help others. Macro PSP_INTERRUPT_TABLE_INDEX is redefined to MCF5441_INTERRUPT_TABLE_INDEX which is enumerated type.

I finally found why it might be misinterpreted. I have to test this with MCF compiler in CW. It can misinterpreted enum type. Because the type is implementation defined, here's copy from C ISO standard:

Each enumerated type shall be compatible with char, a signed integer type, or an

unsigned integer type. The choice of type is implementation-defined,110) but shall be

capable of representing the values of all the members of the enumeration. The

enumerated type is incomplete until after the } that terminates the list of enumerator

declarations.

It shall be capable of representing all values, in MCF case it's not if it is char type. We have to verify this.

Regards,

c0170

[stevejanisch]

I did post some non-MQX into my original thread.

https://community.freescale.com/thread/306288

I started without MQX since there was no official MQX support for mcPWM and it was recommended to start with Processor Expert code generation and AFAIK MQX and Processor Expert do not work together for my MC. I am still working on the MQX code implementation, although it is very similar to the above.

[c0170]

Hello,

I have finally found some time to test this on MCF54418. I can't reproduce it. I added this line inside the hello_task

_bsp_int_init(MCF5441_INT_PWM_SM0SR_RF, 1, 1, 1);

I stepped inside the function and the first parameter irq was 197.

Regards,

c0170

[stevejanisch]

After some googling, there appear to be two compiler directives (Tool Settings->ColdFire  Compiler->Language Settings) that could be in play... "ANSI Strict" and "Enum Always Int"

When I downloaded and compiled the BSP the "Enum Always Int" option was checked.  The sample code I was using to build my project did not have it set.  Hmmm...

[c0170]

Hello Steve Janish,

what's the outcome then? Does it work if "Enum always Int" but it does not work with "ANSI Strict" set ??

I don't see the reason why would this break enums, both should follow the C Standard which I quoted before. Even though I only opened C99 Draft, but I believe it should be same as well in C89.

I run the test application on hello world example with default settings , same applied for board libraries.

Regards,

c0170

[stevejanisch]

Could it be a compiler directive... something that makes it unsigned in your build but signed in mine?