Why is dmpPacketBuffer never initialized? Memory corruption?

[gittymonkey]

Hello,

First let me say thanks to Jeff for writing this code. It saved me a ton of work and I really appreciate it.

I believe I discovered an issue with MPU6050 code. I found it when working with https://github.com/richardghirst/PiBits code, which is basically the same code, at least for this problem. So, I have not actually compile this code, what I found should apply here.

The problem is how the code is separated; MPU6050_6Axis_MotionApps20.h and MPU6050.cpp. For the example, MPU6050_DMP.6, MPU6050_6Axis_MotionApps20.h is included, which also includes MPU6050.h. MPU6050.o is built with only MPU6050.h, therefore the compile only knows of two variables, devAddr and buffer. When it compiles MPU6050_DMP.6.cpp, it sees two new variables(dmpPacketSize and dmpPacketBuffer), which now comes before the original two variables. This issue would be very obvious if the MPU6050_6Axis_MotionApps20 code made direct I2C calls, but it doesn't.

This now leads to dmpPacketBuffer and why it is not initialized. If you were to uncomment the code,

 /*if ((dmpPacketBuffer = (uint8_t *)malloc(42)) == 0) {
            return 3; // TODO: proper error code for no memory
 }*/

I2C operations will start to fail due to dmpPacketBuffer overwriting devAddr. This is due to MPU6050.o having devAddr as the first variable and MPU6050_DMP.6.0 having dmpPacketBuffer as the first variable, therefore they overlap in memory.

Here are the results of debug printf code,

size of devAddr 0x1 @ 1F348
size of buffer 0xe @ 1F349
From inside function/consturctor MPU6050 at address 0x1f348 of size 15
Dumping contents of memory
0x1f348 0xcc
0x1f349 0xa0
0x1f34a 0xa1
0x1f34b 0xa2
0x1f34c 0xa3
0x1f34d 0xa4
0x1f34e 0xa5
0x1f34f 0xa6
0x1f350 0xa7
0x1f351 0xa8
0x1f352 0xa9
0x1f353 0xaa
0x1f354 0xab
0x1f355 0xac
0x1f356 0xad

So things are as you would expect with only two vars.
This is probably overkill, but in each function, in MPU6050.cpp, I print the address of devAddr

In function setClockSource this is what I think devAddr's address is 0x1f348
In function setFullScaleGyroRange this is what I think devAddr's address is 0x1f348
In function setFullScaleAccelRange this is what I think devAddr's address is 0x1f348
In function setSleepEnabled this is what I think devAddr's address is 0x1f348
In function getDeviceID this is what I think devAddr's address is 0x1f348

Again, it is what you would expect, 0x1F348 is the addr of devAddr according to MPU6050.o

Next we get into the code from MPU6050_DMP.6.0

Initializing DMP...
size of dmpPacketBuffer 0x4 @ 1F348
size of dmpPacketSize 0x2 @ ABCD
size of devAddr 0x1 @ 1F34E
size of buffer 0xe @ 1F34F
From inside function/consturctor dmpInitialize at address 0x1f348 of size 24
Dumping contents of memory
As you can see, the memory is not like we would want
0x1f348 0x44 <--- was devAddr, now dmpPacketBuffer
0x1f349 0x33
0x1f34a 0x22
0x1f34b 0x11
0x1f34c 0xcd<--- now dmpPacketSize
0x1f34d 0xab<--- now dmpPacketSize
0x1f34e 0xdd<--- The new devAddr
0x1f34f 0xf0 <--- The new buffer
0x1f350 0xf1
0x1f351 0xf2
0x1f352 0xf3
0x1f353 0xf4
0x1f354 0xf5
0x1f355 0xf6
0x1f356 0xf7
0x1f357 0xf8
0x1f358 0xf9
0x1f359 0xfa
0x1f35a 0xfb
0x1f35b 0xfc
0x1f35c 0xfd
0x1f35d (nil)
0x1f35e (nil)
0x1f35f (nil)

As you can see, things are not looking good, devAddr is now pointing to 0x1F34E and dmpPacketBuffer is pointing to the original devAddr, not good!

The code that gets called from MPU6050.o is still pointing to 0x1f348(see below), which is also mapped to dmpPacketBuffer. But we are in luck, devAddr get set in the constructor, so it is in the correct place for these functions to do the correct thing. Now if we uncomment the initialization of dmpPacketBuffer, we will see that it overwrites the original devAddr (according to MPU6050.o).

In function reset this is what I think devAddr's address is 0x1f348
In function setSleepEnabled this is what I think devAddr's address is 0x1f348
In function setMemoryBank this is what I think devAddr's address is 0x1f348
In function setMemoryStartAddress this is what I think devAddr's address is 0x1f348
...snip...

Why use this "header file" paradigm? This would work fine if you globally define MPU6050_INCLUDE_DMP_MOTIONAPPS20 or MPU6050_INCLUDE_DMP_MOTIONAPPS41 and then fixed MPU6050.cpp to include the correct header files according to these defines. Not sure if I like that idea.

What about inheritance? This seems like a good place for DMP_MOTIONAPPS20 to be a derived class of MPU6050, since it is extending it's functionality.

Sorry for the long post.

Tony

[jensh]

I run into the same issue while trying to understand why dmpPacketSize changes when calling getFIFOCount(). In this case MPU6050::buffer[14] overlap with MPU6050::dmpPacketSize. The later is not known in MPU6050.cpp, cause MPU6050_INCLUDE_DMP_MOTIONAPPS20 is not defined there.

[jamagby]

I would suggest moving the declaration of the fields "dmpPacketBuffer" and "dmpPacketSize" to the private section so the layout of the class doesn't depend on the directives and correctly allocating "dmpPacketBuffer".