 |
 |
| View previous topic :: View next topic |
| Author |
Message |
samikool
Joined: 01 Apr 2021
Posts: 15
|
| Problem calling function when passing structure *Workaround* |
 Posted: Thu Jun 03, 2021 11:06 am |
 |
|
Hello guys,
I am working with a PIC24FJ256GA106, and CCSC version 5.103, and running into a weird issue with my software.
I have a function that takes uint8_t and a slope typedef structure I defined, which has two doubles, m and b. However, for some reason when I call that function after running my code for a while it doesn't work. The program will just crash and the watchdog timer will reset the PIC.
However, I found that if I break the slope up into two doubles, m and b, and pass them individually instead of as a structure the code works perfectly. Additionally if I call the slope parameter version of the function on startup, it seems to work. Only after the program has been running for a while does it seem to crash. I was hoping you guys would have some insight into what is going on, as I am very confused. I have passed structures before without issue and this structure is very simple. All the relevant code will be listed below.
Here is where I call the two versions of the function from:
| Code: |
double m = getDouble(&payloadStartAddress);
double b = getDouble(&payloadStartAddress);
slope s = {0, 0};
s.m = m;
s.b = b;
printf("Connector: %d Slope: %0.5f Intercept: %0.5f\r\n", connector, s.m, s.b);
delay_ms(1000);
//realsetslope
setCalibration(connector, s); //doesn't work
// setCalibration(connector, s.m, s.b); //works
|
Here are the functions I'm calling:
| Code: |
// void setCalibration(uint8_t connector, double m, double b) //works
void setCalibration(uint8_t connector, slope s) //doesn't work
{
printf("Here...\r\n");
delay_ms(1000);
//if(!connectorValid(connector)) return; //this function is working but commented
printf("Valid...\r\n");
delay_ms(1000);
printf("Setting J%d -> m:%0.5f b:%0.5f\r\n", connector, s.m, s.b);
// printf("Setting J%d -> m:%0.5f b:%0.5f\r\n", connector, m, b);
delay_ms(1000);
}
|
Finally here is the code that works, no matter which function I use. This is called early on in the programs initializing stage:
| Code: |
//fakesetslope
slope s = {0.123123, 0.4635344};
uint8_t connector = 2;
setCalibration(connector, s); //works
//setCalibration(2, 0.123123, 0.4545645); //works
|
Please let me know if anyone needs clarification or more info.
Last edited by samikool on Thu Jun 03, 2021 12:49 pm; edited 1 time in total |
|
 |
