RTOS/EK-TM4C1294XL: Setting an IP Address from a stored value in the EEPROM

First, please use paste code (the </> icon). If you don't the formatting is lost.

Now onto the meat of the matter.

This is odd, not wrong but decidedly odd, it leads you astray later.

Thodden Saporetti26 said:

uint32_t temp[1];

This should be either explicitly unsigned char or uint8_t. As written it could be signed or unsigned (compiler dependent) leading to interesting sign extension issues

Thodden Saporetti26 said:

char address[4];

sizeof(temp) is the size of a pointer to a uint32_t not the size of a uint32_t. The latter is what you want. You are fortunate that they happen to be the same.

Thodden Saporetti26 said:

EEPROMRead(temp, 100, sizeof(temp));

Here you mix signed and unsigned integers.

Thodden Saporetti26 said:

address[0] = temp[0]&255;
address[1] = (temp[0] >> 8) & 255;
address[2] = (temp[0] >> 16) & 255;
address[3] = (temp[0] >> 24) & 255;

Here you are using sprint (not a good idea) and passing in a value of unknown sign, it should be converted to a signed integer I believe.

Thodden Saporetti26 said:

sprintf(addressString, "%u.%u.%u.%u",address[3],address[2],address[1],address[0]);

And this I what is probably causing your problem. You seem to be expecting this to copy a string. That's not what you are doing, look a little closer.

Thodden Saporetti26 said:

*LocalIPAddr = *addressString;

Robert

I am open to suggestions on better coding methods since coding isn't my primary or even secondary job requirement, so I don't program on a daily basis.  I'll say why I made my choices though. 

I took this code directly from the example given in the TIVA C user Peripheral Driver Library user guide.  I had some issues when using a single variable to read a page(word?) from the EEPROM with this command.  So I simply set up a single element array.  The example specifically used sizeof() for getting the size of a multi-element array, it just happens that my code the array is one element.  As far as I can tell, sizeof(array) returns the size of the array, not the size of the pointer to the array.

In this case, I could have just as easily put 4 instead and saved some processor time, but as I'm not pressed for processor usage, I prefer to use the same method everywhere.  

Robert Adsett said:

sizeof(temp) is the size of a pointer to a uint32_t not the size of a uint32_t. The latter is what you want. You are fortunate that they happen to be the same.

Thodden Saporetti26

EEPROMRead(temp, 100, sizeof(temp));

Traditionally, a char variable is neither signed nor unsigned unless you specify that it is though I have encountered versions of C where it's default is one or the other.  In this case, it doesn't make a difference for how I'm using it, though for safety sake, I probably should force it to be unsigned in the declaration.

Robert Adsett said:

Here you mix signed and unsigned integers.

Thodden Saporetti26

address[0] = temp[0]&255;
address[1] = (temp[0] >> 8) & 255;
address[2] = (temp[0] >> 16) & 255;
address[3] = (temp[0] >> 24) & 255;

I specifically want an unsigned value, since IP addresses are unsigned, and %u will automatically interpret the char variables as unsigned.  However, using sprintf was a shortcut on my side since it was the easiest way I knew of to convert several numbers into a single string.  I am open to suggestions if there is a better way.

Robert Adsett said:

Here you are using sprint (not a good idea) and passing in a value of unknown sign, it should be converted to a signed integer I believe.

Thodden Saporetti26

sprintf(addressString, "%u.%u.%u.%u",address[3],address[2],address[1],address[0]);

And this is where I'm sure the issue is.  This was simply my last attempt at making it work, and I went through several iterations of trying to copy the string.  This was the only one that didn't crash the code until after the function was exited, the rest crashed directly after running the line of code.  I also tried strcpy(), as well as tried manually copying the string with the following code:

    addressPointer = addressString;
    i = 0;
    while (*addressPointer != '\0')
        LocalIPAddr[i++] = *addressPointer++;

Robert Adsett said:

And this I what is probably causing your problem. You seem to be expecting this to copy a string. That's not what you are doing, look a little closer

Thodden Saporetti26

*LocalIPAddr = *addressString;

