Global Var Set (or not set) w/o Me Knowing

DavidZemon · 2013-10-15 08:18

I have a multi-threaded app running with two devices using my SPI routine as written in PropWare. Something seems to be wrong (maybe I'm just missing a keyword?) because despite having never yet called 'SPIStart(...)', my global variable g_spiCog is not equal to -1. At the top of spi.c is a line:

static int8_t g_spiCog = -1;

Here is a link directly to spi.c. I've searched the file; there are exactly 5 occurrences of the variable:

Line 28: Initialization
Line 60: Assignment by cognew(...)
Line 88: Stopping the cog
Line 89: Assigning -1 after stopping the cog
Line 96: Checking value in comparison for SPIIsRunning()

I use g_spiCog to determine whether or not the assembly cog has already started so, if it is randomly changing from its default value of -1, then my assembly cog never starts and the whole module fails.

I can only think of two possible things: 1) the variable is never initialized, and 2) Some pointer has gone haywire and is overwriting g_spiCog. But I use pointers sparingly and carefully (and never malloc) so I find that hard to believe.

If more context is needed, let me know. I can upload the full project to github or just snippets.

Thanks,
David

Invent-O-Doc · 2013-10-15 14:41

I didn't review your code, but try "static volatile" in your approach. static will let you share between objects in C++ mode, but volatile is required if you want to share between COGs, according to my limited knowledge.

DavidZemon · 2013-10-15 15:18

Thanks. I've tried volatile and it doesn't help. This variable and function call (SPIIsRunning) is not being used in multiple threads/cogs (er.... Shouldn't be)

David Betz · 2013-10-15 17:29

What's in spi_as_cog?

Edit: Never mind. I see you aren't even calling the start function. I guess seeing more of the code might help. Can you post a zip file?

DavidZemon · 2013-10-15 17:45

I can post the full code tomorrow morning. I didn't think to grab it before leaving

In the mean time, I'll post a short overview of the program:
There are three cogs

Cog 1 - Segway.c, Motors.c
Runs main(), PID loop, and outputs PWM signal to two GPIOs

Cog 2 - Segway_backend.c, spi.c, mcp300x.c, l3g.c, PropWare.c... I think that's it
controls SPI assembly cog, reads values from two devices via SPI, runs a weighted, rolling average filter on the inputs

Cog 3 - spi_as.S
GAS cog running SPI routines

All of the SPI code (along with mcp300x.c, l3g.c, PropWare.c are in PropWare here - same git repo as spi.c posted above)

David Betz · 2013-10-15 17:53

SwimDude0614 wrote: »

I can post the full code tomorrow morning. I didn't think to grab it before leaving

In the mean time, I'll post a short overview of the program:
There are three cogs

Cog 1 - Segway.c, Motors.c
Runs main(), PID loop, and outputs PWM signal to two GPIOs

Cog 2 - Segway_backend.c, spi.c, mcp300x.c, l3g.c, PropWare.c... I think that's it
controls SPI assembly cog, reads values from two devices via SPI, runs a weighted, rolling average filter on the inputs

Cog 3 - spi_as.S
GAS cog running SPI routines

All of the SPI code (along with mcp300x.c, l3g.c, PropWare.c are in PropWare here - same git repo as spi.c posted above)

How do you know that this variable is getting trashed since it's a static variable in the spi.c module? Also, I don't think it does any good to declare a function as inline in the header file unless the body of the function is also in the header file.

DavidZemon · 2013-10-16 08:12

Attached is a zip file with my debug code. All of the files not preceeded with "Segway" are from PropWare and are nearly identical to the git repo, but there are currently some extra print statements (namely dealing with the LCD since printf is all kinds of whack for some reason).

David Betz wrote:

How do you know that this variable is getting trashed since it's a static variable in the spi.c module?

The first step in Segway_backend.c is calling the setup() function. The first step of setup() is to initialize the L3G module with L3GStart(). First step of L3G is to check if SPI has already been initialized and, if not, initialize it. Therefore, if we ignore the long string of function calls, the first thing that this cog (Segway_backend.c) does is check if SPI is running, which can be done by calling "SPIIsRunning". Source code for which is very simple:

inline int8_t SPIIsRunning (void) {
    return !(-1 == g_spiCog);
}

Since printf/__simple_printf is acting very bizarre, I'm printing values to my 16x2 LCD such as:

HD44780_putchar('0' + SPIIsRunning());

It should return "false" and print out as '0'... but it doesn't. '1' every time.

To be absolutely sure that something weird isn't calling SPIStart() by accident, I've thrown a simple snippet at the top of the SPI assembly cog:

mov temp, cnt
                        add temp, quarterSecond
HORRIBLE_LOOP           xor outa, outa
                        waitcnt temp, quarterSecond
                        jmp #HORRIBLE_LOOP

With the lights not blinking, I know SPIStart() has not [successfully] run.

David Betz wrote:

Also, I don't think it does any good to declare a function as inline in the header file unless the body of the function is also in the header file.

Thanks. I'm pretty fuzzy on the various keywords. I know what they do but am not 100% sure where they should and should not be placed.

David Betz · 2013-10-16 08:30

I think you forgot to attach the zip file. :-)

