PDA

View Full Version : Pointers/References?


htwiz2002
01-28-2007, 01:14 PM
:yikes: I'm trying to write a function that reads values from eeprom and stores their values into variables passed into the function. This is what it looks like:

:]

void Terminal_eepromreadset(unsigned char set, unsigned char *data1, unsigned char *data2, unsigned char *data3, unsigned char *data4)
{
//Consolidate this on verify that it works!!!
unsigned int address = set * 4 -1;
//*data1 = EEPROM_Read(address + 1);
//*data2 = EEPROM_Read(address + 2);
//*data3 = EEPROM_Read(address + 3);
//*data4 = EEPROM_Read(address + 4);
}
...
// later on in another function this is how it's called:
unsigned char version1;
unsigned char version2;
unsigned char version3
unsigned char version4;
Terminal_eepromreadset(0, version1, version2, version3, version4); break;


version1 - version 4 are all inited with 0.
For some reason I get the following warnings when I compile:
C:\frc\FrcCode_2007_8722\terminal.c:550:Warning [2054] suspicious pointer conversion
C:\frc\FrcCode_2007_8722\terminal.c:550:Warning [2054] suspicious pointer conversion
C:\frc\FrcCode_2007_8722\terminal.c:550:Warning [2054] suspicious pointer conversion
C:\frc\FrcCode_2007_8722\terminal.c:550:Warning [2054] suspicious pointer conversion


line 550 is the call to the function. If i comment it out, everything is okay. But i want the function to work!

Any ideas?

buddy.smith
01-28-2007, 01:38 PM
void Terminal_eepromreadset(unsigned char set, unsigned char *data1, unsigned char *data2, unsigned char *data3, unsigned char *data4)
{



Why not do this:

void Terminal_eepromreadset(unsigned char set, unsigned char data[4])
{
//Consolidate this on verify that it works!!!
unsigned int address = set * 4;
data[0] = EEPROM_Read(address);
data[1] = EEPROM_Read(address + 1);
data[2] = EEPROM_Read(address + 2);
data[3] = EEPROM_Read(address + 3);
}

Note that this is the same as:

void Terminal_eepromreadset(unsigned char set, unsigned char *data)




// later on in another function this is how it's called:
unsigned char version1;
unsigned char version2;
unsigned char version3
unsigned char version4;
Terminal_eepromreadset(0, version1, version2, version3, version4); break;



version1 is a 'char'. Terminal_eepromreadset wants a char*. To convert a char to a char *, you use the addressof operator (&):

Terminal_eepromreadset(0, &version1,.....)


Alternately, you can use my suggestion:

unsigned char version[4];
Terminal_eepromreadset(0, version);


In C, an array and a pointer are mostly equivalent. version is a char*, version[N] is a char.

Confusing as dirt? :)

ttyl,

--buddy

htwiz2002
01-28-2007, 09:49 PM
Why not do this:

void Terminal_eepromreadset(unsigned char set, unsigned char data[4])
{
//Consolidate this on verify that it works!!!
unsigned int address = set * 4;
data[0] = EEPROM_Read(address);
data[1] = EEPROM_Read(address + 1);
data[2] = EEPROM_Read(address + 2);
data[3] = EEPROM_Read(address + 3);
}

Note that this is the same as:

void Terminal_eepromreadset(unsigned char set, unsigned char *data)




version1 is a 'char'. Terminal_eepromreadset wants a char*. To convert a char to a char *, you use the addressof operator (&):

Terminal_eepromreadset(0, &version1,.....)


Alternately, you can use my suggestion:

unsigned char version[4];
Terminal_eepromreadset(0, version);


In C, an array and a pointer are mostly equivalent. version is a char*, version[N] is a char.

Confusing as dirt? :)

ttyl,

--buddy
Well, the idea is that I can simply specify a variable into the function so that the function can load the value into the variable, rather than having to call it and then store the information. its' purpose is to save space and ease of use, not reverse it. The point is to simply call the function to load four variables in the set (part of the autonomous routine). The other half of the function uses the same input to save it.

Is there a way to make the multi-variable-in-the-input idea work? It cleans the code up so much that I would rather keep it that way. Can someone at least tell me why I get that message about a "suspicious pointer conversion"? I'm sure the code would work if I could pass more than just one variable into the function (which is what it seems).