So my issue still occurs regardless of how I try to copy the address string.  For testing purposes, I also tried LocalIPAddr[0] = '1';  to just change the first character in LocalIPAddr, and it will crash, even though '1' is the same character that's currently stored there.  So I'm guessing there's something special about LocalIPAddr that I don't know about.  Or maybe I'm forgetting something about manipulating strings in C.  

If I replace the string copying methods in the function with just    LocalIPAddr = "192.168.6.2';     it works fine.  Unfortunately, it doesn't resolve the issue, since that means the IP Address is hard coded.

Thodden Saporetti26 said:

I had some issues when using a single variable to read a page(word?) from the EEPROM with this command. 

And they were?

Thodden Saporetti26 said:

  The example specifically used sizeof() for getting the size of a multi-element array, it just happens that my code the array is one element.  As far as I can tell, sizeof(array) returns the size of the array, not the size of the pointer to the array.

Yep, you're right, I was thinking of a different case.

Thodden Saporetti26 said:

In this case, I could have just as easily put 4 instead and saved some processor time

Nope the processor time taken would have been identical. This is determined at compile time.

Thodden Saporetti26 said:

Traditionally, a char variable is neither signed nor unsigned

Again, no. It's compiler specific. In fact the compiler must specify whether it is signed or unsigned in the documentation. The user should not depend on the representation being either. If you are using char to represent a small number you should explicitly choose either signed or unsigned. If you are using char to represent a visible character you should use plain char.

Thodden Saporetti26 said:

I specifically want an unsigned value, since IP addresses are unsigned, and %u will automatically interpret the char variables as unsigned.  However, using sprintf was a shortcut on my side since it was the easiest way I knew of to convert several numbers into a single string.  I am open to suggestions if there is a better way.

Robert Adsett

Here you are using sprint (not a good idea) and passing in a value of unknown sign, it should be converted to a signed integer I believe.

Thodden Saporetti26

sprintf(addressString, "%u.%u.%u.%u",address[3],address[2],address[1],address[0]);

My point on the signed/unsigned here is that the address parameters are implicitly converted to ints. Such conversions are potentially troublesome relying on rules that I doubt more than a handful of people (probably all writers of compilers or test suites) have fully internalized.

sprintf (Damn, I think the original was autocorrected from sprintf to sprint)  is dangerous from several perspectives

1. It uses a lot of stack and in some cases heap. This is especially a problem in embedded applications and embedded demos that often have a small stack allocated.
2. It has no overflow protection
3. It uses a complicated syntax , although frequent users will have memorized it
4. I takes a lot of code space.

I've not seen anyone make a compelling case that it should not be forbidden in embedded applications. As far as alternatives, itoa is simpler, smaller and safer.

Thodden Saporetti26 said:

Robert Adsett

And this I what is probably causing your problem. You seem to be expecting this to copy a string. That's not what you are doing, look a little closer

Thodden Saporetti26

*LocalIPAddr = *addressString;

OK, this and

Thodden Saporetti26 said:

LocalIPAddr = "192.168.6.2'; 

this are completely and utterly different.

One copies (or attempts to) a character over another character. What happens when that character to be overwritten is in ROM?

The other replaces a pointer to a string with a pointer to a different string.

So different types and different memory locations.

Robert

Thodden Saporetti26 said:

and %u will automatically interpret the char variables as unsigned. 

A subtle note, I  don't think I properly addressed. Not many, if any introductions to the *printf family do either.

%u will interpret the associated signed integer at the passed variable location as if it were a signed integer. That's a subtle difference but it could potentially be important is unusual circumstances. It also points out that the *printf family is vulnerable to the wrong sized types being passed. If the type sizes do not match the expected not only will the wrong result be 'printed' but any result after will be as well, indeed it is possible that you will get a processor fault.

Robert

Robert Adsett said:

Thodden Saporetti26

Traditionally, a char variable is neither signed nor unsigned

Again, no. It's compiler specific. In fact the compiler must specify whether it is signed or unsigned in the documentation.

Yes, this means 'c' or any other character constant has a defined sign.

Robert

Thodden Saporetti26 said:

Robert Adsett

Here you mix signed and unsigned integers.

Thodden Saporetti26

address[0] = temp[0]&255;
address[1] = (temp[0] >> 8) & 255;
address[2] = (temp[0] >> 16) & 255;
address[3] = (temp[0] >> 24) & 255;

