search

chipKIT32 / chipKIT-core

Downloadable chipKIT core for use with Arduino 1.6 - 1.8+ IDE, PlatformIO, and UECIDE
http://chipkit.net/
Apache License 2.0
59 stars 53 forks source link

Non PPS PIC32s: compile error with any sketch that uses DSPI #339

Closed EmbeddedMan closed 7 years ago

EmbeddedMan commented 7 years ago

Reported by Digilent while testing on the chipKIT Pro MX4.

When

if defined(PIC32MX1XX) || defined(PIC32MX2XX) || defined(PIC32MX3XX) || defined(PIC32MX4XX) || defined(PIC32MZXX) || defined(PIC32MX47X)

define _SPI2_IPL_ISR IPL3SOFT //interrupt priority for the ISR

define _SPI2_IPL_IPC 3 //interrupt priority for the IPC register

define _SPI2_SPL_IPC 0 //interrupt subpriority for the IPC register

endif

got changed to

if defined (__PIC32_PPS__)

in System_Defs.h (line 375) it broke DSPI for non-PPS PIC32s.

The compile error for the pModJstkDspiInt sketch on the chipKIT Pro MX4 board with chipKIT-core v1.4.0 is as follows:

Compiling library "DSPI"
"C:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\tools\pic32-tools\1.42-pic32gcc/bin/pic32-g++"  -c -g -O2 -Wall -Wextra  -DARDUINO_ARCH_PIC32 -mno-smart-io -ffunction-sections -fdata-sections  -mdebugger -Wcast-align -fno-short-double -ftoplevel-reorder -MMD -fno-exceptions -mprocessor=32MX460F512L -DF_CPU=80000000L  -DARDUINO=10801 -D_BOARD_CEREBOT_MX4CK_ -DMPIDEVER=16777998 -DMPIDE=150 -DIDE=Arduino "-G1024"  -IC:\Users\bschmalz\AppData\Local\Temp\arduino_build_82358/sketch "-IC:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\cores\pic32" "-IC:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\variants\Cerebot_MX4cK" "-IC:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\libraries\DSPI" "C:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\libraries\DSPI\DSPI.cpp" -o "C:\Users\bschmalz\AppData\Local\Temp\arduino_build_82358\libraries\DSPI\DSPI.cpp.o"
In file included from C:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\cores\pic32/pins_arduino.h:263:0,

                 from C:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\cores\pic32/WProgram.h:9,

                 from C:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\libraries\DSPI/DSPI.h:41,

                 from C:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\libraries\DSPI\DSPI.cpp:46:

C:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\libraries\DSPI\DSPI.cpp: In constructor 'DSPI0::DSPI0()':

C:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\variants\Cerebot_MX4cK/Board_Defs.h:334:22: error: '_SPI2_IPL_IPC' was not declared in this scope

 #define _DSPI0_IPL   _SPI2_IPL_IPC

                      ^

C:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\libraries\DSPI\DSPI.cpp:1072:10: note: in expansion of macro '_DSPI0_IPL'

  ipl = ((_DSPI0_IPL & 0x07) << 2) + (_DSPI0_SPL & 0x03);

          ^

C:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\variants\Cerebot_MX4cK/Board_Defs.h:335:22: error: '_SPI2_SPL_IPC' was not declared in this scope

 #define _DSPI0_SPL   _SPI2_SPL_IPC

                      ^

C:\Users\bschmalz\AppData\Local\Arduino15\packages\chipKIT\hardware\pic32\1.4.0\libraries\DSPI\DSPI.cpp:1072:38: note: in expansion of macro '_DSPI0_SPL'

  ipl = ((_DSPI0_IPL & 0x07) << 2) + (_DSPI0_SPL & 0x03);

                                      ^

This seems like a great thing to get fixed for v1.4.1. It the right thing to do simply to revert that #ifdef line?

@majenkotech

lstandage commented 7 years ago

Look at #336. This is kind of a duplicate issue.

I think the root cause is actually because on 5xx/6xx/7xx parts, the SPI interrupt is shared with the UART, but on all other parts, it's normal. I think we change the following:

#if defined(__PIC32_PPS__)
#define _SPI2_IPL_ISR   IPL3SOFT    //interrupt priority for the ISR
#define _SPI2_IPL_IPC   3       //interrupt priority for the IPC register
#define _SPI2_SPL_IPC   0       //interrupt subpriority for the IPC register
#endif

#if defined(__PIC32MX5XX__) || defined(__PIC32MX6XX__) || defined(__PIC32MX7XX__)
#define _SPI2_IPL_ISR   _UART3_IPL_ISR  //shared with UART3
#define _SPI2_IPL_IPC   _UART3_IPL_IPC
#define _SPI2_SPL_IPC   _UART3_SPL_IPC

to something like this

#if defined(__PIC32MX5XX__) || defined(__PIC32MX6XX__) || defined(__PIC32MX7XX__)
#define _SPI2_IPL_ISR   _UART3_IPL_ISR  //shared with UART3
#define _SPI2_IPL_IPC   _UART3_IPL_IPC
#define _SPI2_SPL_IPC   _UART3_SPL_IPC
#else
#define _SPI2_IPL_ISR   IPL3SOFT    //interrupt priority for the ISR
#define _SPI2_IPL_IPC   3       //interrupt priority for the IPC register
#define _SPI2_SPL_IPC   0       //interrupt subpriority for the IPC register
#endif

But we need to handle the MZ case differently, since we can have SRS. Right now, the way this code is defined, I think the SPI interrupt would be a SOFT interrupt on MZ.

majenkotech commented 7 years ago

Surely this is just the fault of the board definition specifying a macro that doesn't exist? Why are we even using these cryptic IPL macros anyway, when they're just a number? Just adjust the board definition to replace _SPI2_IPL etc with just the numbers for the interrupt levels we want on that board.

I have never understood why these macros even exist - what the point of them is. If you want to run something at IPL3 then specify it at IPL3, not some cryptic IPL macro that you have no clue what it equates to.

#define _DSPI0_IPL 3
#define _DSPI0_SPL 0

Now it's obvious that the SPI interrupt runs at priority 3.0 with this board.

Or even better don't have it in the board at all. If DSPI0 wants to be at IPL3 then code it to be at IPL3, not at some unknown IPL that is taken from a macro that points, eventually, to something defined depending on the SPI device it is allocated to. DSPI0's priority should be DSPI0's priority, not SPI1's priority on one board, SPI2's priority on another, etc.

That said, the interrupt system in DSPI has been written wrong (or is outdated) and needs ripping out and re-writing. For a start it has hard coded ISR functions using vectors for MZ parts. Instead it need to use the __USER_ISR macro. The _DSPIx_IPL_ISR macro is then deleted, since it is not needed - in fact it only has any use on the MZ parts, so why it's even being specified for MX parts is a mystery.

EmbeddedMan commented 7 years ago

Pull request #340 resolves this issue and has been merged.