| View previous topic :: View next topic |
| Author |
Message |
Aileyus
Joined: 23 Mar 2017
Posts: 27
|
| Funny struct behavior |
 Posted: Mon Jul 17, 2017 3:02 pm |
 |
|
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. 
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
|
|
| Posted: Mon Jul 17, 2017 3:26 pm |
|
|
| What is your PIC ? |
|
|
Aileyus
Joined: 23 Mar 2017
Posts: 27
|
|
| Posted: Mon Jul 17, 2017 3:34 pm |
|
|
| PCM programmer wrote: | | What is your PIC ? |
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
|
|
| Posted: Mon Jul 17, 2017 3:49 pm |
|
|
| 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
|
|
| Posted: Mon Jul 17, 2017 3:53 pm |
|
|
| 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
|
|
| Posted: Mon Jul 17, 2017 4:37 pm |
|
|
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
|
|
| Posted: Tue Jul 18, 2017 7:15 am |
|
|
| 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
|
|
| Posted: Tue Jul 18, 2017 8:09 am |
|
|
| 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
|
|
| Posted: Tue Jul 18, 2017 9:39 am |
|
|
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.  |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19540
|
|
| Posted: Tue Jul 18, 2017 12:21 pm |
|
|
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
|
|
| Posted: Tue Jul 18, 2017 3:17 pm |
|
|
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: 1354
|
|
| Posted: Wed Jul 19, 2017 6:16 am |
|
|
| 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
|
|
| Posted: Wed Jul 19, 2017 10:17 am |
|
|
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
|
|
| Posted: Wed Jul 19, 2017 11:10 am |
|
|
| 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.
 |
|
|
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
