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 support@ccsinfo.com

Funny struct behavior

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



Joined: 23 Mar 2017
Posts: 27

View user's profile Send private message

Funny struct behavior
PostPosted: Mon Jul 17, 2017 3:02 pm     Reply with quote

Hi,

I'm using CCS PCW compiler version 5.070 and I'm seeing some very strange behavior with one of my structs. The struct is my data struct. The issue is when the mot_curr variable is before the discretes variable, the discretes variable seems to be ignored. Even during initialization it doesn't get zero'd out. If I swap the position where the discretes variable comes first, the problem moves to the mot_curr variable (ignored and can't be initialized). The variables don't cause an error in the compile but the variables are full of junk.

Here is my code. Please go easy on me. I'm a hardware guy. Embarassed

My PIC is 16F1829



Code:

#include <DataLoggerv1p1.h>


#include <stdio.h>

// Global constants and variables

#define  CURR_BUFFER 50
#define  TEMP_BUFFER 5


int16  buffer_address;
int8  count0p1s;

void get_current(int8);
void get_temp(int8);
void get_discretes(int8);

struct {
   int32 motor_min;
   int8  motor_sec;
   int8  motor_0p1sec;
   int32 PDU_hr;
   int8  PDU_min;
   int8  PDU_sec;
} run_time;

struct {
   int8  curr_samples;
   int8  temp_samples;
   int8  temp1[TEMP_BUFFER];
   int8  temp2[TEMP_BUFFER];
   int8  mot_curr[CURR_BUFFER];
   int8  discretes[CURR_BUFFER];     
} data;

struct {
   int1  mot_running;
   int1  fault;
} flag;


   
#INT_TIMER2
void  TIMER2_isr(void)
{
   count0p1s++;
   
   if (flag.mot_running)
   {
      run_time.motor_0p1sec++;      // increment 0.1 second counter
      if (data.curr_samples >= 20)
      {
         data.curr_samples = 0;
      }
      get_current(data.curr_samples);    // get the current
      get_discretes(data.curr_samples);    // get the current

      data.curr_samples++;               // increment the data samples counter
     
      if (run_time.motor_0p1sec >= 10)    // every second of motor running
      {
         run_time.motor_0p1sec = 0;       // reset 0.1 second counter
         run_time.motor_sec++;            // increment the 1 second counter

         if (data.temp_samples >= 5)
         {
            data.temp_samples = 0;
         }
         get_temp(data.temp_samples);           // get the temperature data

         data.temp_samples++;
         output_toggle(PIN_C7);
         

         if (run_time.motor_sec >= 60)    // every minute of motor running
         {
            run_time.motor_sec = 0;       // reset the 1 second counter
            run_time.motor_min++;         // increment the minute counter
         }
      }
   }     

   if (count0p1s >= 10)                   // every second of power on
   {
      count0p1s = 0;                      // reset the 0.1 second counter
      run_time.PDU_sec++;                 // increment the second counter
         
      if (run_time.PDU_sec >= 60)         // every minute of power on
         {
            run_time.PDU_sec = 0;         // reset the 1 second counter
            run_time.PDU_min++;           // increment the minute counter
           
            if (run_time.PDU_min >= 60)   // every hour of power on
            {
               run_time.PDU_min = 0;      // reset the minute counter
               run_time.PDU_hr++;         // increment the hour counter
            }
         }
   }

}// end of TIMER2_isr

#INT_TBE
void  TBE_isr(void)
{

}

#INT_RDA
void  RDA_isr(void)
{

}

#INT_IOC
void  IOC_isr(void)
{

}

void main()
{
   int8 i=0;
   
   setup_adc_ports(sAN2|sAN3|sAN7);
   setup_adc(ADC_CLOCK_INTERNAL);
   init_ext_eeprom();

   setup_timer_2(T2_DIV_BY_64, 38, 10);
   set_timer2(0);


   run_time.PDU_sec = 0;
   run_time.PDU_min = 0;
   run_time.PDU_hr = 0;

   run_time.motor_0p1sec = 0;
   run_time.motor_sec = 0;
   run_time.motor_min = 0;
   data.curr_samples = 0;
   data.temp_samples = 0;
   
   
   
   for (i=0; i<CURR_BUFFER; i++)
   {
      data.mot_curr[i] = 0;
      data.discretes[i] = 0;
   }
   
   for (i=0; i<TEMP_BUFFER; i++)
   {
      data.temp1[i] = 0;
      data.temp2[i] = 0;
   }
   
   flag.mot_running = 1;
   
//   enable_interrupts(INT_TIMER2);
   enable_interrupts(INT_TBE);
   enable_interrupts(INT_RDA);
//   enable_interrupts(INT_AD);
   enable_interrupts(INT_IOC);
   enable_interrupts(GLOBAL);

   
   while(TRUE)
   {
      if (!input(PIN_A5))
      {
         disable_interrupts(INT_TIMER2);
      }
      else
      {
         enable_interrupts(INT_TIMER2);
      }
     
      //TODO: User Code
   }

}// end of main


void get_current(int8 ibuffer)
{
   set_adc_channel(3);
   while (!adc_done());
   data.mot_curr[ibuffer] = read_adc();

}// end of get_current


void get_temp(int8 ibuffer)
{

   set_adc_channel(2);
   while (!adc_done());
   data.temp1[ibuffer] = read_adc();

   set_adc_channel(7);
   while (!adc_done());
   data.temp2[ibuffer] = read_adc();

}// end of get_temp



void get_discretes(int8 ibuffer)

   data.discretes[ibuffer] = 0;

   data.discretes[ibuffer] = input(PIN_C0);
   data.discretes[ibuffer] = input(PIN_C1) || data.discretes[ibuffer]<<1;
   data.discretes[ibuffer] = input(PIN_C2) || data.discretes[ibuffer]<<1;
   data.discretes[ibuffer] = input(PIN_B7) || data.discretes[ibuffer]<<1;
   data.discretes[ibuffer] = input(PIN_C6) || data.discretes[ibuffer]<<1;
   data.discretes[ibuffer] = input(PIN_A5) || data.discretes[ibuffer]<<1;
   
}// end of get_discretes