DavidZemon · 2013-10-16 08:35

Lol yea... I just edited that.

David Betz · 2013-10-16 08:37

SwimDude0614 wrote: »

Lol yea... I just edited that.

Thanks! I'll look at it later after work.

DavidZemon · 2013-10-18 07:39

Any chance you found an issue or bug in the code?

David Betz · 2013-10-18 08:05

SwimDude0614 wrote: »

Any chance you found an issue or bug in the code?

Haven't had time yet I'm afraid. Sorry!!

jac_goudsmit · 2013-10-18 08:14

I thought at first that you had defined g_SPIcog as an unsigned byte and tried to compare it to -1 which will yield the wrong result (if you set a uint8_t to -1, it really gets set to 255 and when you compare that to -1, the 255 byte gets sign-extended to an unsigned integer with value 255 so it will be unequal to -1).

But I was wrong: you have the variable declared as a SIGNED 8-bit integer (int8_t -- really a char) of value -1 which gets sign-extended to an integer with value -1 so the comparison should be correct and should have made the function return 0 (false, not running).

I don't have SimpleIDE on the system I'm working on at this moment but you might want to try casting the -1 in the comparison to the same type as the variable just to make sure. Try:

return (int8_t)-1 != g_SPIcog;

You may also want to make the g_SPIcog variable a volatile but I don't think that should make a difference: volatile only prevents consecutive fetches of the same variable in the same function to be optimized away and that doesn't happen at all in your code as far as I can see.

If the above modification makes it work, we may need to take a good look at the assembler output of the original version, I wonder if there's something wrong with the sign-extension code. I can't really think of anything else.

A simple test to see if the sign-extension code in the compiler works correctly would be:

#include <propeller.h>
#include <stdio,h>

static volatile int8_t x = -1;

void main(void)
{
  printf("x is %d\n", x);

  printf("x is %s to negative one\n", (x == -1 ? "equal" : "unequal"));
}

===Jac

David Betz · 2013-10-18 08:24

Is it possible that THREAD_STACK_SIZE is too small and that your stack is overflowing? It looks like you've got a bunch of local arrays declared in your segway_backen function. Do they by any chance add up to more than 128 longs?

DavidZemon · 2013-10-18 08:30

Thanks for the tip jac. I tried adding the explicit cast to int8_t inside SPIIsRunning() and it didn't help. Volatile is changing something... can't tell what though... I'll come back when I figure out what's going on with that.

I tried increasing the stack size to 1024 and it's making no difference. I'll leave it there for now to be sure. And to answer your question David, not quite. There are four arrays, each are currently 32 vars long. Two of them are 2-byte vars and two are 1-byte. 32*(2*2 + 2*1) / 4 = 48.

DavidZemon · 2013-10-21 09:28

Is sprintf supposed to work correctly/normally? Doing a google for "sprintf propeller" came up completely blank except for the source code in propgcc

jazzed · 2013-10-21 09:38

SwimDude0614 wrote: »

Is sprintf supposed to work correctly/normally? Doing a google for "sprintf propeller" came up completely blank except for the source code in propgcc

Yes, it works.

Function sprintf is part of the standard C library. This is a good reference: http://publications.gbdirect.co.uk/c_book/chapter9/formatted_io.html

Of course it will grow just like standard printf. Use the alternative Simple Library print, scan, and friends functions for manageable code size.

DavidZemon · 2013-10-21 10:47

Okay... sprintf is broken in my program as well (works fine in a sample program on the prop though

). Good news is, I at least have a working printf function built into the HD44780 library now and a 24x2 instead of 16x2 LCD so I can get some real debugging done. sprintf would have made that a lot easier, but I clearly have a fairly large problem in the code that's messing up a lot of things.

or could it be in the makefile???

jazzed · 2013-10-21 11:37

SwimDude0614 wrote: »

Okay... sprintf is broken in my program as well (works fine in a sample program on the prop though ).

Example please?

What Propeller-GCC distribution are you running? I.E. $ propeller-elf-gcc -v

DavidZemon · 2013-10-21 11:54

Fairly sure it's the version that ships with SimpleIDE 0.9.40 for Linux. I'm away from my computer for another hour - I'll post back with the full version as soon as I can though.