I specifically want an unsigned value, since IP addresses are unsigned, and %u will automatically interpret the char variables as unsigned.

I think you may have missed my point here.

Without looking it up can you tell me the various implicit conversions made in this block of code and in what order?

Robert

I'm going to simply reply to this one to address everything, including some of the responses that came afterwards.

I understand these are completely different. As I mentioned, this was my last iteration and I basically just throwing code at the wall to see what stuck.  I tried standard copying methods, and even simply changing a single character in LocalIPAddr, IE LocalIPAddr[0] = '1'.  Just for a test, I also tried *LocalIPAddr = *LocalIPAddr;   which should do nothing, but all of them caused the same issue.  As far as I can tell, the memory allocated to LocalIPAddr prevents individual changes, and the act of trying to change it is causing the crash.  However, the pointer itself is not locked, which is what led me to I try the code I posted and why it crashes at the end of the function, rather than in the middle of it.

This discussion did lead me to figuring out a workaround.  I'm not happy with it but it does work.  I create a global char array for the new IP Address(So it doesn't get cleared), and then point LocalIPAddr to the new array.  This does mean whatever memory block was originally allocated to LocalIPAddr can't be reclaimed.  

Robert Adsett said:

Thodden Saporetti26

*LocalIPAddr = *addressString;

OK, this and

Thodden Saporetti26

LocalIPAddr = "192.168.6.2'; 

this are completely and utterly different.

As for my issues with the EEPROMread function, it may simply been I used an unsigned int instead of uint32_t, and I just switched it to an array instead.  It also occurs to me that I may have made a simple mistake by just sending the variable instead of a pointer to the variable.  I honestly don't remember since I had the issue weeks ago, and resolved it this way.  As for sizeof(), I didn't realize the compiler uses it as a constant.  That is good to know.

Using itoa() is a good suggestion, but which library is it included in?  It is not in the CCSV stdlib.h where atoi, and strtol and similar functions are(I checked).  I suppose it doesn't matter, I can build my own, I was being lazy.  Still, I have been using vsprintf for formatting any strings I need to send out via ethernet.  I've already made sure the stack and the heap are large enough for this, and the application doesn't require high speed performance, though I'm still all for better methods of formatting and generating output strings with multiple variables.  

Robert Adsett said:

address[0] = temp[0]&255;
address[1] = (temp[0] >> 8) & 255;
address[2] = (temp[0] >> 16) & 255;
address[3] = (temp[0] >> 24) & 255;

I think you may have missed my point here.

Without looking it up can you tell me the various implicit conversions made in this block of code and in what order?

I might be misinterpreting what you are asking me.  But when setting a char = int, the char variable is set to the least significant byte of the integer, and that's the only implicit conversion that takes place, and it is the last to occur.  Since I'm only using a one variable in the equation portion and it is a 32 bit integer, the math and logic are done entirely as an integer so there are no implicit conversions here, it treats everything as if it is a 32 bit value. Since this is the case, technically address[0] = temp[0]&255; is equivalent to address[0] = temp[0];  

For:   address[1] = (temp[0] >> 8) & 255;  The right bit shift of 8 takes place first, shifting the second most significant byte to the least significant byte, then the bitwise & takes place, and finally the least significant byte is put into address[1].  And again, & 255 is technically unneeded, since only the least significant byte is stored in the char.   If the equations weren't so simple, I doubt I could answer without having to look up the order of operations, for instance if an int was divided by a float.

IF a char is treated as signed, the number placed into the char will be negative is the most significant bit is 1, and then it will interpret the value as a negative in two's complement format.  This will be done regardless of whether the integer was signed, unsigned or if the original number was negative or positive.  However, CCSV for all intents and purposes treats a char as unsigned.  I do agree that I should specify it is unsigned, simply for due diligence if the code was ever ported to a compiler that treats a char as signed.  

But none of this discussion deals with the underlying problem, though I do appreciate it since as I said, I'm not primarily a coder, and good practice is always important to know.  And as I said at the beginning I found a work around, though it doesn't answer my question as to why I can't change the value pointed to by LocalIPAddr.  

Thodden, I think you lack knowledge of pointers in C, and this forum is more dedicated to hardware.