Last edited by Aileyus on Mon Jul 17, 2017 3:42 pm; edited 2 times in total
PCM programmer



Joined: 06 Sep 2003
Posts: 21708

View user's profile Send private message

PostPosted: Mon Jul 17, 2017 3:26 pm     Reply with quote

What is your PIC ?
Aileyus



Joined: 23 Mar 2017
Posts: 27

View user's profile Send private message

PostPosted: Mon Jul 17, 2017 3:34 pm     Reply with quote

PCM programmer wrote:
What is your PIC ?


Embarassed I thought that information was already in the code but it's in the .h. Anyway I'm using PIC16F1829. I have added this info in the original post now.

Thanks

Incidentally, while I've initially set the variable array size to 50 on these two variables, I am currently limiting my use to 20 just so I can see what's happening and debug the code. Eventually, I will want to set the array size of these two variables to 200 each.


Last edited by Aileyus on Mon Jul 17, 2017 3:49 pm; edited 1 time in total
PCM programmer



Joined: 06 Sep 2003
Posts: 21708

View user's profile Send private message

PostPosted: Mon Jul 17, 2017 3:49 pm     Reply with quote

Quote:

The issue is when the mot_curr variable is before the discretes variable,
the discretes variable seems to be ignored. Even during initialization it
doesn't get zero'd out.

How do you know this ? How do you examine the array ?
Aileyus



Joined: 23 Mar 2017
Posts: 27

View user's profile Send private message

PostPosted: Mon Jul 17, 2017 3:53 pm     Reply with quote

PCM programmer wrote:
Quote:

The issue is when the mot_curr variable is before the discretes variable,
the discretes variable seems to be ignored. Even during initialization it
doesn't get zero'd out.

How do you know this ? How do you examine the array ?


I use the watch list.

when I step through this section of the code:

for (i=0; i<CURR_BUFFER; i++)
{
data.mot_curr[i] = 0;
data.discretes[i] = 0;
}

data.mot_curr[i] each get set to 0 and data.discretes[i] each get ignored (retain the garbage originally in the variables). Swap the order of the variables in the struct and the problem swaps. data.discretes[i] get initialized and data.mot_curr[i] get ignored.
PCM programmer



Joined: 06 Sep 2003
Posts: 21708

View user's profile Send private message

PostPosted: Mon Jul 17, 2017 4:37 pm     Reply with quote

I tested it with MPLAB vs. 8.92 simulator with CCS vs. 5.070.
I setup a watch window and looked at the data structure.
Instead of clearing the bytes, I set them to 'M' for mot_curr elements
and 'D' for the discretes elements. I put a breakpoint on the discretes
update line, and ran to it and then clicked the "run" button repeatedly
to watch the updates.

mot_curr appears to update properly. discretes doesn't update correctly.
The CCS code is doing linear memory accesses with the FSR register.
I didn't research it beyond this point because I wanted to report what
I found so far. You are correct, there is a problem.

