CCS C Software and Maintenance Offers
FAQFAQ   FAQForum Help   FAQOfficial CCS Support   SearchSearch  RegisterRegister 

ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 

CCS does not monitor this forum on a regular basis.

Please do not post bug reports on this forum. Send them to CCS Technical Support

To override the compiler's intent on disabling interrupts?

 
Post new topic   Reply to topic    CCS Forum Index -> General CCS C Discussion
View previous topic :: View next topic  
Author Message
kaz



Joined: 15 May 2009
Posts: 12

View user's profile Send private message

To override the compiler's intent on disabling interrupts?
PostPosted: Thu Jul 02, 2009 7:13 pm     Reply with quote

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

View user's profile Send private message

PostPosted: Thu Jul 02, 2009 7:52 pm     Reply with quote

How to create a 2nd instance of the math routines:
http://www.ccsinfo.com/forum/viewtopic.php?t=25464&start=4
FvM



Joined: 27 Aug 2008
Posts: 2337
Location: Germany

View user's profile Send private message

PostPosted: Thu Jul 02, 2009 11:39 pm     Reply with quote

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







PostPosted: Fri Jul 03, 2009 4:32 am     Reply with quote

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

View user's profile Send private message AIM Address

second that on another front
PostPosted: Fri Jul 03, 2009 3:30 pm     Reply with quote

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

View user's profile Send private message

PostPosted: Fri Jul 03, 2009 3:54 pm     Reply with quote

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

View user's profile Send private message

PostPosted: Fri Jul 03, 2009 10:19 pm     Reply with quote

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

View user's profile Send private message

PostPosted: Fri Jul 03, 2009 11:46 pm     Reply with quote

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

View user's profile Send private message

PostPosted: Sat Jul 04, 2009 12:10 am     Reply with quote

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

View user's profile Send private message

PostPosted: Sat Jul 04, 2009 1:44 am     Reply with quote

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







PostPosted: Sat Jul 04, 2009 3:20 am     Reply with quote

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

View user's profile Send private message

PostPosted: Sat Jul 04, 2009 1:00 pm     Reply with quote

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.
Display posts from previous:   
Post new topic   Reply to topic    CCS Forum Index -> General CCS C Discussion All times are GMT - 6 Hours
Page 1 of 1

 
Jump to:  
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