 |
 |
| View previous topic :: View next topic |
| Author |
Message |
allenhuffman
Joined: 17 Jun 2019
Posts: 537
Location: Des Moines, Iowa, USA
|
| I2C global int8 can be altered by user code in ISR |
 Posted: Thu Aug 29, 2019 3:06 pm |
 |
|
"...or any int8 used by any library code that is accessed in an ISR."
Just a fun warning about something I ran into at work recently. We had an issue where the I2C communications would hang, and finally tracked it down to the use of global int8s in the code.
If you have a global "int8 value;" that gets set to zero, it might generate code like this:
| Code: | .............................. value = 0;
0022C EF6800 CLR.B 800 : [800] = 0 |
But if you have enough globals being used (more than 6K on my PIC24), it might generate multiple "load, modify, store" instructions for the same C code:
| Code: | .............................. value = 0;
0022E 817880 MOV 2F10,W0 : W0 = [2F10]
00230 B3C000 MOV.B #0,W0L : W0L = 0
00232 897880 MOV W0,2F10 : [2F10] = W0 |
In our situation, there was an int8 called I2C_STATUS (or similar) being used by the CCS I2C code, and it "just happened" to end up sharing a 16-bit word with one of our int8 variables.
Our code "just happened" to be running that multiple step "load, modify, restore" when an IRQ happened, then the ISR would modify the value and store it back, then our code would resume and store back what we had, overwriting what the ISR did to the previous value.
Once we saw this, it was an "oh, sure, of course that can happen" moment. It's very similar to how you always want to protect variable access with semaphores when they could be accessed from other bets of code via threads, ISRs, etc.
In this case, though, the int8 is part of a library, so any int8 we used could have ended up sharing that word and potentially causing an unexpected value change.
So keep in mind, avoid using int8s in the ISR "just in case" you have an int8 in the rest of your code that might cause this to happen. You can't tell where your value is unless you look at the symbol table file.
We can change all our values to int16+ and prevent any of our code from doing something like this moving forward.
Fun stuff!
Side note...
This is just like the classic ISR issue of:
| Code: | int g_dataReady = 0;
isr_data()
{
g_dataReady++;
// store data in a buffer or whatever...
}
something()
{
while (processing)
{
if (g_dataReady > 0)
{
processData(); // A
g_dataReady = 0; // B
}
}
} |
For those without the same background, imagine data comes in and the while() code goes to process it. After done processing, it is about to set that counter back to 0. An ISR comes in, and dataReady increments. Then the main code continues, and proceeds to set it to 0.
Now that data seems "lost".
Traditionally, you'd protect accessing the g_dataReady valye with a semaphore or whatever mechanism the OS or environment supports. Pretty basic stuff, but in the case of code generated by the compiler, there wouldn't be a way to know that's happening, or get into that code to lock around the 16-bit value.
Fun stuff!
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ? |
|
 |