I have to go do something else now for a while.
Aileyus



Joined: 23 Mar 2017
Posts: 27

View user's profile Send private message

PostPosted: Tue Jul 18, 2017 7:15 am     Reply with quote

Should I report this as a compiler bug or is this likely a bug in my code?
RF_Developer



Joined: 07 Feb 2011
Posts: 839

View user's profile Send private message

PostPosted: Tue Jul 18, 2017 8:09 am     Reply with quote

Aileyus wrote:
Should I report this as a compiler bug or is this likely a bug in my code?


I strongly suspect neither. I've also looked at the code produced by my PCW 5.067 and like PCM I can't find much wrong with it. Its doing much the same thing.

What I have found in the past is that the various debuggers have a lot of trouble with correctly dealing with structures. I use the CCS IDE debugger most of the time, but the MPLAB one has roughly similar issues. They often can't get the types right in structures and tend to make a right pig's ear of unions. Just because it looks wrong in the debugger is not necessarily any indication of a problem in the code.

As such, I would look carefully at the debugger, because it could very well be lying to you. If in doubt look at the raw RAM dump rather than the interpreted watches, even then there can be issues, so beware!

There are other things in your code to consider. Have a look again at the way the ADC works. You need to wait a short time after selecting a channel. You can configure the ADC to do the wait for you on this and many other PICs. read_adc() will, by default, trigger and then wait for the conversion to finish, so you don't need to wait for adc_done(), which in any case is in the wrong place in your code. Also ADC_CLOCK_INTERNAL is unsuitable for general use in all but a handful of PICS. Instead use a clock divider from the main processor clock.

Interrupts like the serial reads, e.g. INT_RDA, require the source of the interrupt to be serviced for the interrupt flag to be cleared (it is cleared automatically). In the case or RDA you must read the character from the hardware, even if you don't store it anywhere. Similarly for IOC where you must read the relevant change notification register. INT_TBE is a somewhat special case, and needs handling carefully.

Your timer 2 ISR is doing a lot. Things like taking readings with the ADC take up a lot of time, relatively speaking. It would be better to make it just make the passage of time, i.e. be a clock tick, and deal with the timed actions in mainline code, i.e. in your main loop.

Your now infamous structures are arranged as structures with arrays of variables. Where you have more than one array indexed by the same index, it often makes sense to organise them as arrays of structures that contain the variables. So you will have a structure with an array of structures rather than a structure with arrays (plural) of variables. This will also alter the arrangement of data in the structure and may well alter the results you are seeing in the debugger, for better or worse.

Personally I don't much like to use the get/set routine thing outside of objects, though that's a matter of style and taste, except that it adds a layer of stack to your code, and stack is very limited on most PICs :-( Likewise, I'd avoid calling routines in ISRs wherever practical.

That said, I doubt your get_discretes() routine will do what you're expecting. You are logical oring the values together (||) where I think you probably want to bitwise or them (|). Also you do a lot of array accesses, which are slow and code heavy. Instead I suggest you might want to build your bits into a simple local variable and only write them into the array at the end.

They are just a few things to think about.
Aileyus



Joined: 23 Mar 2017
Posts: 27

View user's profile Send private message

PostPosted: Tue Jul 18, 2017 9:39 am     Reply with quote

Thanks RF. I'll look at each of your suggestions and try to rewrite/re-architect my code around them.

Told you I was a hardware guy. Laughing
Ttelmah



Joined: 11 Mar 2010
Posts: 19463

View user's profile Send private message

PostPosted: Tue Jul 18, 2017 12:21 pm     Reply with quote

Have to agree with RF_Developer.

I've found with all the debuggers (CCS, MPLAB, and MPLAB-X), that if the variable is slightly more complex than a basic variable or array, the IDE's get confused about how things are placed, and watches rarely work.
The solution is to explicitly declare your own watch at the location of the component you require.
Some ways of doing this:
1) Look in the symbol table for where the structure is. Calculate from this where the component is you want to look at and declare a watch to this location with the right type.
2) Declare a pointer in the code. Point this to the component, and then declare a watch on this. Again specify the type manually.
3) Use #byte to assign a dummy variable to the component, at the same memory location. This then works for a watch.
Aileyus



Joined: 23 Mar 2017
Posts: 27

View user's profile Send private message

PostPosted: Tue Jul 18, 2017 3:17 pm     Reply with quote

RF:

With regards to moving the data gathering from my interrupt:

