1

Recently I needed to compare two uint arrays (one volatile and other nonvolatile) and results were confusing, there got to be something I misunderstood about volatile arrays.

I need to read an array from an input device and write it to a local variable before comparing this array to a global volatile array. And if there is any difference i need to copy new one onto global one and publish new array to other platforms. Code is something as blow:

#define ARRAYLENGTH 30
volatile uint8 myArray[ARRAYLENGTH];

void myFunc(void){
    uint8 shadow_array[ARRAYLENGTH],change=0;
    readInput(shadow_array);
    for(int i=0;i<ARRAYLENGTH;i++){
        if(myArray[i] != shadow_array[i]){
            change = 1;
            myArray[i] = shadow_array[i];
            }
        }
    if(change){
        char arrayStr[ARRAYLENGTH*4];
        array2String(arrayStr,myArray);
        publish(arrayStr);
        }
    }

However, this didn't work and everytime myFunc runs, it comes out that a new message is published, mostly identical to the earlier message.

So I inserted a log line into code:

for(int i=0;i<ARRAYLENGTH;i++){
    if(myArray[i] != shadow_array[i]){
        change = 1;
        log("old:%d,new:%d\r\n",myArray[i],shadow_array[i]);
        myArray[i] = shadow_array[i];
        }
    }

Logs I got was as below:

old:0,new:0
old:8,new:8
old:87,new:87
...

Since solving bug was time critical I solved the issue as below:

char arrayStr[ARRAYLENGTH*4];
char arrayStr1[ARRAYLENGTH*4];
array2String(arrayStr,myArray);
array2String(arrayStr1,shadow_array);
if(strCompare(arrayStr,arrayStr1)){
    publish(arrayStr1);
    }
}

But, this approach is far from being efficient. If anyone have a reasonable explanation, i would like to hear.

Thank you.


[updated from comments:]

For the volatile part, global array has to be volatile, since other threads are accessing it.

alk
  • 69,737
  • 10
  • 105
  • 255
fatih
  • 1,010
  • 1
  • 8
  • 20
  • For the two 1st snippets: Are you sure you show us the exact code which produces the observed behaviour? – alk Feb 22 '15 at 16:53
  • What is the definition of readInput? Compiling with -Wall, were there any warnings? Why is the array volatile? Can it really change? – rici Feb 22 '15 at 16:59
  • As I said, its not the exact code, only equivalent. In original code, I am reading array from uart interface byte by byte so i didn't wanted to make snippets really long. But comparison steps are identical to the ones in the original code except for array names. – fatih Feb 22 '15 at 17:00
  • @rici readInput sends a message over uart and puts recevied bytes into shadow_array, besides it also checks checksum. But it works alright. For the volatile part, global array has to be volatile, since other threads are accessing it. And shadow_array must be non-volatile since readInput method does not accept volatile arrays as parameter. – fatih Feb 22 '15 at 17:03
  • Does your real code perhaps looks something like this: `if (!(myArray[i] = shadow_array[i])) { change = 1; myArray[i] = shadow_array[i]; } }` – alk Feb 23 '15 at 18:23
  • No it isn't. Here is the screenshot from code: http://prntscr.com/68hcrf – fatih Feb 25 '15 at 08:05

3 Answers3

3

If the global array is volatile, your tracing code could be inaccurate:

for(int i=0;i<ARRAYLENGTH;i++){
    if(myArray[i] != shadow_array[i]){
        change = 1;
        log("old:%d,new:%d\r\n",myArray[i],shadow_array[i]);
        myArray[i] = shadow_array[i];
        }
    }

The trouble is that the comparison line reads myArray[i] once, but the logging message reads it again, and since it is volatile, there's no guarantee that the two reads will give the same value. An accurate logging technique would be:

for (int i = 0; i < ARRAYLENGTH; i++)
{
    uintu_t value;
    if ((value = myArray[i]) != shadow_array[i])
    {
        change = 1;
        log("old:%d,new:%d\r\n", value, shadow_array[i]);
        myArray[i] = shadow_array[i];
    }
}