Try a tutorial: www.cprogramming.com/.../lesson6.html Pointers require a bit more understanding than the normal C syntax. if statements, operators, and functions are all much simpler to understand.

If you understood what you were doing, you would be ashamed of the code you posted.

Thodden Saporetti26 said:

Just for a test, I also tried *LocalIPAddr = *LocalIPAddr;   which should do nothing,

Not true. Given a sufficient advanced compiler that might translate to a NOP but not necessarily. It would depend on what LocalIPAddr had been declared as.

However, I suspect most compilers would not optimize it out. In that case given an initial declaration of

char *LocalIPAddr = "192.168.1.1";

and a followup assignment of

*LocalIPAddr = *LocalIPAddr;

What memory location is being written (I.e. what kind of memory)?

As a side note, this assignment is explicitly forbidden in the C standard.

Thodden Saporetti26 said:

This discussion did lead me to figuring out a workaround.  I'm not happy with it but it does work.  I create a global char array for the new IP Address(So it doesn't get cleared), and then point LocalIPAddr to the new array.  This does mean whatever memory block was originally allocated to LocalIPAddr can't be reclaimed.  

I don't have the declaration of LocalIPAddr nor any documentation on it's declaration (you should have those), but I would not be surprised if this is required.

Thodden Saporetti26 said:

I might be misinterpreting what you are asking me.  But when setting a char = int, the char variable is set to the least significant byte of the integer, and that's the only implicit conversion that takes place, and it is the last to occur.

Nope, every single line of that code block has multiple implicit conversions.

Thodden Saporetti26 said:

I found a work around, though it doesn't answer my question as to why I can't change the value pointed to by LocalIPAddr. 

You also haven't shown us what LocalIPAddr is declared as.

Thodden Saporetti26 said:

I'm not primarily a coder, and good practice is always important to know.

There is a reason I keep suggesting PC-Lint

Robert

Thodden Saporetti26 said:

As for my issues with the EEPROMread function, it may simply been I used an unsigned int instead of uint32_t,

That's highly unlikely to be the root cause.

Robert

I'm sorry, when I said it should do nothing, I meant the value should literally be identical.  It shouldn't cause a crash because the value was set incorrectly.  It's being set to itself, so it shouldn't create a fatal error, yet it does.

Robert Adsett said:

Thodden Saporetti26

Just for a test, I also tried *LocalIPAddr = *LocalIPAddr;   which should do nothing,

Not true. Given a sufficient advanced compiler that might translate to a NOP but not necessarily. It would depend on what LocalIPAddr had been declared as.

Robert Adsett said:

You also haven't shown us what LocalIPAddr is declared as.

This may be the source of my problem.  LocalIPAddr is declared through the TI-RTOS files.  I set the static IP address in the cfg file under IP-General Settings, and it's creates own files automatically with these settings.  I'm new to using Code Composure studio, but if I use F3 to open the declaration, I see this:

char *LocalIPAddr = "192.168.1.2";

Which is the initial static IP address I entered.  Only reason I knew about this variable at all is I followed an older forum topic discussing this.  To make sure I have access to LocalIPAddr, I'm declaring at the top of my code:

extern char *LocalIPAddr;

It's entirely possible I'm missing something obvious.  I spent some time investigating this before replying and it appears the program will crash if I try to write to any spot in a string declared using

char *string = "xyz";
char temp;

So if I do

temp = string[1]; 

temp is set to 'y', but the program crashes when I try the reverse, 

string[1] = temp;

Just declaring a char array works perfectly, as well as pointers set to a char array.  So normally I work around this by just creating a char array for LocalIPAddr, except I don't have control over how LocalIPAddr is declared, since it is created by the TI-RTOS side.  So ignoring any of my other glaring non-standard code issues we've been discussing, this is the root of where my issue is.  As I mentioned, I do have a viable work around, but I'd love to know what's causing this.

This is more of an TI-RTOS question, which it seems is now combined with the hardware forums.  However, I am regretting that I posted the last code I used during the day to troubleshoot and find exactly where the issue was occurring.  It was late in the work day, and it has clearly detracted from finding what the issue is.  You are right I should never have posted that particular line, because it is wrong and rather embarrassing.  It did, however, give me useful information.  I'll list chain of code I tried, which I should have done in the first place.  The first thing I tried was

