 |
 |
| View previous topic :: View next topic |
| Author |
Message |
kaz
Joined: 15 May 2009
Posts: 12
|
| To override the compiler's intent on disabling interrupts? |
 Posted: Thu Jul 02, 2009 7:13 pm |
 |
|
Chip: PIC16F886
Version: 4.093
IDE: MPLAB 8.33
I keep getting these messages:
| Code: |
>>> Warning 216 "Main.c" Line 322(0,1): Interrupts disabled during call to prevent re-entrancy: (send_byte)
>>> Warning 216 "Main.c" Line 322(0,1): Interrupts disabled during call to prevent re-entrancy: (@DIV3232)
|
How exactly is this done? I can't find it in the disassembly listing (searched for modifications to SFR's INTCON, PIE1, PIE2), and I'm concerned that somehow: Timer2 (disabled in a parent function of send_byte()), is being disabled forever, or enabled-prematurely and causing re-entries.
Anyway, for future knowledge, I would greatly enjoy to know how to prevent CCS from disabling interrupts during a call. |
|
 |
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
|
|
FvM
Joined: 27 Aug 2008
Posts: 2337
Location: Germany
|
|
| Posted: Thu Jul 02, 2009 11:39 pm |
|
|
To answer the first part of your question, interrupt disabling is generally done in a safe way, involving save and restore of the global interrupt enable state. So you mainly should fear additional interrupt latencies with lengthy math operations. This problem is actually brought on already by using math (e.g. a division) in an interrupt function.
Regarding send_byte, it's possibly "unsafe" by design, in so far that you can produce inconsistent output by using it in an interrupt function and the main code simultaneously. In my opinion, the said warning should be regarded also as a pretty strong hint, that something should be better avoided in your code. But sometimes, it cant. |
|
|
Ttelmah
Guest
|
|
| Posted: Fri Jul 03, 2009 4:32 am |
|
|
I'd also be seriously looking at whether you can avoid using 32bit arithmetic in the interrupt....
Remember even on a 40Mhz PIC, a single 32bit division, takes over 100uSec. In general, it is often possible to 'cheat' and use other alternatives (scaling numbers so they fit into smaller types, or choosing values that result in binary divisions).
Turning off interrupts, if the same code is used both inside, and outside the interrupt, is _essential_ on the PIC, not 'voluntary', so you need either to duplicate the code to avoid re-entrancy (duplicate maths routines etc.), or re-think your approach to move routines to one location. For example, you could consider possibly having the actual 'send' routine, being interrupt driven. Then use simple routines that add bytes to a buffer for this, and have versions written so that they use a semaphore, or their own interrupt protection, to avoid this problem.
Best Wishes |
|
|
asmboy
Joined: 20 Nov 2007
Posts: 2128
Location: albany ny
|
| second that on another front |
| Posted: Fri Jul 03, 2009 3:30 pm |
|
|
re 32 bit math after finding out the hard way - re atoi32()
Due to excess code space hogged in PCM -
I abandoned using the function to pass 32 bit integers in the range of 0-130,000 - opting instead for TWO different but related calls to atol() and passing either a value that is 0-65535 or one with args starting at 0 that is added to 65536 to get the upper half of the option range - and storing the sum in an int32.
Other than for bit related operations - I've found int32 to be a
space hog to be avoided in all ways - at all times. |
|
|
kaz
Joined: 15 May 2009
Posts: 12
|
|
| Posted: Fri Jul 03, 2009 3:54 pm |
|
|
Err. I don't use any division/multiplication in my interrupts, ever. Especially the 32-bit variation. Or I'm fairly certain I don't. Just about 99% of the heavy code occurs in the main program/loop, and that last 1% is just because of the many if-else statements I have in a timer. And I've made very-sure that interrupts only queue heavy-operations for the main loop to execute.
In any case, I removed half of my program and the crash disappeared, but so did the warning about interrupts disabled in those particular functions. I'm still wary that it those may be the problem. I don't want to duplicate functions (or shouldn't need to(?), because these heavy functions aren't even used inside the interrupt routines), because the chip's already 98% full at no-debug, release-build.
Edit: Also, thanks Ttelmah, because this is like the fourth time you've helped me. |
|
|
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
|
| Posted: Fri Jul 03, 2009 10:19 pm |
|
|
| Quote: | I don't use any division/multiplication in my interrupts, ever.
>>> Warning 216 "Main.c" Line 322(0,1): Interrupts disabled during call to prevent re-entrancy: (@DIV3232) |
That may be, but if you're getting this warning, you're almost certainly
using some division function in an isr. It's probably the modulus
operator "%". You could post your isr code. |
|
|
kaz
Joined: 15 May 2009
Posts: 12
|
|
| Posted: Fri Jul 03, 2009 11:46 pm |
|
|
I am almost 99% certain I do not. I will however post the code (and the sub-children possible of calling) in here. I admire your patience for reading these :) .
Although reflecting upon what's been said in here, I think I'll move the input handling() code into main.
I don't have any data structures, so usually if there's a variable-type that isn't recognized, it's a BYTE. There's also an alarming amount of globals in this program, but I'm just not sure of how to cut them down.
As requested though (Have I mentioned I'm 98% certain I don't use a division operation?):
| Code: |
#int_ccp1
void ccp1_isr()
{
FlowRate_Clicks_5++; //Counting a pulse. Will tally them at 5 seconds.
clear_interrupt(INT_CCP1);
}
#int_ccp2
void ccp2_isr()
{
FlowRate_Clicks_5++; //Counting a pulse. Will tally them at 5 seconds.
clear_interrupt(INT_CCP2);
}
//This one wasn't written by me, except for the array-of-pointers.
#int_ssp
void ssp_isr(void)
{ //SPI activity
// read in a 16 bit command
static unsigned int16 spi_command = 0; // spi_command
static unsigned int16 spi_working_in = 0; // spi incoming working byte
static unsigned int16 K_Temp_LS; // temp storage in case we want to change K Factor
static unsigned int16 K_Temp_MS; // temp storage in case we want to change K Factor
static short More_Spi_Data = FALSE; // flag set to tell us the next 16bit word is new data to store
// flag doesn't work well here extern short int Clear_Sensor_Readings_Flag; // set by SPI to tell us to clear
extern unsigned int Clear_Sensor_Readings_Req; // set by SPI to tell us to clear
//Prevent byte_count from being reset.
delay100ms = 10;
byte_count ++; // point to next byte
/* if (byte_count ==2)
{//debug break
byte_count = 2;
} */
switch (byte_count ){
case 1:
SPI_OUT1 = (SSPBUF<<8); // get msbyte
SSPBUF=(unsigned int)(SPI_OUT&0xff); // put lsbyte of output data in buffer, waiting for clock
break;
case 2:
SPI_OUT1 |= SSPBUF; // get lsbyte
SSPBUF=(unsigned int)(SPI_OUT1>>8); // put msbyte of output data in buffer, waiting for clock
break;
case 3:
spi_command = (SSPBUF<<8); // get msbyte
SSPBUF=(unsigned int)(SPI_OUT1&0xff); // put lsbyte of output data in buffer, waiting for clock
break;
case 4:
spi_command |= SSPBUF; // get lsbyte
//SSPBUF=(unsigned int)(SPI_OUT>>8); // put msbyte of output data in buffer, waiting for clock
break;
case 5:
SPI_OUT1 = (SSPBUF<<8); // get msbyte
SSPBUF=(unsigned int)(SPI_OUT&0xff); // put lsbyte of output data in buffer, waiting for clock
break;
case 6:
SPI_OUT1 |= SSPBUF; // get lsbyte
SSPBUF=(unsigned int)(SPI_OUT1>>8); // put msbyte of output data in buffer, waiting for clock
break;
case 7:
SPI_OUT = (SSPBUF<<8); // get msbyte
SSPBUF=(unsigned int)(SPI_OUT1&0xff); // put lsbyte of output data in buffer, waiting for clock
break;
case 8:
SPI_OUT |= SSPBUF; // get lsbyte
SSPBUF=(unsigned int)(SPI_OUT>>8); // put msbyte of output data in buffer, waiting for clock
break;
default:
break;
}//switch
byte_count &= 0x07; //Wrap-around.
// if (byte_count==0)
// {
// spi_working_in = (SSPBUF<<8); // get msbyte, put in place, msbyte of SPI_OUT was just sent
// SSPBUF=(unsigned int)(SPI_OUT&0xff); // put lsbyte of output data in buffer, waiting for clock
// byte_count++; // point back to msbyte
// }
// else
// {
// spi_working_in |= SSPBUF; // get lsbyte, put in place
// if (More_Spi_Data == FALSE)
// spi_command = spi_working_in; // complete command received
// else {} // else, keep the spi_command the same and let the switch statement process
// byte_count = 0; // point back to lsbyte
/* 0x1000 - read device id
0x1001 - read HW/SW rev
0x1002 - read defined Flow meter (SIGNET=0x01,MC_FLOW_METER=0x02)
0x1003 - read Water Pressure
0x1004 - read Water Pressure_Average
0x1005 - read Water Pressure_Max
-- Truncated for readability --
*/
if (byte_count == 4)
{ // our time slot has arrived
// if(spi_command == 0) SPI_OUT = 0;
/* else */
if((spi_command <= 0x1010) && (spi_command >= 0x1000))
{ SPI_OUT = *(SPI_reponses[spi_command & 0xff]); } //SPI_Responses is an array of pointers to int16.
else
{ switch (spi_command)
{ case 0x101A:
if (More_Spi_Data == TRUE)
{
K_Temp_MS = spi_working_in;
More_Spi_Data = FALSE; // no more data following
}
else
More_Spi_Data = TRUE; // next spi 16 bit word will be K_Temp_MS
break;
case 0x101B:
if (More_Spi_Data == TRUE)
{
K_Temp_LS = spi_working_in;
More_Spi_Data = FALSE; // no more data following
}
else
More_Spi_Data = TRUE; // next spi 16 bit word will be K_Temp_LS
break;
case 0x101C:
if (More_Spi_Data == TRUE)
{
power_of = spi_working_in;
K_Factor = ((K_Temp_MS << 16) || K_Temp_LS); // complete K_Factor update
More_Spi_Data = FALSE; // no more data following
}
else
More_Spi_Data = TRUE; // next spi 16 bit word will be K_Temp_LS
break;
case 0x1011:
if (More_Spi_Data == TRUE)
{
Averaging_Period = spi_working_in;
More_Spi_Data = FALSE; // no more data following
}
else
More_Spi_Data = TRUE; // next spi 16 bit word will be K_Temp_LS
break;
case 0x4402:
Clear_Sensor_Readings_Req = TRUE;
break;
default:
break;
} // end of switch
SPI_OUT = 0x00;
}
SSPBUF=(SPI_OUT>>8); // put msbyte of output data in buffer, waiting for clock
} // end of our time slot
}
#endif SPI_H
#int_timer2
void timer2_isr()
{ // timer 2 is set to interrupt every 10msec
// and is used for scanning.
const unsigned int16 FlowRate_Sentivity = 70;
static u8 Consec_Count = 0;
static u8 Last_Keycode = 0;
static u8 Last_Sent_Keycode = 0;
static u8 delay_15m = 180; //15*(60/5);
static u8 idle_10m = 120; //10*60/5;
u8 Keys_Pressed = 0;
BOOLEAN Should_Send = FALSE;
//Scan first set of keys.
output_high(SCAN1); //Blinking LEDs make SCAN1 undeterminable.
//Turn it _off for now.
output_low(SCAN0);
delay_us(5);
Keys_Pressed |= (input_b() & 0x30) >> 4;
output_high(SCAN0);
output_low(SCAN1);
delay_us(5);
Keys_Pressed |= (input_b() & 0x30) >> 2;
output_low(SCAN0);
Keys_Pressed ^= 0x0F; //Inverted logic (pull-ups).
//By now: There is Last_keycode, which was the keycode scanned last time.
//There's also consec_count, which says how many times that key was scanned.
//And Last_Sent_Keycode, which ensures that each signal sent is unique.
//And the way this is handled took me incredibly long, because I kept approaching it from
//the wrong angle. I made a flow-chart though for documentation.
if(Last_Keycode == Keys_Pressed) //Shouldn't send on good ticks.
{ Consec_Count++; }
else //But on the release of the button.
{ if( Keys_Pressed == 0 && Consec_Count > BTN_PRESS_COUNT) //Button was released after hold.
{ Should_Send = TRUE; } //But not more than a second.
Consec_Count = 0;
}
if(Consec_Count == BTN_HOLD_SEC) //Send for every second held down.
{ Consec_Count = 0;
Keys_Pressed += (Last_Sent_Keycode & 0xF0) + 0x10; //Add on a second.
Should_Send = TRUE; //BUG: No defines here, likely bug place.
}
if(Should_Send)
{ if((Last_Keycode & ACTIVE_KEY_MASK) && (Keys_Pressed == 0)) //Sends if button released.
{// GUI_Handler(Last_Keycode);
Key_Signal = Last_Keycode;
GUI_Handler();
Last_Sent_Keycode = Last_Keycode;
} //Current keys pressed are none. Send last keycode.
else if(Keys_Pressed & 0xF0) //Otherwise it's an idle-signal.
{// GUI_Handler(Keys_Pressed);
Key_Signal = Keys_Pressed;
GUI_Handler();
Last_Sent_Keycode = Keys_Pressed;
} //Send the updated version.
}
Last_Keycode = Keys_Pressed;
delay5s++; // inc 50msec counter
if((Keys_Pressed & ACTIVE_KEY_MASK) != 0) //Reset the 10-minute idle timer.
{ idle_10m = 120; }
if (delay5s >= 250) //250 * 10mS for 2.5 seconds, plus one extra bit flag 5 seconds.
{ delay5s=0;
if(delay5s_ext) //Only perform this every other 2500 msec.
{ Read_Sensor_Time = TRUE;
GUI_needs_update = TRUE;
delay_15m--;
if(delay_15m == 0)
{ save_FlowRate_Total_Clicks = TRUE;
delay_15m = 180; //15*(60/5);
}
if(idle_10m) //Decrement the 10-minute idle timer
idle_10m--; //every 5 seconds.
else
{ Switch_UI(FLOWRATE_VIEW);
idle_10m = 120; //Delay the switch another 10 minutes.
//So it won't occur every 5 seconds.
}
}
delay5s_ext ^= TRUE;
}
if(delay100ms == 0)
{ byte_count = 0; }
else
{ delay100ms--; }
//Blinking Functionality. This is a LOONG interrupt.
if((FlowRate > FlowRate_Sentivity) && (!(delay5s & 0x08))) //Flowrate is greater than 7.0 units.
{ output_high(SCAN0); }
else
{ output_low(SCAN0); }
if(Pressure > 100 && !(delay5s & 0x08)) //Pressure is above 10.0 PSI.
{ output_high(SCAN1); }
else
{ output_low(SCAN1); }
}
|
GUI_Handler()
| Code: |
void GUI_Handler()
{ //Universal key-signal. Switch to the Calibration menu if
//the down-key is held for more than 3 seconds.
if(Key_Signal & KEY_DOWN && (Key_Signal >> 4) > 3)
{ Switch_UI(CAL_MENU);
return;
}
(*GUI_Handler_routines[Active_UI])(); //This should be with an argument,
//but having different function pointer
//types takes up more memory.
//GUI_Handler_routines[] is an array of (void)function(void) pointers.
//"State machine"
//Takes Key_Signal, and forwards it to the individual UI handler.
if(Key_Signal & ACTIVE_KEY_MASK) GUI_needs_update = TRUE;
//Nearly every handler did this, so I moved it here instead of
//having it in every handler.
}
|
void Switch_UI(screen UI)
| Code: |
void Switch_UI(screen New_UI) //Switches to a new UI, IE: Switch_UI(GALLON_VIEW).
{ Active_UI = New_UI;
GUI_needs_init = TRUE;
GUI_needs_update = TRUE;
}
|
A lot of the handler() routines just call GUI_cycle():
| Code: |
inline void GUI_cycle() //BREAKS: This breaks if another screen is added on the main menu.
{ Active_UI++;
if(Key_Signal & KEY_UP)
{ Active_UI++;
if(Active_UI > 7) Active_UI = 1;
GUI_needs_init = TRUE;
}
if(Key_Signal & KEY_DOWN)
{ Active_UI--;
if(Active_UI == 0) Active_UI = 7;
GUI_needs_init = TRUE;
}
Active_UI--;
}
|
Except for Cal_Menu_Handler()
| Code: |
void Cal_Menu_Handler()
{ if(Key_Signal & KEY_ENTER && (Key_Signal >> 4) > 2) //Holding Enter key for more than 2 seconds.
{ Switch_UI(GALLON_VIEW);
}
if(Key_Signal & KEY_RIGHT)
{ Switch_UI(Next_UI);
}
if(Key_Signal & KEY_DOWN)
{ Next_UI--;
if(Next_UI < OPT_AVERAGING_VIEW)
Next_UI = OPT_X1000_VIEW;
}
if(Key_Signal & KEY_UP)
{ Next_UI++;
if(Next_UI > OPT_X1000_VIEW)
Next_UI = OPT_AVERAGING_VIEW;
}
}
|
Anyway, I'm going to start moving the input_handling code into the main loop, because at least then input won't change Active_UI in the middle of a screen update. |
|
|
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
|
| Posted: Sat Jul 04, 2009 12:10 am |
|
|
I didn't see anything obvious in your posted code. Are you still getting
the re-entrancy warning about @DIV3232 ?
If so, comment out all the code in each isr, one at a time. If the warning
goes away, that will tell you which isr is causing the problem.
Then comment out 1/2 of the code in that isr, which will tell you which
half is causing the problem. Then comment out 1/2 of that remaining
code, and so on, until you narrow it down to the exact line.
Also, post your PIC and your compiler version. |
|
|
kaz
Joined: 15 May 2009
Posts: 12
|
|
| Posted: Sat Jul 04, 2009 1:44 am |
|
|
I removed the GUI_Handler() from the timer2 ISR and put it in main, so now it has no chance to execute within the main loop code (which probably spends 50% of its time in DIV3232 or send_byte() ).
I may revert the code to just satisfy curiosity of why DIV3232 disabled interrupts, but will do so in the morning.
Much thanks for the support and insight, it really does make my day. |
|
|
Ttelmah
Guest
|
|
| Posted: Sat Jul 04, 2009 3:20 am |
|
|
A number of comments apply:
Your switch statements, like the SPI_command' one, are _very_ inefficient. Because you are using 16bit switch values, and have a default statement, these will be performed as effectively a series of 16bit value 'if' tests. The time needed for these will be large. Your 'byte count' one, will also be performed with if tests, because it contains a 'default'. In CCS, it is more efficient, to code with a simple 'range' test, followed by a switch statement for the values inside the range, with no default statement. This generates a 'table jump' routine, while if a default is present, 'if' tests for the individual values are performed.
For your SPI command one, access the upper byte of the value, and if it is '44', then check for 02 in the lower byte, and do your 0x4402 routine. If not, test if the upper is 0x10, and if it is, then test the four possible low byte combinations. If you use 'make8', or a union to access the bytes, this will result in single instruction tests, and save many dozens of instructions over the existing code!...
Then, do you really need the delay_us instructions?. It is often worth coding your own simple 'macro' delay, using the delay_cycle instruction, for short signal delays like this. This avoids the re-entrancy problem from delays, in a possibly 'simpler' manner, than having to duplicate the delay library (a search here will find how to do this).
You realise, that every routine whose address is held in the array 'GUI_Handler_routines', will also be tested for re-entrancy, and every subroutine in these....
Best Wishes |
|
|
kaz
Joined: 15 May 2009
Posts: 12
|
|
| Posted: Sat Jul 04, 2009 1:00 pm |
|
|
The CCS compiler would make a table for me :) ?
Before, it used to be an even larger switch-statement, but I tried to cut it down and use that table of pointers to variables. But it makes perfect sense that I should be using 1-byte switches and range tests. That'll help a lot in the ROM and speed department.
I always thought CCS was awesome enough to generate a table for me, but was never sure how to convince it to. Thanks for the feedback, I'll make sure to use it when I friggin overhaul the SPI.
As for the delays in timer2, I just wanted to play it safe as I'm not sure how long it takes for the pin to go high. I'll try removing them or using a few NOPs.
I realize the every routine in that array will be tested, but I think they only go one level deeper into Switch_UI(), which is just three assignment functions I got tired of typing repeatedly. I wanted to keep the handlers lightweight since they were handled in an ISR.
Much thanks for your comments, I'll keep the switch statement one specifically in mind when using CCS. |
|
|
|
|
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