allenhuffman
Joined: 17 Jun 2019
Posts: 537
Location: Des Moines, Iowa, USA
|
|
| Posted: Thu Aug 29, 2019 4:03 pm |
|
|
Sharing some of my notes. In my environment,, globals start at the 2K mark (0x800). After 8K (0x2000) it doesn't look like there is a MOV op code that works with memory, so the compiler loads a value into a register, modifies the register, then writes the register back.
| Code: | .............................. // Offset -> Address
.............................. memory[0] = 0; // 0x0 -> 0x0800
00228 EF6800 CLR.B 800 : [800] = 0
..............................
.............................. memory [6143] = 0; // 0x17ff -> 0x1fff
0022A EF7FFF CLR.B 1FFF : [1FFF] = 0
..............................
.............................. memory [6144] = 0; // 0x1800 -> 0x2000
0022C 810000 MOV 2000,W0 : W0 = [2000]
0022E B3C000 MOV.B #0,W0L : W0L = 0
00230 890000 MOV W0,2000 : [2000] = W0
..............................
..............................
.............................. memory[0] = 1; // 0x0 -> 0x0800
00232 B3C010 MOV.B #1,W0L : W0L = 1
00234 B7E800 MOV.B W0L,800 : [800] = W0L
..............................
.............................. memory [6143] = 1; // 0x17ff -> 0x1fff
00236 B3C010 MOV.B #1,W0L : W0L = 1
00238 B7FFFF MOV.B W0L,1FFF : [1FFF] = W0L
..............................
.............................. memory [6144] = 1; // 0x1800 -> 0x2000
0023A 810000 MOV 2000,W0 : W0 = [2000]
0023C B3C010 MOV.B #1,W0L : W0L = 1
0023E 890000 MOV W0,2000 : [2000] = W0 |
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ? |
|
|
jeremiah
Joined: 20 Jul 2010
Posts: 1315
|
|
| Posted: Thu Aug 29, 2019 8:23 pm |
|
|
| Make sure you report it to CCS support. I just submitted a similar type bug (though caused by something else) and they fixed it for the next upcoming release. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19215
|
|
| Posted: Fri Aug 30, 2019 12:05 am |
|
|
As a comment, it used to be standard practice on quite a few compilers
to always word align any 8bit variables accessed in IRQ's. It sounds as if the
same really needs to be done with CCS.
As a comment to the original poster, he says 'int16+'. Remember that anything
over an int16, itself needs protection when accessed inside an IRQ, since you then automatically have the potential for two accesses to be involved....
Only the 'native' processor size (so int8 on PIC16/18, and int16 on PIC24/33)
has inherent protection against this problem. |
|
|
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
| Re: I2C global int8 can be altered by user code in ISR |
| Posted: Fri Aug 30, 2019 2:44 am |
|
|
| allenhuffman wrote: |
But if you have enough globals being used (more than 6K on my PIC24). |
6000 individual named global variables ? Why ? I was taught to use only
a few, and only if necessary. Maybe you mean a global buffer. |
|
|
temtronic
Joined: 01 Jul 2010
Posts: 9097
Location: Greensville,Ontario
|
|
| Posted: Fri Aug 30, 2019 4:43 am |
|
|
I won't live long enough to type up 6000 individually named variables, sigh...
That's a LOT of typing ( and typos for me.......)
I'm thinking some kind of buffer as well....
Jay |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 537
Location: Des Moines, Iowa, USA
|
| Re: I2C global int8 can be altered by user code in ISR |
| Posted: Fri Aug 30, 2019 7:39 am |
|
|
| PCM programmer wrote: | | allenhuffman wrote: |
But if you have enough globals being used (more than 6K on my PIC24). |
6000 individual named global variables ? Why ? I was taught to use only
a few, and only if necessary. Maybe you mean a global buffer. |
There are, indeed, a lot of variables (including buffers).
| Code: | ROM used: 38608 bytes (22%)
Largest free fragment is 44544
RAM used: 16969 (52%) at main() level
17195 (53%) worst case |
Why coders do what they do remains one of the mysteries of the universe. I am more on the "clean code" side and try to localize variables and functions only where they need to be, to avoid polluting the rest of the project. Oh, the stories I could tell from various jobs...
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ?
Last edited by allenhuffman on Fri Aug 30, 2019 7:59 am; edited 2 times in total |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 537
Location: Des Moines, Iowa, USA
|
|
| Posted: Fri Aug 30, 2019 7:39 am |
|
|
| jeremiah wrote: | | Make sure you report it to CCS support. I just submitted a similar type bug (though caused by something else) and they fixed it for the next upcoming release. |
I did not consider it a bug, per se, but I see another comment here about compilers protecting variables used in ISRs which seems like a smart thing to do. I'll write something up for them.
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ? |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 537
Location: Des Moines, Iowa, USA
|
|
| Posted: Fri Aug 30, 2019 7:41 am |
|
|
| Ttelmah wrote: | As a comment, it used to be standard practice on quite a few compilers
to always word align any 8bit variables accessed in IRQ's. It sounds as if the
same really needs to be done with CCS.
As a comment to the original poster, he says 'int16+'. Remember that anything
over an int16, itself needs protection when accessed inside an IRQ, since you then automatically have the potential for two accesses to be involved....
Only the 'native' processor size (so int8 on PIC16/18, and int16 on PIC24/33)
has inherent protection against this problem. |
Good point - I hadn't thought about the implication of using 32-bit/64-bit variables that are "faked" on 16-bit architectures.
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ? |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 537
Location: Des Moines, Iowa, USA
|
|
| Posted: Fri Aug 30, 2019 9:42 am |
|
|
I need to rename this thread or start a new one. It gets better.
We have multiple boards, and are finding similar issues in other boards. The next one was us having this in our code:
rtc_time_t realTimeClock;
That is a 9-byte structure (9 int8s).
In memory, it happened to be lined up right before @I2C_STATUS (internal variable used by i2c library code).
We then saw corruption of the last byte of the realTimeClock structure.
We can control where we declare "realTimeClock", or add a bogus int8 after it in our program (since the compiler doesn't seem to be optimizing unused things away with our settings).
But … Whac-A-Mole! 
https://en.wikipedia.org/wiki/Whac-A-Mole
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ? |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 537
Location: Des Moines, Iowa, USA
|
|
| Posted: Fri Aug 30, 2019 9:50 am |
|
|
More compiler Whac-a-Mole... I have a tiny test program with these globals:
| Code: | int8 value1;
rtc_time_t realTimeClock;
int8 value2;
int8 value3;
int8 value4; |
The compiler is smart enough to use available int8 bytes, so the actual layout of variables in memory is:
| Code: | 800 value1
801 value2
802-80A realTimeClock
80B value3
80C value4 |
It places value2 after value1 since there is a byte free. Thus, that @I2C_STATE global variable can be placed in a 16-bit value that is shared with another int8. Nothing we can do about that with library code, unless there is a pragma or something that helps.
There is the __attribute_x thing which can force variables to be aligned. I chose a 2-byte boundry (even bytes):
| Code: | int8 value1 __attribute__((aligned(0x2)));
rtc_time_t realTimeClock;
int8 value2 __attribute__((aligned(0x2)));
int8 value3 __attribute__((aligned(0x2)));
int8 value4 __attribute__((aligned(0x2))); |
Now we get everything starting on an even byte boundry, in the order I declared them:
| Code: | 800 value1
802-80A realTimeClock
80C value2
80E value3
810 value4 |
That's kind of silly, since if we control the code, why not switch to just using native ints? Then we only have to worry about two int8s in library code conflicting -- not sure we have any control over that.
Fun with memory day continues...
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ? |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 537
Location: Des Moines, Iowa, USA
|
|
| Posted: Fri Aug 30, 2019 9:54 am |
|
|
For fun … a quick #define macro that will add or not add the attribute as needed. Make the define there, but empty, and you get default behavior. Define the macro to the attribute, and you get alignment:
| Code: | //#define ALIGN __attribute__((aligned(0x2)))
#define ALIGN
int8 value1 ALIGN;
rtc_time_t realTimeClock;
int8 value2 ALIGN;
int8 value3 ALIGN;
int8 value4 ALIGN; |
Now I can easily switch it on and off to look at what the compiler is doing. Fun stuff.
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ? |
|
|
allenhuffman
Joined: 17 Jun 2019
Posts: 537
Location: Des Moines, Iowa, USA
|
|
| Posted: Fri Aug 30, 2019 10:02 am |
|
|
_attribute_x help file entry:
| Quote: | By default each element in a struct or union are padded to be evenly spaced by the size of 'int'. This is to prevent an address fault when accessing an element of struct. See the following example:
struct
{
int8 a;
int16 b;
} test;
On architectures where 'int' is 16bit (such as dsPIC or PIC24 microcontrollers), 'test' would take 4 bytes even though it is comprised of3 bytes. ... |
While this example is true, the wording may be a bit misleading. It will not pad int8s to be on even byte alignments unless an int8 is followed by a non-int8 in the structure.
That's the behavior I would expect. When I read the phrase "By default each element in a struct or union are padded to be evenly spaced by the size of 'int'." I wondered if they were padding int8s.
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, BASIC Stamp and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ? |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19215
|
|
| Posted: Fri Aug 30, 2019 10:43 am |
|
|
No, they are talking about all elements. Remember there are lots of
things that can use an odd number of bytes. A structure, an int8, an
int1.
Their example there is for the situation where you want to pack things
for something like a structure that is then passed to another device.
The opposite to what is involved here.
The advantage of word aligning bytes, rather than using an int16, is that
some things can be slightly faster done on a byte, especially if this is then
going to be used for something like I/O. |
|
|
|
|
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
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