samikool
Joined: 01 Apr 2021
Posts: 15
|
|
| Posted: Thu Jun 03, 2021 11:57 am |
|
|
Interesting update.
I found that changing the structure of a slope, can "fix" the issue.
The original slope structure is as follows
| Code: |
typedef struct {
double m;
double b;
} slope;
|
For testing I changed it to two integers as shown below.
| Code: |
typedef struct{
int m;
int b;
} slope;
|
This worked without issue, so thinking it could be something to do with structure size I changed it to two int64 instead as follows.
| Code: |
typedef struct{
int64_t m;
int64_t b;
} slope;
|
It stopped working. I then changed the original structure to four floats, so again it was different datatypes but a total of 16 bytes
| Code: |
typedef struct {
float m;
float b;
float a;
float c;
} slope;
|
This also did not work. The conclusion I'm drawing is that for some reason when the program gets to this point in my code, and I try to pass 16 bytes packed as a single structure the PIC locks up. If I am under 16 bytes in my single structure, or I pass the two doubles separately (2x8 byte parameters) things work. I have emailed CCS, but I'm leaning towards this being some sort of compiler bug, unless anyone can thing of something I'm doing wrong. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19513
|
|
| Posted: Thu Jun 03, 2021 12:07 pm |
|
|
I'd actually try reversing the variables in the function declaration.
Problem is having an int_8, then a structure. The structure needs to be
word aligned. Now the compiler should ensure this is the case, but I'm
always careful and pass things 'largest type first'. Similarly in structures
themselves.
It seems to prevent some oddities... |
|
|
samikool
Joined: 01 Apr 2021
Posts: 15
|
|
| Posted: Thu Jun 03, 2021 12:32 pm |
|
|
| Quote: |
I'd actually try reversing the variables in the function declaration.
|
I can confirm that this works.
So then this is definitely something in the compiler. I will add this to the email thread I have with CCS and hopefully they can find a fix for this.
As of now though, thank you for the solution! |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19513
|
|
| Posted: Fri Jun 04, 2021 1:16 am |
|
|
What you can do is explicitly add the aligned keyword.
__attribute__((aligned(0x2))
to the structure double declarations.
In the compiler help, it specifically says:
| Quote: |
By default the compiler will allocate a variable in the first free memory location.
|
Now the problem with this is that if you have something that requires
'word' alignment, following something that uses byte alignment, you can
end up with word memory fetches being attempted to on odd memory
location. Result should actually be an exception error. However it sounds as
if instead it is handling the access incorrectly... 
It is interesting that standard int16's, it does correctly word align. It
seems to be the larger types where it is failing to do this.
I remember finding a while ago, that you had to be 'careful' in structure
allocation, or it could give incorrect alignments, so have taken to explicitly
using 'largest first' order, or adding alignment declarations. |
|
|
samikool
Joined: 01 Apr 2021
Posts: 15
|
|
| Posted: Mon Jun 07, 2021 8:11 am |
|
|
On emailing with CCS they told me to include pcd_traps.c which I had not used before but indeed it seems to give a memory access violation.
Error:
| Code: |
ADDRESS FAULT PC:73A2 W0:DE3 W1:E12 W2:750A W3:C030 W4:AB5 W5:0 W6:DC8 W7:0 W8:0 W9:0 W10:0 W11:0 W12:0 W13:0 W14:6000 W15:46F8 PSVPAG:0 TBLPAG:0 INTCON2:0 CORCON:8
STACK (4) 0 8152 0 84FE
|
The program at 73A2:
| Code: |
.............................. slope s = {0, 0};
0734E 20DE44 MOV #DE4,W4 : W4 = DE4
07350 887094 MOV W4,E12 : E12 = W4
07352 026F74 000000 CALL 6F74 :
07356 B7ADEE MOV W0,DEE : DEE = W0
07358 886F81 MOV W1,DF0 : DF0 = W1
0735A 886F92 MOV W2,DF2 : DF2 = W2
0735C 886FA3 MOV W3,DF4 : DF4 = W3
0735E 20DE44 MOV #DE4,W4 : W4 = DE4
07360 887094 MOV W4,E12 : E12 = W4
07362 026F74 000000 CALL 6F74 :
07366 B7ADF6 MOV W0,DF6 : DF6 = W0
07368 886FC1 MOV W1,DF8 : DF8 = W1
0736A 886FD2 MOV W2,DFA : DFA = W2
0736C 886FE3 MOV W3,DFC : DFC = W3
0736E EF2DFE CLR DFE : DFE = 0
07370 EF2E00 CLR E00 : E00 = 0
07372 EF2E02 CLR E02 : E02 = 0
07374 EF2E04 CLR E04 : E04 = 0
07376 EF2E06 CLR E06 : E06 = 0
07378 EF2E08 CLR E08 : E08 = 0
0737A EF2E0A CLR E0A : E0A = 0
0737C EF2E0C CLR E0C : E0C = 0
.............................. s.m = m;
0737E F80DEE PUSH DEE : PUSH DEE to TOS
07380 F90DFE POP DFE : POP TOS to DFE
07382 F80DF0 PUSH DF0 : PUSH DF0 to TOS
07384 F90E00 POP E00 : POP TOS to E00
07386 F80DF2 PUSH DF2 : PUSH DF2 to TOS
07388 F90E02 POP E02 : POP TOS to E02
0738A F80DF4 PUSH DF4 : PUSH DF4 to TOS
0738C F90E04 POP E04 : POP TOS to E04
.............................. s.b = b;
0738E F80DF6 PUSH DF6 : PUSH DF6 to TOS
07390 F90E06 POP E06 : POP TOS to E06
07392 F80DF8 PUSH DF8 : PUSH DF8 to TOS
07394 F90E08 POP E08 : POP TOS to E08
07396 F80DFA PUSH DFA : PUSH DFA to TOS
07398 F90E0A POP E0A : POP TOS to E0A
0739A F80DFC PUSH DFC : PUSH DFC to TOS
0739C F90E0C POP E0C : POP TOS to E0C
..............................
.............................. setCalibration(connector, s); //doesn't work (v5.104) - hoping ccs will fix so leaving here
0739E 20DE30 MOV #DE3,W0 : W0 = DE3
073A0 20E121 MOV #E12,W1 : W1 = E12
073A2 09000F REPEAT #F : Repeat next instruction (F + 1) times
073A4 7818B0 MOV [W0++],[W1++] : [W1++] = [W0++]
073A6 20DFE0 MOV #DFE,W0 : W0 = DFE
073A8 20E141 MOV #E14,W1 : W1 = E14
073AA 09000F REPEAT #F : Repeat next instruction (F + 1) times
073AC 7818B0 MOV [W0++],[W1++] : [W1++] = [W0++]
|
Thank you for the info Ttelmah, you've been very helpful. I'll try to keep the thread updated as I talk with CCS and hopefully there can be some sort of permanent fix, rather than having to order parameter based on data size. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19513
|
|
| Posted: Mon Jun 07, 2021 11:15 am |
|
|
An address error trap is what I'd have expected. The alignment
declaration is the correct fix really. However what is odd is that the
compiler does automatically align int16's, but is not doing so for the
larger types. That is really a compiler fault. It ought to word align
all things unless you specifically use the packed declaration...
Looking at the assembler, you can see it is doing a 16bit transfer from
an odd address (#DE3). |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19513
|
|
| Posted: Tue Jun 08, 2021 12:35 am |
|
|
OK. Have done some playing.
If you declare a structure, it correctly 'word aligns', whatever you do.
The thing that goes wrong is very specifically passing a set of values to
a function, where there is a structure requiring word alignment, following
something that is an int8. However by default this seems to be handed
correctly.
The following things all fix this:
| Code: |
void setCalibration(uint16_t connector, slope s) //make sure the first
//thing is 'word sized.
void setCalibration(slope s, uint8_t connector) //put the non word sized
//thing after the structure
typedef struct __attribute__((aligned(2))) {
double m;
double b;
} slope;
//Force the 'slope' data type to be word aligned
|
You can very easily 'see' if the problem is going to happen, by looking at
the symbol table, and at where 'setCalibration.s' is actually located.
If this is on an odd address there is going to be a problem.
Now it is interesting, since a basic test, has this on an even address. I tried
dozens of configurations and all correctly gave even addresses. The only
way I could actually make it go wrong, was by explicitly using
the packed attribute on the structure.
So the compiler does seem to be correctly respecting the alignment rules.
Something in the code must be triggering it to permit this misalignment.
Have no idea 'what'!...
Do you want to post the whole of your processor 'setup' (fuses, #OPT,
etc. etc.). and what standard libraries you are using (obviously stdint.h etc.)
something is changing the default behaviour of the compiler in this regard.
Trying to work out what it is. |
|
|
samikool
Joined: 01 Apr 2021
Posts: 15
|
|
| Posted: Tue Jun 08, 2021 7:54 am |
|
|
Oh wow very interesting find.
Right now our main.h doesn't seem too crazy:
| Code: |
#ifndef MAIN_H
#define MAIN_H
#case
/******************************************************************************
* Include Files
*****************************************************************************/
#include <24FJ256GA106.h>
/******************************************************************************
* Constants
*****************************************************************************/
#define CODE_PROTECTION_ON 0
#define DEBUGGER_ON 1 // Set both debugger #defines to use printf ().
#define DEBUGGER_WITH_PRINTF 1 // Set both debugger #defines to use printf ().
/*---------------------------------------------------------------------------*/
// #device
/*---------------------------------------------------------------------------*/
// Sets the debugger to ON
#if DEBUGGER_ON == 1
// Allows DEBUGGER
#device ICSP=TRUE
#device ICD=3
#endif
/*---------------------------------------------------------------------------*/
// #use
/*---------------------------------------------------------------------------*/
#use delay(clock=32MHz,crystal=8MHz)
// Allows both the debugger and the printf ()
#if DEBUGGER_WITH_PRINTF == 1
// Turn on the RS-232 functionality.
//#use rs232 (ICD,XMIT=PIN_B4,RCV=PIN_B5) // ICD-U80 console
//#use rs232 (BAUD=115200,XMIT=PIN_G8,RCV=PIN_G7) // TP pins.
// Hardware UART1 using TP pins.
#pin_select U1TX=PIN_G8
#pin_select U1RX=PIN_G7
#use rs232(UART1, baud=115200, stream=TP)
// Or, for ICD Debugger I/O:
//#use rs232(ICD, stream=TP)
#define DEBUG_PRINTF printf
#define DEBUG_PRINT printf
#else
#define DEBUG_PRINTF(...)
#endif
// SPI
#pin_select SCK1OUT=PIN_D4
#pin_select SDI1=PIN_D5
#pin_select SDO1=PIN_D3
#use spi(MASTER, SPI1, MODE=0, BITS=8, stream=SPI1_MODE0, BAUD=2000000)
#use spi(MASTER, SPI1, MODE=1, BITS=8, stream=SPI1_MODE1, BAUD=2000000)
// SPI2
#pin_select SCK2OUT=PIN_B1
#pin_select SDI2=PIN_B2
#pin_select SDO2=PIN_B0
#use spi(MASTER, SPI2, MODE=0, BITS=8, stream=SPI2_MODE0, BAUD=2000000)
#use spi(MASTER, SPI2, MODE=1, BITS=8, stream=SPI2_MODE1, BAUD=2000000)
#WORD PMCON = getenv("PMCON")
#bit PMPEN = PMCON.15
/*---------------------------------------------------------------------------*/
// #FUSES
/*---------------------------------------------------------------------------*/
#FUSES NOWDT //No Watch Dog Timer
#FUSES CKSFSM //Clock Switching is enabled, fail Safe clock monitor is enabled
#FUSES NOIESO //wait for PLL start, to prevent changing SPI baud rate
#FUSES NOPROTECT // Disable code protection.
#endif /* MAIN_H */
|
I can double check but I do believe this is the only place we set/use fuses.
Library wise we use
| Code: |
#include <stdint.h>
#include <stdlib.h> // atoi ()
#include <stdlibm.h>
#include <stddef.h> // size_t
#include <stdbool.h> // bool
#include "pcd_traps.c"
#include "socket.c"
|
Otherwise everything else is our own libraries we've written for the project.
Hope that's helpful. |
|
|
samikool
Joined: 01 Apr 2021
Posts: 15
|
|
| Posted: Tue Jun 08, 2021 8:06 am |
|
|
Another very interesting find.
After finding and solving some other bugs in the program, I reran the old structure set up and the fault changed. This time I got an oscillator fault.
| Code: | OSCILLATOR FAULT PC:108AA W0:DE3 W1:E12 W2:31B0 W3:C033 W4:AB5 W5:0 W6:DC8 W7:0 W8:0 W9:0 W10:0 W11:0 W12:0 W13:0 W14:6000 W15:46D8 PSVPAG:0 TBLPAG:0 INTCON2:0 CORCON:8
STACK (4) 1 15CC 0 458C
|
The asm at that location seems to be the same line of that caused the previous crash, just a different reason.
| Code: | .............................. s.m = m;
10886 F80DEE PUSH DEE : PUSH DEE to TOS
10888 F90DFE POP DFE : POP TOS to DFE
1088A F80DF0 PUSH DF0 : PUSH DF0 to TOS
1088C F90E00 POP E00 : POP TOS to E00
1088E F80DF2 PUSH DF2 : PUSH DF2 to TOS
10890 F90E02 POP E02 : POP TOS to E02
10892 F80DF4 PUSH DF4 : PUSH DF4 to TOS
10894 F90E04 POP E04 : POP TOS to E04
.............................. s.b = b;
10896 F80DF6 PUSH DF6 : PUSH DF6 to TOS
10898 F90E06 POP E06 : POP TOS to E06
1089A F80DF8 PUSH DF8 : PUSH DF8 to TOS
1089C F90E08 POP E08 : POP TOS to E08
1089E F80DFA PUSH DFA : PUSH DFA to TOS
108A0 F90E0A POP E0A : POP TOS to E0A
108A2 F80DFC PUSH DFC : PUSH DFC to TOS
108A4 F90E0C POP E0C : POP TOS to E0C
..............................
.............................. setCalibration(connector, s); //doesn't work (v5.104) - hoping ccs will fix so leaving here
108A6 20DE30 MOV #DE3,W0 : W0 = DE3
108A8 20E121 MOV #E12,W1 : W1 = E12
108AA 09000F REPEAT #F : Repeat next instruction (F + 1) times
108AC 7818B0 MOV [W0++],[W1++] : [W1++] = [W0++]
108AE 20DFE0 MOV #DFE,W0 : W0 = DFE
108B0 20E141 MOV #E14,W1 : W1 = E14
108B2 09000F REPEAT #F : Repeat next instruction (F + 1) times
108B4 7818B0 MOV [W0++],[W1++] : [W1++] = [W0++]
108B6 02FEBA 000000 CALL FEBA : |
|
|
|
jeremiah
Joined: 20 Jul 2010
Posts: 1349
|
|
| Posted: Wed Jun 09, 2021 12:33 pm |
|
|
| Do you put your chip to sleep per chance? I've run into some PIC24 chips that have a chip errata where putting the chip to sleep can cause various TRAP conflicts (OSC, ADDR, etc). I don't know if this is one of them, but you might check the chip errata document for your chip on the Microchip website. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19513
|
|
| Posted: Wed Jun 09, 2021 11:35 pm |
|
|
Good suggestion.
A few things that might cause issues:
#pin_select SCK1OUT=PIN_D4
As a general comment, you should always map the SCK1IN as well (and
the same for SCK2). Some of the chips have the input clock path separate
from the output clock path, and not mapping both can cause erratic behaviour. So add:
#pin_select SCK1IN=PIN_D4
and a similar entry for SCK2.
Now Jeremiah's question about if you do use sleep is important. If you do,
you should change the clock to a non PLL based source before switching.
The chip has issues with restarting the PLL based clocks from sleep.
Also add a delay_cycles(1) as the instruction before the sleep. Again
there is an issue if any read instruction occur before the sleep.
The chip also has issues with restarting from a brownout.
Is it possible you use RB5 for anything needing open collector operation?.
I2C etc.. If so there is a hardware problem with this pin...
Are you running in the debugger mode?. If so, there is a really nasty
erratum about the UART. Could you switch to using UART2 instead of
UART1?.
I don't see a PSV setup?. If you are using PSV, there is an erratum on
this that can result in a false address trap.
Now I notice this:
| Quote: |
33.Module:Oscillator (Two-Speed Start-up)
Two-Speed Start-up is not functional. Leaving the
IESO Configuration bit in its default state
(Two-Speed Start-up enabled) may result in
unpredictable operation.
Work around
None. Always program the IESO Configuration bit
to disable this feature (CW2[15] = 0).
|
Check the fuses actually being generated by the compiler. Two speed
startup is the default on a lot of chips, and if this is being programmed
to the default, this could cause "unpredictable operation". Sounds rather
an 'ouch' one to me!....
The end of the .lst file shows the fuses. |
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
Powered by phpBB © 2001, 2005 phpBB Group
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