This copies the value used in the comparison and reports that. My gut feel is it is not going to show a difference, but in theory it could.

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
  • Actually, just to test if anything happens in between two reads, we inserted two variables in place you inserted 'value' but it is still pushing updates: here is the screenshot: http://prntscr.com/68hcrf – fatih Feb 22 '15 at 17:53
  • I'm not sure how volatile `myArray` really is; it would have to be rather peculiarly volatile for what I show as a possible cause to be the actual cause. It's peculiar because by the time you read it a second time, it has come up with the answer you wanted but didn't get the first time. You're messing with UARTs and the like, you say in comments, so it could be that your compiler isn't doing what you tell it to do. You may have to look at the generated assembler to tell that, though. Are there timing properties on memory where the array is stored? What platform are you using? – Jonathan Leffler Feb 22 '15 at 18:01
  • The code in the screen shot is `for (ii = 0; ii < ARRAYLENGTH; ii++) { uint8_t a_ = myArray[ii]; uint8_t b_ = shadow_array[ii]; if (a_ != b_) { change = 1; printf("old:%d,new:%d\r\n", a_, b_); myArray[ii] = shadow_array[ii]; }`, where the second close brace is not shown. The code which is shown clearly should not generate lines where old and new are the same. I don't have an explanation for why it is doing so, other than 'compiler bug', but it is pretty unlikely that you'd run into the problem here for the first time. – Jonathan Leffler Feb 22 '15 at 18:08
  • We too couldn't bring any reasonable explanation. Then I thought there might be something we don't know about volatility. Let's wait if anybody else has any explanation. If not, it is probably due to compiler or optimization level. Btw, braces are complete, i forgot to include it while taking screenshot. – fatih Feb 22 '15 at 19:54
1

global array has to be volatile, since other threads are accessing it

As you "nicely" observe declaring an array volatile is not the way to protect it against concurrent read/write access by different threads.

Use a mutex for this. For example by wrapping access to the "global array" into a function which locks and unlocks this mutex. Then only use this function to access the "global array".

References:


Also for printf()ing unsigned integers use the conversion specifier u not d.

Community
  • 1
  • 1
alk
  • 69,737
  • 10
  • 105
  • 255
  • I'm currently on vacation, I will try inserting a mutex for functions accessing global array when Im back. I hope it works. But, honestly Im not that optimistic as nothing explains why it happens even when i change the code to the following: http://prntscr.com/68hcrf – fatih Mar 01 '15 at 00:37
0

A variable (or Array) should be declared volatile when it may Change outside the current program execution flow. This may happen by concurrent threads or an ISR. If there is, however, only one who actually writes to it and all others are jsut Readers, then the actual writing code may treat it as being not volatile (even though there is no way to tell teh Compiler to do so).

So if the comparison function is the only Point in the Project where teh gloal Array is actually changed (updated) then there is no Problem with multiple reads. The code can be designed with the (external) knowledge that there will be no Change by an external source, despite of the volatile declaration.

The 'readers', however, do know that the variable (or the array content) may change and won't buffer it reads (e.g by storing the read vlaue in a register for further use), but still the array content may change while they are reading it and the whole information might be inconsistent. So the suggested use of a mutex is a good idea. It does not, however, help with the original Problem that the comparison Loop fails, even though nobody is messing with the array from outside.

Also, I wonder why myArray is declared volatile if it is only locally used and the publishing is done by sending out a pointer to ArrayStr (which is a pointer to a non-volatile char (array). There is no reason why myArray should be volatile. Actually, there is no reason for its existence at all: Just read in the data, create a temporary tring, and if it differes form the original one, replace the old string and publish it. Well, it's maybe less efficient to always build the string, but it makes the code much shorter and apparently works.

static char arrayStr[ARRAYLENGTH*4]={};
char tempStr[ARRAYLENGTH*4];
array2String(tempStr,shadow_array);
if(strCompare(arrayStr,tempStr)){
    strCopy(arrayStr, tempStr);
    publish(arrayStr);
    }
}