sprintf(LocalIPAddr, "%u.%u.%u.%u",address[3],address[2],address[1],address[0]);

Since the program crashed on that line, I created my own string, addressString,  and tried strcpy(LocalIPAddr, addressString);  Again, it crashed in the function, even when I tried a hard coded string.   Next I tried do to it manually with this code:

addressPointer = addressString;
i = 0;
while (*addressPointer != '\0')
    LocalIPAddr[i++] = *addressPointer++;

Which is pretty standard I think, but it also crashed when it ran the line LocalIPAddr[i++] = *addressPoint++;  Now it is fair to say I'm rusty on dealing with pointers and strings, but I'm believe that's a correct way to work with them.  I also tried several variants of that line.  So I tried simply changing a single character in a couple ways:

*LocalIPAddr = '1';

LocalIPAddr[1] = '9';

Both of which crashed instantly.  That's why I tried the more outrageous code that you rightfully took exception with, and it didn't crash until after the function ended rather than directly following the line of code.  Since this is a different result, it gave me a lead on a workaround I found today(Create a new string, point LocalIPAddr to that string instead.)  

Now part of the issue is LocalIPAddr is a pointer variable that is created by the TI-RTOS, so I'm not sure how it's declared other than it's a pointer.  In writing this and reading another response, I found the declaration that the RTOS side created.  

char *LocalIPAddr = "192.168.1.2";

Now I have done some further testing, and you can read it in my other post.  But the short version is, declaring any string in this method will cause a crash if I try to write to a specific location in it.  

Thodden Saporetti26 said:

This may be the source of my problem.  LocalIPAddr is declared through the TI-RTOS files.  I set the static IP address in the cfg file under IP-General Settings, and it's creates own files automatically with these settings.  I'm new to using Code Composure studio, but if I use F3 to open the declaration, I see this:

char *LocalIPAddr = "192.168.1.2";

I think this is a bug in the RTOS, it should be declared as

const char *LocalIPAddr = "192.168.1.2";

for reasons that I think will be obvious later.

Thodden Saporetti26 said:

char *string = "xyz";
char temp;

So if I do

temp = string[1]; 

temp is set to 'y', but the program crashes when I try the reverse, 

string[1] = temp;

Just declaring a char array works perfectly, as well as pointers set to a char array.  So normally I work around this by just creating a char array for LocalIPAddr, except I don't have control over how LocalIPAddr is declared, since it is created by the TI-RTOS side.  So ignoring any of my other glaring non-standard code issues we've been discussing, this is the root of where my issue is.  As I mentioned, I do have a viable work around, but I'd love to know what's causing this.

Yes, absolutely as would be expected*.

Think about what is happening.

1. String "xyz" will be stored in ROM (flash on the micro)
2. So string[1] refers to 'x' at a specific location in flash
3. So string[1]= temp attempts to overwrite 'x' in the flash
   1. Best case this does nothing
   2. Worst case the core faults due to a write to read only memory.

The C standard explicitly disallows your second sequence**.

Robert

* In memory protected environments. If the program is stored in volatile memory then you may end up with the constant string being modified***.

** The first C standard decided string constants would have a type of char *. This was done to keep backward compatibility by not requiring existing programs to update all string assignment to explicitly recognize const. I'm not sure that was the correct approach since it makes this situation much harder to diagnose, but given the volume of existing code it is understandable and probably is justified. Static analysis programs such as PC-Lint do recognize this shortcoming and can help diagnose this condition. Really string should be declared as const char *string (pointer to a constant char)

*** I have heard that early Fortran programs had this problem in spades. If you assigned a value to a number i.e. 42 = 3*2 then any other reference to 42 in the program would now have the value of 6

Thodden Saporetti26 said:

I'm sorry, when I said it should do nothing, I meant the value should literally be identical.  It shouldn't cause a crash because the value was set incorrectly.  It's being set to itself, so it shouldn't create a fatal error, yet it does.

Robert Adsett

Thodden Saporetti26

Just for a test, I also tried *LocalIPAddr = *LocalIPAddr;   which should do nothing,

Not true. Given a sufficient advanced compiler that might translate to a NOP but not necessarily. It would depend on what LocalIPAddr had been declared as.