sprintf yields a bunch of jargon, as if it's dropping a null-terminator or something. I've made the buffer very long but it doesn't matter. Again, I'll post full details when I get back.

David Betz · 2013-10-21 12:10

I'm sorry I haven't had time to look in detail through your code. Is there any chance you could come up with a smaller example that illustrates this problem?

Thanks,
David

DavidZemon · 2013-10-21 12:12

I've actually been working on that all morning, trying to narrow down on the problem. I brought out the backend files into a separate project and am now getting a *different* error (but at least g_spi_cog is -1 where I expect it to be!). I'll post back again when determine where this latest problem is.

David Betz · 2013-10-21 12:23

SwimDude0614 wrote: »

I've actually been working on that all morning, trying to narrow down on the problem. I brought out the backend files into a separate project and am now getting a *different* error (but at least g_spi_cog is -1 where I expect it to be!). I'll post back again when determine where this latest problem is.

The fact that the problem moves from trashing g_spi_cog to causing some other trouble certainly suggests that there is some code somewhere that is randomly writing memory. A store through an uninitialized pointer is the most likely cause of this.

DavidZemon · 2013-10-21 13:07

PropGCC Version:

Using built-in specs.COLLECT_GCC=propeller-elf-gcc
COLLECT_LTO_WRAPPER=/opt/parallax/libexec/gcc/propeller-elf/4.6.1/lto-wrapper
Target: propeller-elf
Configured with: ../../propgcc/gcc/configure --target=propeller-elf --prefix=/opt/parallax --disable-nls --disable-libssp --disable-lto --disable-shared --with-pkgversion=propellergcc_v1_0_0_2090 --with-bugurl=http://code.google.com/p/propgcc/issues
Thread model: single
gcc version 4.6.1 (propellergcc_v1_0_0_2090)

DavidZemon · 2013-10-21 13:49

Found the "error" in the new project. It was a debug infinite loop I'd forgotten about. With that now out of the way, it works correctly.

The error has something to do with multi-core. I'll continue trying to narrow it down some more.

David Betz · 2013-10-21 14:10

SwimDude0614 wrote: »

Found the "error" in the new project. It was a debug infinite loop I'd forgotten about. With that now out of the way, it works correctly.

The error has something to do with multi-core. I'll continue trying to narrow it down some more.

Have you tried commenting out the starting of each COG to see if you can isolate which one is causing the trouble? How many COGs are you using besides the main one?

David Betz · 2013-10-21 14:15

I think I see your problem. Stacks grow from the top of memory down in PropGCC so I think you need to change your code as follows:

From this:

	// Initialize lean angle read cog
	*backendCogID = _start_cog_thread(g_cog_stack, segway_backend, NULL,
			&g_thread_data);

To this:

	// Initialize lean angle read cog
	*backendCogID = _start_cog_thread(g_cog_stack + sizeof(g_cog_stack) / sizeof(uint32_t), segway_backend, NULL,
			&g_thread_data);

DavidZemon · 2013-10-21 14:20

Mr. Betz sir... thank you. Been stuck on this for a week and a half, and there it was. I'm not sure how I missed this... I'll go over the tutorials and samples that I used in the past to see what happened.

David Betz · 2013-10-21 14:24

SwimDude0614 wrote: »

Mr. Betz sir... thank you. Been stuck on this for a week and a half, and there it was. I'm not sure how I missed this... I'll go over the tutorials and samples that I used in the past to see what happened.

I'm glad that solved your problem. As you can see, I was wrong. This didn't have anything to do with storing through an uninitialized pointer. :-)

kuroneko · 2013-10-21 20:25

David Betz wrote: »

I'm glad that solved your problem. As you can see, I was wrong. This didn't have anything to do with storing through an uninitialized pointer. :-)

To be fair, the prototype for _start_cog_thread actually says stacktop. That said, cogstart requires you to pass the bottom address (and size) which makes the whole stack business slightly confusing. Not sure how this could be addressed given the willingness of the average programmer to read documentation. Guard pages are probably too expensive in this environment.

David Betz · 2013-10-21 20:32

kuroneko wrote: »

To be fair, the prototype for _start_cog_thread actually says stacktop. That said, cogstart requires you to pass the bottom address (and size) which makes the whole stack business slightly confusing. Not sure how this could be addressed given the willingness of the average programmer to read documentation. Guard pages are probably too expensive in this environment.

I guess cogstart is my fault. I thought that passing the address and size of the stack would likely result in fewer errors than passing the top of the stack but I guess that didn't really work out. The scheme that _start_cog_thread has the advantage that one fewer arguments have to be passed to the function but it is awkward because you have to do the funky "array + sizeof(array)" thing to pass the correct address. Not sure which is better.

Global Var Set (or not set) w/o Me Knowing

Comments