The only thing this code will ever do is capture data and write it to EEPROM at the end of a run. When power is removed, I'm holding up the VDD so I have time to dump the data to EEPROM in case of power failure.

My intent it to capture current and command data every 0.1 seconds (during the tmr2 interrupt). The interrupt only happens every 0.1 seconds and at 1 MHz, that's a lot of time (100,000 cycles). I was expecting the ADC was fast enough. The worst case is when I capture 3 ADC bytes and the discrete word between a 0.1 second interrupt.

I tried to move the capture data code to the main but then I have to set a flag every 0.1 second and 1 second and clear the flag when I'm done. This is basically creating my own pseudo-interrupt. The only thing that will happen in the main loop is to capture this data and maintain those flags.

The other interrupts were put in there by the code generation tool when I used the wizard. I've already gotten rid of the ADC interrupt as it turns out I didn't need it. I'm not sure if I'll need the rest of them or not as I haven't gotten that far.

The next stage in my code is to write data to the EEPROM. The final stage will be to set up an RS232 communication so we can initialize product information in the EEPROM and read that information and data. During these RS232 communications and EEPROM writes, the tmr2 interrupt will be disabled.

Do you think I should still move the data capturing out of the interrupt and into main? If so, I'll do that.

Also, when I changed the get_discretes code per your recommendation, the data started showing up in RAM. It's still not working in the watch area but at least I can see it in the RAM so I know it's working now.

Either way, I thank you a lot for your input (and the others). I learned a lot more than I expected in just one thread.

I have posted the current code just because. I'm still pondering at how to change my struct of arrays into arrays of structs.

Code:
#include <DataLoggerv1p1.h>


#include <stdio.h>

// Global constants and variables

#define  CURR_BUFFER 200
#define  TEMP_BUFFER 20


int16  buffer_address;
int8  count0p1s;

void get_current(int8);
void get_temp(int8);
void get_discretes(int8);

struct {
   int32 motor_min;
   int8  motor_sec;
   int8  motor_0p1sec;
   int32 PDU_hr;
   int8  PDU_min;
   int8  PDU_sec;
} run_time;

struct {
   int8  curr_samples;
   int8  temp_samples;
   int8  temp1[TEMP_BUFFER];
   int8  temp2[TEMP_BUFFER];
   int8  mot_curr[CURR_BUFFER];
   int8  discretes[CURR_BUFFER];     
} data;

struct {
   int1  mot_running;
   int1  fault;
} flag;


   
#INT_TIMER2
void  TIMER2_isr(void)
{
   count0p1s++;
   
   if (flag.mot_running)
   {
      run_time.motor_0p1sec++;      // increment 0.1 second counter
      if (data.curr_samples >= 20)
      {
         data.curr_samples = 0;
      }
      get_current(data.curr_samples);    // get the current
      get_discretes(data.curr_samples);    // get the current

      data.curr_samples++;               // increment the data samples counter
     
      if (run_time.motor_0p1sec >= 10)    // every second of motor running
      {
         run_time.motor_0p1sec = 0;       // reset 0.1 second counter
         run_time.motor_sec++;            // increment the 1 second counter

         if (data.temp_samples >= 5)
         {
            data.temp_samples = 0;
         }
         get_temp(data.temp_samples);           // get the temperature data

         data.temp_samples++;
         output_toggle(PIN_C7);
         

         if (run_time.motor_sec >= 60)    // every minute of motor running
         {
            run_time.motor_sec = 0;       // reset the 1 second counter
            run_time.motor_min++;         // increment the minute counter
         }
      }
   }     

   if (count0p1s >= 10)                   // every second of power on
   {
      count0p1s = 0;                      // reset the 0.1 second counter
      run_time.PDU_sec++;                 // increment the second counter
         
      if (run_time.PDU_sec >= 60)         // every minute of power on
         {
            run_time.PDU_sec = 0;         // reset the 1 second counter
            run_time.PDU_min++;           // increment the minute counter
           
            if (run_time.PDU_min >= 60)   // every hour of power on
            {
               run_time.PDU_min = 0;      // reset the minute counter
               run_time.PDU_hr++;         // increment the hour counter
            }
         }
   }

}// end of TIMER2_isr

//!#INT_TBE
//!void  TBE_isr(void)
//!{
//!
//!}
//!
//!#INT_RDA
//!void  RDA_isr(void)
//!{
//!
//!}
//!
//!#INT_IOC
//!void  IOC_isr(void)
//!{
//!
//!}
//!