htwiz2002
01-28-2007, 10:56 PM
:yikes: I think I (might) have fixed it. (at least, it's not giving me errors anymore...)
The following is the example in entirety:

// Eepromhelper.c
#include "eeprom.h"
//Version verify codes:
unsigned char version1 = 0;
unsigned char version2 = 1;
unsigned char version3 = 1;
unsigned char version4 = 0;
//end verify codes...
// Verify codes are supposed to be for version check so in case the wrong version data is stored the robot will not use it (until saveing, which the robot will post a warning of overwrite (still in the works...))

// EEprom section:
void Terminal_eepromwriteset(char data1, char data2, char data3, char data4, unsigned char set)
{
//Consolidate this on verify that it works!!!
unsigned int address = set * 4 -1;
EEPROM_Write(address + 1, data1);
EEPROM_Write(address + 2, data2);
EEPROM_Write(address + 3, data3);
EEPROM_Write(address + 4, data4);
}
void Terminal_eepromreadset(unsigned char set, unsigned char *data1, unsigned char *data2, unsigned char *data3, unsigned char *data4)
{
//Consolidate this on verify that it works!!!
unsigned int address = set * 4 -1;
*data1 = EEPROM_Read(address + 1);
*data2 = EEPROM_Read(address + 2);
*data3 = EEPROM_Read(address + 3);
*data4 = EEPROM_Read(address + 4);
}
void Limit_S0witch_Max(unsigned char switch_state, unsigned char *input_value, unsigned char *yep){}
char Terminal_loadFromEeprom(char set)
{
char rtn = 0;
//This routine loads four char values from eeprom using a set switch te determine which bits to load.
//Set starts with zero and also indicates what address to use (starting with zero)
//Returns 0 if send to buffer success, 1 if read failed, 999 if set number is invalid
//Make sure this information is mirrored in Terminal_saveToEeprom() !!!
switch (set)
{
case 0: //this is an example set, meant only for verification of data...
//This is also viable for version verify, for automatic reset...
Terminal_eepromreadset(0, &version1, &version2, &version3, &version4); break;
default: return 999; break;
}
return rtn;
}
//
char Terminal_saveToEeprom(char set)
{
//This routine saves four char values to eeprom using a set switch te determine which bites to save.
//Set starts with zero and also indicates what address to use (starting with zero)
//Returns 0 if send to buffer success, 1 if buffer is full, 999 if set number is invalid
//Make sure this information is mirrored in Terminal_loadFromEeprom() !!!
if (EEPROM_Queue_Free_Space() > 4)
{
switch (set)
{
case 0: //this is an example set, meant only for verification of data...
//This is also viable for version verify, for automatic reset...
Terminal_eepromwriteset(version1, version2, version3, version4, 0); break;

default: return 999; break;
}
}
else
{
return 1;
}
return 0;
}
// End eeprom section...


The full source is included (just in case someone wants to see a W.I.P. :ahh:

Hope this can be helpful to some others

buddy.smith
01-29-2007, 02:12 PM
:yikes: I think I (might) have fixed it. (at least, it's not giving me errors anymore...)
The following is the example in entirety:

unsigned char version1 = 0;
unsigned char version2 = 1;
unsigned char version3 = 1;
unsigned char version4 = 0;


I was just suggesting the array method to show the possibility. You can do it the way you were doing it before.... (details below)



//end verify codes...
// Verify codes are supposed to be for version check so in case the wrong version data is stored the robot will not use it (until saveing, which the robot will post a warning of overwrite (still in the works...))

// EEprom section:
void Terminal_eepromwriteset(char data1, char data2, char data3, char data4, unsigned char set)
{
//Consolidate this on verify that it works!!!
unsigned int address = set * 4 -1;
EEPROM_Write(address + 1, data1);
EEPROM_Write(address + 2, data2);
EEPROM_Write(address + 3, data3);
EEPROM_Write(address + 4, data4);
}


What's the -1 for? Why not just set*4, then add 0, 1, 2, 3?



void Terminal_eepromreadset(unsigned char set, unsigned char *data1, unsigned char *data2, unsigned char *data3, unsigned char *data4)
{
//Consolidate this on verify that it works!!!
unsigned int address = set * 4 -1;
*data1 = EEPROM_Read(address + 1);
*data2 = EEPROM_Read(address + 2);
*data3 = EEPROM_Read(address + 3);
*data4 = EEPROM_Read(address + 4);
}


This looks right. I'd recommend changing 'data1' to pData1, and so forth. This lets you know that it's a pointer. I sometimes prefer to put the * next to 'char' as well, but that's something that C programmers argue about constantly :)

[QUOTE=htwiz2002;567274]

char Terminal_loadFromEeprom(char set)
{
char rtn = 0;
//This routine loads four char values from eeprom using a set switch te determine which bits to load.
//Set starts with zero and also indicates what address to use (starting with zero)
//Returns 0 if send to buffer success, 1 if read failed, 999 if set number is invalid
//Make sure this information is mirrored in Terminal_saveToEeprom() !!!
switch (set)
{
case 0: //this is an example set, meant only for verification of data...
//This is also viable for version verify, for automatic reset...
Terminal_eepromreadset(0, &version1, &version2, &version3, &version4); break;


I'd work on the formatting here. Move the 'break' to a line by itself.

Also, why are you using global variables here instead of declaring them inside this function?

Also, perhaps you should name your case....

#define VERSION_SET (0)
....
...
case VERSION_SET:
....




default: return 999; break;


Again i'd improve the formatting:

default:
return 999;
break;


I hope this doesn't come off as critical. I'm trying to be constructive and help you come up with code that is clear to the human.

A programmer's job is to write documentation that happens to compile.... :)

Suggestions: Use whitespace, it's free. Avoid global variables like the plague.

If you must use globals, stick a 'g' in front of the name, again so that you know it's a global.

Make all your globals 'static'. This means that they won't conflict with another global in a different file. That can cause huge headaches.

ttyl,

--buddy

htwiz2002
01-29-2007, 05:45 PM
I was just suggesting the array method to show the possibility. You can do it the way you were doing it before.... (details below)


What's the -1 for? Why not just set*4, then add 0, 1, 2, 3?

[QUOTE=htwiz2002;567274]

void Terminal_eepromreadset(unsigned char set, unsigned char *data1, unsigned char *data2, unsigned char *data3, unsigned char *data4)
{
//Consolidate this on verify that it works!!!
unsigned int address = set * 4 -1;
*data1 = EEPROM_Read(address + 1);
*data2 = EEPROM_Read(address + 2);
*data3 = EEPROM_Read(address + 3);
*data4 = EEPROM_Read(address + 4);
}


This looks right. I'd recommend changing 'data1' to pData1, and so forth. This lets you know that it's a pointer. I sometimes prefer to put the * next to 'char' as well, but that's something that C programmers argue about constantly :)


I'd work on the formatting here. Move the 'break' to a line by itself.

Also, why are you using global variables here instead of declaring them inside this function?

Also, perhaps you should name your case....

#define VERSION_SET (0)
....
...
case VERSION_SET:
....



Again i'd improve the formatting:

default:
return 999;
break;


I hope this doesn't come off as critical. I'm trying to be constructive and help you come up with code that is clear to the human.

A programmer's job is to write documentation that happens to compile.... :)

Suggestions: Use whitespace, it's free. Avoid global variables like the plague.

If you must use globals, stick a 'g' in front of the name, again so that you know it's a global.

Make all your globals 'static'. This means that they won't conflict with another global in a different file. That can cause huge headaches.

ttyl,

--buddy
!!! sorry, i should have mentioned it's incomplete! :)
Anyways, I'll make some major changes....

With the "break" thing I was trying to consolidate things so that I wouldn't need to scroll half a mile just to get from section to section, while (hopefully) improving readability (since most of the code that I combine to one line doesn't need (normally) to be changed or read much.

Also, my globals (generally) have the file's name prefixed so as to help prevent them being used (normally, again) outside of the file. Problems do arise when I try to access them o-o-f, but since I moved the handler code i haven't needed to (much).

Again, code in beta, far from finished. I was using the version globals for version checking and for the test set. They will become #define s soon enough, as well as many of the other global variables.

buddy.smith
01-29-2007, 07:04 PM
No worries.

I personally use vim with some nice things for auto-indenting code.

Also, if your functions are so long that they won't fit on one screen, it's a good sign that your function might be too long.

But I've digressed far beyond the initial topic.

If you require more assisstance, I'll be trolling this forum. I'd love to help any other teams that need it.... :)

Another thing you can do, is group the things that will be in your eeprom into a struct:


struct {
unsigned char version[4];
unsigned char foo;
unsigned char bar;
} gEepromStruct;

Then you can change your eeprom read/write:

[CODE]
void eeprom_read(void *pData, unsigned char size)
{
/* Read data into pData here */

}

void test( void )
{
eeprom_read( &gEepromStruct, sizeof(gEepromStruct));
}

Now your eeprom read/write are generic, and you can put anything you want in the eeprom without changing that code.

ttyl,

--buddy

Shinigami2057
01-29-2007, 10:21 PM
Also, you can prevent global variables from being accessed outside of their file by declaring them as static. For example:

In user_routines_fast.c:

static int auto_counter;

void User_Autonomous_Code(void)
{
...


In user_routines.c:

extern int auto_counter; /* This will not compile! */


It is important to note that the static qualifier has a different meaning if you use it in the scope of a function.