adafruit / seesaw

I2C friend to expand capabilities of other chips.
Other
76 stars 34 forks source link

NVIC_Enable() not working for D51 #36

Closed wallarug closed 4 years ago

wallarug commented 4 years ago

Hi there,

Not really sure if anyone is able to assist with this query. It relates to the porting of SeeSaw to SAMD51. I've hit another roadblock where the compiler is throwing an error:

Building robohatmm1
source/bsp.cpp
In file included from lib/samd51/samd51a/include/samd51g19a.h:523:0,
                 from lib/samd51/samd51a/include/sam.h:36,
                 from ./bsp/bsp_nvmctrl.h:4,
                 from source/bsp.cpp:38:
lib/cmsis/CMSIS/Include/core_cm4.h: In static member function 'static void QP::QF::onStartup()':
lib/cmsis/CMSIS/Include/core_cm4.h:1497:44: error: array subscript is above array bounds [-Werror=array-bounds]
   NVIC->ISER[(uint32_t)((int32_t)IRQn) >> 5] = (uint32_t)(1 << ((uint32_t)((int32_t)IRQn) & (uint32_t)0x1F)); /* enable interrupt */
                                            ^
cc1plus: all warnings being treated as errors
Makefile:200: recipe for target 'build/robohatmm1/source/bsp.o' failed

Now I have narrowed down what it is saying and am going to copy the function here for reference.

seesaw\lib\cmsis\CMSIS\Include\core_m4.h

/** \brief  Enable External Interrupt

    The function enables a device-specific interrupt in the NVIC interrupt controller.

    \param [in]      IRQn  External interrupt number. Value cannot be negative.
 */
__STATIC_INLINE void NVIC_EnableIRQ(IRQn_Type IRQn)
{
/*  NVIC->ISER[((uint32_t)(IRQn) >> 5)] = (1 << ((uint32_t)(IRQn) & 0x1F));  enable interrupt */
  NVIC->ISER[(uint32_t)((int32_t)IRQn) >> 5] = (uint32_t)(1 << ((uint32_t)((int32_t)IRQn) & (uint32_t)0x1F)); /* enable interrupt */
}

Also for reference, the SAMD21 varient of the same function... seesaw\lib\cmsis\CMSIS\Include\core_m0.h

/** \brief  Enable External Interrupt

    The function enables a device-specific interrupt in the NVIC interrupt controller.

    \param [in]      IRQn  External interrupt number. Value cannot be negative.
 */
__STATIC_INLINE void NVIC_EnableIRQ(IRQn_Type IRQn)
{
  NVIC->ISER[0] = (1 << ((uint32_t)(IRQn) & 0x1F));
}

Example Usage in SeeSaw seesaw\source\bsp.cpp

#if CONFIG_SERCOM0
#ifndef SAMD51
    NVIC_EnableIRQ(SERCOM0_IRQn);
#else
    NVIC_EnableIRQ(SERCOM0_0_IRQn);
    NVIC_EnableIRQ(SERCOM0_1_IRQn);
    NVIC_EnableIRQ(SERCOM0_2_IRQn);
    NVIC_EnableIRQ(SERCOM0_3_IRQn);
#endif
#endif

Based on the error above it seems like the compiler does not like the >>5 shifting or something but I am at a loss as to how to fix it or where to report any bugs at this level.

Any help would be great.

Thanks!

tannewt commented 4 years ago

My guess is that the includes are defining the wrong number of interrupts. The shift isn't the problem, it's that the compiler doesn't think ISER is the correct length. Triple check that you have the correct defines for the microcontroller you are using. The length should be the total number of IRQs / 32 (which is >> 5). Each register is enable bits for 32 IRQs. Also check that the value of the passed in IRQ is correct.

wallarug commented 4 years ago

Hey @tannewt ,

I've had a chance to investigate this a bit further now.