void main()
{
   int8 i=0;
   
   setup_adc_ports(sAN2|sAN3|sAN7);
   setup_adc(ADC_CLOCK_DIV_2);
   init_ext_eeprom();

   setup_timer_2(T2_DIV_BY_64, 38, 10);
   set_timer2(0);


   run_time.PDU_sec = 0;
   run_time.PDU_min = 0;
   run_time.PDU_hr = 0;

   run_time.motor_0p1sec = 0;
   run_time.motor_sec = 0;
   run_time.motor_min = 0;
   data.curr_samples = 0;
   data.temp_samples = 0;
   
   
   
   for (i=0; i<CURR_BUFFER; i++)
   {
      data.mot_curr[i] = 0;
      data.discretes[i] = 0xFF;
   }
   
   for (i=0; i<TEMP_BUFFER; i++)
   {
      data.temp1[i] = 0;
      data.temp2[i] = 0;
   }
   
   flag.mot_running = 1;
   
//   enable_interrupts(INT_TIMER2);
//   enable_interrupts(INT_TBE);
//   enable_interrupts(INT_RDA);
//   enable_interrupts(INT_AD);
//   enable_interrupts(INT_IOC);
   enable_interrupts(GLOBAL);

   
   while(TRUE)
   {
      if (!input(PIN_A5))
      {
         disable_interrupts(INT_TIMER2);
      }
      else
      {
         enable_interrupts(INT_TIMER2);
      }
     
      //TODO: User Code
   }

}// end of main


void get_current(int8 ibuffer)
{
   set_adc_channel(3);
   data.mot_curr[ibuffer] = read_adc();

}// end of get_current


void get_temp(int8 ibuffer)
{

   set_adc_channel(2);
   data.temp1[ibuffer] = read_adc();

   set_adc_channel(7);
   data.temp2[ibuffer] = read_adc();

}// end of get_temp



void get_discretes(int8 ibuffer)

   int8  dword = 0;

   dword = input(PIN_C0);
   dword = input(PIN_C1) | dword<<1;
   dword = input(PIN_C2) | dword<<1;
   dword = input(PIN_B7) | dword<<1;
   dword = input(PIN_C6) | dword<<1;
   dword = input(PIN_A5) | dword<<1;
   data.discretes[ibuffer] = dword;
   
}// end of get_discretes
jeremiah



Joined: 20 Jul 2010
Posts: 1341

View user's profile Send private message

PostPosted: Wed Jul 19, 2017 6:16 am     Reply with quote

Aileyus wrote:

I have posted the current code just because. I'm still pondering at how to change my struct of arrays into arrays of structs.


Well, one way is to change:

Code:

struct {
   int8  curr_samples;
   int8  temp_samples;
   int8  temp1[TEMP_BUFFER];
   int8  temp2[TEMP_BUFFER];
   int8  mot_curr[CURR_BUFFER];
   int8  discretes[CURR_BUFFER];     
} data;


to

Code:


typedef struct{
   int8 temp1;
   int8 temp2;
} some_type_name;

typedef struct{
   int8  mot_curr;
   int8  discretes; 
} another_type_name;

struct {
   int8  curr_samples;
   int8  temp_samples;
   some_type_name    some_var_name[TEMP_BUFFER];
   another_type_name another_var_name[CURR_BUFFER];   
} data;




On the topic of getters/setters. I actually find them useful and easier to maintain than dealing with the data structures directly. There have been many times where I needed to later add some extra code around getting/setting a data structure (due to a new requirement/interface/discovery/etc.) and having them in getters/setters made that much easier and cleaner. I'm also able to give them more readable names in some cases, which has been nice. If function call overhead is really a concern, then I either inline the functions or use reference parameters (which forces an inline).
Aileyus



Joined: 23 Mar 2017
Posts: 27

View user's profile Send private message

PostPosted: Wed Jul 19, 2017 10:17 am     Reply with quote

I used the structs from RF's recommendation and Jeremiah's example and it works great. There appears to be no way to call the variables in the watch list though. It really doesn't matter as I can read the RAM and see that my variables are being initialized and written to.

Thanks for all the help.
Aileyus



Joined: 23 Mar 2017
Posts: 27

View user's profile Send private message

PostPosted: Wed Jul 19, 2017 11:10 am     Reply with quote

Aileyus wrote:
There appears to be no way to call the variables in the watch list though.


Scratch that. When I type "data" into the watch list, I get the whole enchilada. I just wasn't expecting it to work that way. Also, the watch list now works for all of the struct.

Very Happy
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