Yeah, it can cause a crash. Just because the end result is apparently the same as the start doesn't mean that nothing happens.

Robert

Robert Adsett said:

I think this is a bug in the RTOS, it should be declared as

//nothing wrong here
char * strPtr = "Hey guys\n";

//compiler will give a warning
char c = 'c';
const char * constPtr = &c;

Thodden Saporetti26 said:

That's probably where my confusion came from.  Previously I've only used the C compiler for the PIC Microcontrollers

I have my doubts that any C compiler for the PICs is standard compliant, although it should be possible to make a compliant compiler. I suspect this is not the only place you have been led astray if you learned C by programming a PIC

Thodden Saporetti26 said:

C compiler treats char *pointer = "xyz" the same as char *string[3] = "xyz".  It may not be the standard, but it's what I'm familiar with.

They are perfectly allowed to generate code that treats them identically, you are not allowed to assume you can write to a string literal. In fact the standard forbids you to do such. It's also logically a bug to so, so it should not be something you are tempted to do.

Thodden Saporetti26 said:

let me know what direction I need to take.

You've already determined that (and awarded yourself a problem solved in the process ;). You need to store the IP address string so that it is not overwritten and update the address LocalIPAddr with the address of the string.

Robert

Peter Borenstein said:

Robert Adsett

I think this is a bug in the RTOS, it should be declared as

Its not. Its perfectly fine for a pointer to non constant char to actually store a const char address. The reverse is not true, and will give you a warning.

//nothing wrong here
char * strPtr = "Hey guys\n";

//compiler will give a warning
char c = 'c';
const char * constPtr = &c;

I do believe you have misunderstood why I think it's a bug

Robert Adsett said:

I think this is a bug in the RTOS, it should be declared as

const char *LocalIPAddr = "192.168.1.2";

for reasons that I think will be obvious later.

Note that my declaration is different from your second one. While the difference is important I don't think it affects your objection. Apparently my reasons weren't obvious, let me expand. Given the following declared in the RTOS (note this assigns a pointer to a literal to a pointer to a non-const char)

char *LocalIPAddr = "192.168.1.2";

There are, I think, two possibilities

1. LocalIPAddr is meant to be written (as the declaration implies) either by the RTOS itself or by the application.
   1. Then clearly the assignment of a string literal is a bug.
2. LocalIPAddr is not meant to be written by the RTOS
   1. In which case the string literal assignment is correct
   2. Also in which case the declaration should be as I've given for several reasons
      1. To signal that the RTOS will not write to it
      2. To prevent RTOS users from attempting to modify the contents pointed to by LocalIPAddr and successfully compiling. If that had been done this entire thread would not have been necessary.

In both cases this qualifies as a bug.

To address your point directly. Just as the fact that the compiler produces no warning or error for the following

Peter Borenstein said:

//nothing wrong here char * strPtr = "Hey guys\n";

does not mean it is bug free, it likewise does not follow that the warning for the following

Peter Borenstein said:

//compiler will give a warning char c = 'c'; const char * constPtr = &c;

necessarily indicates that there is a bug in the code (although it should be examined closely and commented properly). I can think of other valid uses for this construct, especially in combination with volatile. Yes, you should avoid compiler warnings but not at the expense of bugs. This particular warning behavior comes from the C standards committee's decision to make string literals behave as constants in order to follow both earlier practice and to follow the logical meaning of the construct, but to not make string literals have a constant type. If they had given string literals the constant type that they behave as if they have then you would have a different set of warnings. That different set of warnings would more closely match the actual behavior of the code.

Having gone through that I can make the lesser case for additional bugs

• The pointer should probably not be directly modifiable by the user in any case. That leaves it open for direct manipulation when it should probably be protected by the IP stack. Setting it through a function (say int setIPaddr(const char *addr); ) gives several advantages
  • You avoid the user have to deal with any assignment warnings generated by the compiler
  • You can check for address validity and return appropriately.
• The IP Address should probably not be stored as a string in any case. There's not particular advantage in doing so, using the above function you could convert the validated IP address to the integer it represents and store that for use in the stack without needing additional parsing by the stack.

These later two are more arguable but I do think there is a case to be made for both as design bugs and the design decisions involved did contribute to the OPs problems.

Robert