I was in-fact compiling for a m0 instead šŸ¤¦ā€ā™‚ . After I fixed up the MakeFile and one or two other issues, I am still left with the same error.

Based on the information you provided above, It should be the following:

Total IRQs: 137 PERIPH_COUNT_IRQn = 137 /**< Number of peripheral IDs */ t_IRQ / 32 = 4.28

This does not make sense. It should be a whole number.

I am under the impression that whatever the corresponding SERCOM0_0_IRQn value is as per samd51g19a.h is being passed through. I have not found a way to verify this.

Latest Error (with changes)

Building robohatmm1
source/bsp.cpp
In file included from lib/samd51/samd51a/include/samd51g19a.h:523:0,
                 from lib/samd51/samd51a/include/samd51.h:41,
                 from ./include/bsp.h:37,
                 from source/bsp.cpp:37:
lib/cmsis/CMSIS/Include/core_cm4.h: In static member function 'static void QP::QF::onStartup()':
lib/cmsis/CMSIS/Include/core_cm4.h:1497:44: error: array subscript is above array bounds [-Werror=array-bounds]
   NVIC->ISER[(uint32_t)((int32_t)IRQn) >> 5] = (uint32_t)(1 << ((uint32_t)((int32_t)IRQn) & (uint32_t)0x1F)); /* enable interrupt */
                                            ^
cc1plus: all warnings being treated as errors
Makefile:205: recipe for target 'build/robohatmm1/source/bsp.o' failed
make: *** [build/robohatmm1/source/bsp.o] Error 1

The annoying bit is that this is just throwing the errors on the includes instead of the line causing the problem. So I may need to now go through and check that all the uses of NVIC_EnableIRQn() are correct.

wallarug commented 4 years ago

I found the issue. It does not like: NVIC_EnableIRQ(SysTick_IRQn);.

SysTick_IRQn has a value of -1. This compiles for SAMD09, SAMD21 boards but refuses for SAMD51 for some reason.

Any ideas @tannewt ?

tannewt commented 4 years ago

Yup! The SysTick interrupt isn't managed by the NVIC because it is part of the CPU. The NVIC only handles interrupt lines from peripherals. (There are 16 cortex IRQs that are -16 to -1.) The SysTick is managed by the System Control Block (SCB). I'm surprised it works for SAMD21! Check the EnableIRQ there. It may just ignore all values < 0.

wallarug commented 4 years ago

Hey @tannewt - I've got the EnableIRQ for SAMD21 posted in the original question.

It doesn't take any array indices. It is just set to 0. So that might answer that question šŸ˜† . So one might ask what is it setting up then?

NVIC->ISER[0] = (1 << ((uint32_t)(IRQn) & 0x1F));

Probably some random register value that doesn't impact anything (overflow would have it make IRQ very big I think).

I'll try disabling it for SAMD21 and see if it makes a difference to functionality.

tannewt commented 4 years ago

It is guarded by an if statement: https://github.com/adafruit/asf4/blob/039b5f3bbc3f4ba4421e581db290560d59fef625/samd21/CMSIS/Include/core_cm0plus.h#L720

It's not needed. It just obscures how it works. :-)

wallarug commented 4 years ago

I'm not too sure this is correct. The one included in SeeSaw is not guarded. Either way, it doesn't seem to use it anyway.

tannewt commented 4 years ago

Well that seems bad! We should update CMSIS I guess. The asf4 code picked it up here: https://github.com/adafruit/asf4/commit/1310593b8529a25239191fc1173c8632ead8aed9#diff-e2639d8ab4ac82c63b7adcdb0e2a1116R718-R724

wallarug commented 4 years ago

Last week when I was playing around with this issue, I tried updating CMSIS but it ended up being harder because they changed all the includes/defines to lower case where previously they were upper case.

I might play around with it again on a fresh seesaw branch and see if it can be done. It was last updated in 2017 on Seesaw.

wallarug commented 4 years ago

Resolved by PR #39