Arduino Forum

Using Arduino => Programming Questions => Topic started by: Jeff Rowberg on Nov 02, 2014, 04:30 pm

Title: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 02, 2014, 04:30 pm
I am running into a problem which seems to have to do with the code execution pointer, and the behavior makes zero sense at all. There is a conditional statement which doesn't match, and since the code it controls is only one line, I omitted the surrounding { } braces for brevity. This is a pattern that I have reused many other times all throughout this fairly complex project without any observable bad consequences.

However, when I let the code run this way, execution apparently stops at that line. Here is the code block:

Code: [Select]

/**
 * @brief iWRAP "NAME" event handler
 * @param[in] mac MAC address of remote device
 * @param[in] friendly_name Null-ternimated device name string
 */
void my_iwrap_evt_name(const iwrap_address_t *mac, const char *friendly_name) {
    uint8_t name_len = 0;
    if (friendly_name) name_len = strlen(friendly_name);

    uint8_t payload[13 + name_len];
    payload[0] = mac -> address[5];
    payload[1] = mac -> address[4];
    payload[2] = mac -> address[3];
    payload[3] = mac -> address[2];
    payload[4] = mac -> address[1];
    payload[5] = mac -> address[0];
    payload[6] = 0;
    payload[7] = 0;
    payload[8] = 0;
    payload[9] = 0; // RSSI
    payload[10] = 4; // inquiry status
    payload[11] = find_pairing_from_mac(mac);
    if (name_len) memcpy(payload + 13, friendly_name, name_len);

    // send kg_evt_bluetooth_inquiry_response(...) event
    skipPacket = 0;
    if (kg_evt_bluetooth_inquiry_response) skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);
    if (!skipPacket) send_keyglove_packet(KG_PACKET_TYPE_EVENT, 13 + name_len, KG_PACKET_CLASS_BLUETOOTH, KG_PACKET_ID_EVT_BLUETOOTH_INQUIRY_RESPONSE, payload);
}


The offending line of code is this:

Code: [Select]

    if (kg_evt_bluetooth_inquiry_response) skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);


Note that "kg_evt_bluetooth_inquiry_response" function pointer which is currently NULL, so this condition doesn't match. Now, if I modify this one line of code to include braces:

Code: [Select]

    if (kg_evt_bluetooth_inquiry_response) {
        skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);
    }


Suddenly, everything works. There is NO reason I can think of why this should be occurring. Is there some weird Arduino IDE Java -> C++ conversion that is messing with the code blocks before compiling? Can I check this somehow?

I have also noticed that if I add a log output command (not shown here) earlier inside the function, in a way that is logically unrelated (and separated by many lines of code) from the condition shown here, the problem also goes away even if I don't add braces. I suspect some weird memory leak in my code or a buffer overflow or something, but I have no way of doing low-level debugging with breakpoints, watches, etc.

I am using Arduino v1.0.6 with a Teensy++ 2.0 (AT90USB1286) and Teensyduino v1.20 (latest as of this moment).
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: AWOL on Nov 02, 2014, 04:32 pm
Condition or assignment?
Did you mean ==?
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: econjack on Nov 02, 2014, 05:09 pm
In your statement:

Code: [Select]

 if (kg_evt_bluetooth_inquiry_response) {
        skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);
    }


what do you think the function call name in the sub-expression:

Code: [Select]

  if(kg_evt_bluetooth_inquiry_response)


does?
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 02, 2014, 07:45 pm
Condition or assignment?
Did you mean ==?
The "skipPacket" variable should be assigned if the condition matches and the function is executed, but in this case there should be only the test for a non-null function pointer, which should fail since the function pointer is null.


In your statement:

Code: [Select]

 if (kg_evt_bluetooth_inquiry_response) {
        skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);
    }


what do you think the function call name in the sub-expression:

Code: [Select]

  if(kg_evt_bluetooth_inquiry_response)


does?
The condition is meant to check whether the function pointer has been assigned to an actual function or not. The function pointer is declared like this, but not initialized:

Code: [Select]

uint8_t (*kg_evt_bluetooth_inquiry_response)(uint8_t *address, uint8_t *cod, int8_t rssi, uint8_t status, uint8_t index, uint8_t name_len, uint8_t *name_data);


In this circumstance, the non-dereferenced "kg_evt_bluetooth_inquiry_response" pointer is null, so "if (null)" evaluates to false (or it should). This kind of conditional check for assigned vs. unassigned function pointers as optionally defined callbacks is present all over the place in the code, and they appear to work fine--with this one exception.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: AWOL on Nov 02, 2014, 07:47 pm
Quote
The "skipPacket" variable should be assigned if the condition matches
Which condition?

Code: [Select]
if (kg_evt_bluetooth_inquiry_response) skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);
Here, "kg_evt_bluetooth_inquiry_response" is called unconditionally, the return value assigned to "skipPacket" and the result of that assignment tested.
However the semicolon at the end means no decision is taken as a result of that test.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 02, 2014, 07:58 pm
Which condition?
This one:

Code: [Select]

if (kg_evt_bluetooth_inquiry_response) skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);


Translated into English:

"If the kg_evt_bluetooth_inquiry_response function pointer is non-null--in other words, if it has been set to point to a real function instead--then run the kg_evt_bluetooth_inquiry_response function with the supplied arguments, and store the return value in skipPacket."

The skipPacket variable is declared and initialized to 0 one line above this, so it starts with a known value. But it should not be modified regardless, since the "if (kg_evt_bluetooth_inquiry_response)" condition should evaluate as false, and the line of code the subsequently would call that function should not be executed.

My understanding of function pointers (and the rest of the code and other libraries (https://github.com/jrowberg/bglib/blob/master/Arduino/BGLib.cpp#L572) I have written) suggests that if you have a function pointer declared like this:

Code: [Select]

uint8_t (*my_function_pointer)(uint8_t arg1, uint8_t arg2);


...and then you use only the pointer variable name without a parameter list like this:

Code: [Select]

if (my_function_pointer != null) { ... }


...it tests the pointer's assigned memory address only, and does not actually call the function (which is what I want here). On the other hand, if you include the parameter list:

Code: [Select]

if (my_function_pointer(10, 20) != null) { ... }


...then it will dereference the pointer and execute the real function that it is pointing to, with the supplied arguments, and then the conditional statement will evaluate the return value rather than the function pointer's memory address.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: AWOL on Nov 02, 2014, 08:58 pm
Quote
Translated into English:

"If the kg_evt_bluetooth_inquiry_response function pointer is non-null--in other words, if it has been set to point to a real function instead--then run the kg_evt_bluetooth_inquiry_response function with the supplied arguments, and store the return value in skipPacket."
That's not how to translate that code into English.
You're calling the function whether or not the pointer to it is NULL.

Code: [Select]
if (fn && (result = fn(paramList)))
{
  doThis();
}

Tests the function pointer and if it is not NULL, calls the function and assigns the value returned by the function to the variable "result".
If the value of "result" is non-zero, the function "doThis" will be executed.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Vaclav on Nov 02, 2014, 09:14 pm
Until "notify me" is fixed.
I know this should not matter, but have you actually tried to compare to null?
I am not questioning you, but does the process stops at the first if(...) or executes the next if(!...)?
Just asking.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: PaulS on Nov 02, 2014, 09:16 pm
I think it's time for you to post ALL of your code, so that we can try to reproduce the problem.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: KenF on Nov 02, 2014, 10:22 pm
I'm wondering if you expressly initialised kg_evt_bluetooth_inquiry_response to be null when you declared it.  Such as  void (*kg_evt_bluetooth_inquiry_response)()=NULL; 

But we could all make guesses until the cows come home.  Unless you actually post your code, we're all just fumbling around in the dark.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 02, 2014, 10:37 pm
You're calling the function whether or not the pointer to it is NULL.
I follow the example you give, but this statement above doesn't make sense in light of my code or your example. If that were true given the syntax I have in my code, then I would have a gigantic mess of runtime errors all over the place in this codebase and others where attempts to check for function pointer assignment are in fact dereferencing null function pointers and trying to call them. But that is demonstrably not what happens.

These examples from elsewhere online follow my understanding:



In short:



If my code were actually calling the function pointer in the conditional statement, then your code would be doing so twice. In your example (emphasis mine):

Code: [Select]
if (fn && (result = fn(paramList)))
{
  doThis();
}

Tests the function pointer and if it is not NULL, calls the function and assigns the value returned by the function to the variable "result". If the value of "result" is non-zero, the function "doThis" will be executed.
That's exactly what I'm saying. The condition "fn && (result = fn(paramList))" will first check to make sure the pointer is not null, and then if it is not null the second half of the logically AND'ed condition will be evaluated (i.e. "fn(paramList)" will run and have its return value stored in the "result" variable).

But I don't want to evaluate the return value inside my if statement's condition; I want it to be run as a result of the condition. In contrast, my code is equivalent to this mutation of your example:

Code: [Select]
if (fn)
{
  result = fn(paramList);
}


Presumably, this should test the function pointer to ensure it is not null first; then, if it is not null, call the function and store the return value. This is what I am going for. Visually rearranging my code above, it's the same:

Code: [Select]
if (kg_evt_bluetooth_inquiry_response)
{
  skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);
}


Except, I left the braces off because it's one line:

Code: [Select]
if (kg_evt_bluetooth_inquiry_response)
    skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);


Is there anything wrong with this?

I know this should not matter, but have you actually tried to compare to null?
Good question; I have not tried this yet, though it has not been necessary in other places. Using "if (kg_evt_bluetooth_inquiry_response)" should be logically the same as "if (kg_evt_bluetooth_inquiry_response != 0)" as far as I know, but it is worth trying just to be sure.

The challenge here, and why I suspect a memory leak or problem somewhere else in my code, is that seemingly irrelevant modifications suddenly cause this to work again (e.g. adding a debug output line). This is why I don't think it's necessarily due to this specific block of code, but at the same time I don't know where to look.

I think it's time for you to post ALL of your code, so that we can try to reproduce the problem.
No problem as far as I'm concerned, but it's a large file in a project with dozens of others, some of which rely on additional hardware outside the Teensy++ 2.0 board (specifically a Bluegiga WT12 module in this particular case). Reproducing the behavior externally may be difficult. The source file in question is here, on Github:



The full project is on the same Github repo:



I'm wondering if you expressly initialised kg_evt_bluetooth_inquiry_response to be null when you declared it.  Such as  void (*kg_evt_bluetooth_inquiry_response)()=NULL;
I did not, but I can try that as well.

However, the seemingly inexplicable fact remains that the exact same code works with braces around the (non-matching!) conditional code block, and doesn't work when the braces are gone, despite the conditional code being exactly one line.

And, similarly, the same code works (without braces in both cases) when a debug output line is present a dozen lines up, while it locks up without the debug line present.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: AWOL on Nov 02, 2014, 10:42 pm
Quote
If my code were actually calling the function pointer in the conditional statement, then your code would be doing so twice.
Nope.

Code: [Select]
if (fn)
does not call the function, even once, it merely tests the function pointer to decide if it is or is not null.

Code: [Select]
if (fn && fn (parameterList))
does not call the function iff "fn" is null.
It doesn't check to see if "fn" is a valid pointer, only that it is or is not null.
The function can only be called once if the pointer is not null
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 02, 2014, 10:54 pm
Code: [Select]
if (fn)
does not call the function, even once, it merely tests the function pointer to decide if it is or is not null.

Code: [Select]
if (fn && fn (parameterList))
does not call the function iff "fn" is null.
It doesn't check to see if "fn" is a valid pointer, only that it is or is not null.
I think we are talking past each other. This is exactly what I was saying about my original code example, and exactly how I want it to work and how it does work in every one of the dozens of cases where I have this kind of code, except this one. Which is why I am pulling my hair out.

I do realize that the "if (fn)" condition does not validate the memory address if one is assigned, but only checks for null vs. non-null. However, adding braces around the conditional block should have zero impact on whether the function pointer being tested contains null or not, yet this what seems to be occurring (although I know it cannot be, which is why I wish I had hardware-level debug access).
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 02, 2014, 10:58 pm
Also, to test, I have just tried adding explicit zero initializations:

Code: [Select]

uint8_t (*func_pointer)() = 0;


...and explicit non-zero conditional checks:

Code: [Select]

if (func_pointer != 0) ...


...and explicit braces around the single-line conditional blocks:

Code: [Select]

if (func_pointer != 0) { ... }


...and the Arduino compiler reports the exact same flash and RAM usage before and after this modification. I will leave the code in for slightly better (?) explicit readability and try testing with it as well, but it seems that the compiler is optimizing these explicit bits out, or at least not optimizing any differently than before they were added.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: AWOL on Nov 02, 2014, 11:09 pm
Post code - this is all just hand-waving.
(You can't put code formatting inside code tags)
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 02, 2014, 11:17 pm
Post code - this is all just hand-waving.
I did, a couple of replies ago:

The source file in question is here, on Github:

https://github.com/jrowberg/keyglove/blob/master/keyglove/support_bluetooth2_iwrap.h#L1105 (https://github.com/jrowberg/keyglove/blob/master/keyglove/support_bluetooth2_iwrap.h#L1105)

The full project is on the same Github repo:

https://github.com/jrowberg/keyglove/tree/master/keyglove (https://github.com/jrowberg/keyglove/tree/master/keyglove)
(You can't put code formatting inside code tags)
My mistake; I forgot to verify whether that would work in the preview before posting. I have removed the incorrect tags. ...actually, in retrospect (adding this via an edit), the "Preview" button on the forum seems to post, rather than preview. That's the second time this has happened. Odd.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: PaulS on Nov 03, 2014, 12:04 am
In the code you posted (at the #105 link), kg_evt_bluetooth_inquiry_response is NOT explicitly set to NULL anywhere. Where is the pointer EXPLICITLY set to NULL. Where is it set to point to some real function?
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: MarkT on Nov 03, 2014, 12:27 am
In short the presence or absence of braces is a red-herring, since it makes no
difference.  The presence of a memcpy() call earlier up is suggestive, you've likely
smashed the stack or heap. Small changes in code structure can lead to the compiler
generating different stack frame layout, which can interact with the stack corruption
in different ways - in other words the symptoms of memory corruption can be anything
at all and typically are Heisenbugs.  You may have run out of RAM.

The only ways the braces could have an effect is if there is some broken macro
being expended that turns what looks like a single assigment statement into several
statements, or if the compiler has a bug.  Are you using any macros?
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: pYro_65 on Nov 03, 2014, 02:57 am
In one function you set the name at position 13 ( index 12 )

Code: [Select]
payload[12] = 0; // no NAME yet

However in the function you referenced, you do not use index 12 at all.

Code: [Select]
if (name_len) memcpy(payload + 13, friendly_name, name_len);

So the name you copy in overruns the array by 1 byte!

And index twelve is unset. (could be zero, which is effectively an empty string.)
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 03, 2014, 04:58 am
In the code you posted (at the #105 link), kg_evt_bluetooth_inquiry_response is NOT explicitly set to NULL anywhere. Where is the pointer EXPLICITLY set to NULL. Where is it set to point to some real function?
The repo code doesn't explicitly set the pointers to NULL, though I did modify line 133 in the "support_bluetooth2_iwrap.h" file to do this as a test:

Code: [Select]

/* 0x03 */ uint8_t (*kg_evt_bluetooth_inquiry_response)(uint8_t *address, uint8_t *cod, int8_t rssi, uint8_t status, uint8_t index, uint8_t name_len, uint8_t *name_data) = 0;


The compiler output for this run was not different than leaving it uninitialized in the source. However, C standards say that global variables are initialized to zero at compile time, and these are global. They should all be zero anyway, and the behavior of the rest of the application suggests this. For testing, I've added zero assignments to the code again (see lines 131-139 (https://github.com/jrowberg/keyglove/blob/master/keyglove/support_bluetooth2_iwrap.h#L131)).

There are also no places in the code where that particular function pointer is assigned to any real functions, but this is by design because that callback is left to the user to implement only if needed. It's typically left as null because it's rarely needed for a user application.

In short the presence or absence of braces is a red-herring, since it makes no difference.  The presence of a memcpy() call earlier up is suggestive, you've likely smashed the stack or heap. Small changes in code structure can lead to the compiler generating different stack frame layout, which can interact with the stack corruption in different ways - in other words the symptoms of memory corruption can be anything at all and typically are Heisenbugs.  You may have run out of RAM.
THIS is exactly what I expect. ("Heisenbug" is such an appropriate term, too.) The bug appears until I add debug info to look at it, and then it goes away despite no apparent link between the added debug code and the broken behavior. I really do suspect something odd in my code, but can't inspect the stack in a non-invasive way to see what actually happens. Typically I would expect that just adding braces around a single line of code shouldn't do this though...and it appears that with the other changes I've played with today and left in the code, adding braces no longer fixes it (see below). I'm actually glad that this is still reliably occurring right now.

The only ways the braces could have an effect is if there is some broken macro being expended that turns what looks like a single assigment statement into several statements, or if the compiler has a bug.  Are you using any macros?
I'm not using macros that I'm aware of, no. I even checked the Arduino IDE build output (in the \Temp\buildxxxxxxxx folder in my userdir AppData location) to see if anything was modified, but it's exactly as I wrote it. The code there is just normal code, no macros.

In one function you set the name at position 13 ( index 12 )
...
And index twelve is unset. (could be zero, which is effectively an empty string.)
The contents of the "my_iwrap_evt_name" function are all correct in this case except for the fact that index 12 is unset. This is a bug that I introduced today after adding and removing various debugging attempts inside the function. I've fixed it so that this byte is assigned properly again, with the same line of code that used to be there:

Code: [Select]

payload[12] = name_len;


Code: [Select]
if (name_len) memcpy(payload + 13, friendly_name, name_len);

So the name you copy in overruns the array by 1 byte!
I don't think I've overlooked something here, but let me talk through it one more time. The payload packet is always at least 13 bytes (so "uint8_t payload[13]" is the right size). But there could be between 0 and name_len more bytes of ASCII name data (so actually "uint8_t payload[13 + name_len];" is the right size). The memcpy call doesn't include the null terminator, but only the number of actual ASCII character bytes. The payload[12] byte (the 13th byte) contains the name length, and the remaining bytes (if any) are the actual name. There should be enough memory allocated in every case. I think.

The code as it exists on Github right now reliably demonstrates the issue on my hardware. I can currently make it happen every single time I run a device inquiry ("kg_cmd_bluetooth_discover(10);"). Note that this code now has all of the following:



I have noticed that setting name_len to 0 regardless of what is actually in friendly_name appears to get avoid the freeze condition. However, adding debug serial output that spits out the value of name_len to verify that it is correct (which it is) also avoids the freeze condition, despite the fact that in that test case name_len is left alone and not set to 0. The presence of the debug output alone prevents the issue from occurring. Argh.

I will keep trying different modifications and tracing backwards to see where I might be overflowing a buffer or something.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: KenF on Nov 03, 2014, 05:21 am
Have you actually checked your available memory.  You may well be running out of sRam.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: nickgammon on Nov 03, 2014, 05:23 am
Can you confirm that the code as currently found on GitHub also exhibits these characteristics (described in the OP)? I personally don't see anything wrong with the way you have written the "if" (except readability).

I don't have all those libraries so I can't get a clean compile, but I suggest you do one both with and without the braces and compare the object files.

See http://www.gammon.com.au/forum/?id=12153#info1 (http://www.gammon.com.au/forum/?id=12153#info1) for how to disassemble the resulting .elf files.

Since adding the braces shouldn't make any difference I am guessing that you have overwritten memory somewhere, and that the braces have made the compiler make a slightly different register allocation, this different register allocation is springing a crash caused by the memory leak / overwriting.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: nickgammon on Nov 03, 2014, 05:26 am
How big can the (contents of the) variable iwrap_autocall_index get? (eg. 42)
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: pYro_65 on Nov 03, 2014, 05:36 am
It is what your code looked like was happening, as I mentioned, the function above it set index 12 to zero and commented on no name. And it seemed like you had used index 13 by accident. Ah well.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: PaulS on Nov 03, 2014, 11:07 am
Quote
The repo code doesn't explicitly set the pointers to NULL, though I did modify line 133 in the "support_bluetooth2_iwrap.h" file to do this as a test:
You can not initialize variables in a header file. What that did was make the function a pure virtual function. Which means that, if the code compiled, some other class inherits from the class you modified, AND actually implements the function. So, you are NOT looking at the code that actually gets executed.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 03, 2014, 06:54 pm
Thanks, everyone, for all of your comments so far. At Paul Stoffregen's recommendation I have ordered an AVR One and will see if what kind of debugging I can manage with Atmel Studio. (I actually found a used device available for a steal on eBay from a local US seller, so here's to hoping...)

Have you actually checked your available memory.  You may well be running out of sRam.
I haven't actually checked, no. I'll look into the best way to do this on a Teensy++ (I've seen some examples on the Arduino Playground, but haven't verified if they work as-is on this device). I suspect this is not to blame since the compiler projects RAM usage of less than 20% of what is available, and I do not have many large allocations at runtime. I'm acutely aware of the limited memory available and try whenever possible to use flash for constant data like string literals. Buffers malloc()'d at runtime are kept few and small, and free()'d in all cases to avoid memory leaks (assuming I haven't missed any, which is not impossible). But regardless, I will definitely test this to make sure.

Can you confirm that the code as currently found on GitHub also exhibits these characteristics (described in the OP)? ... this different register allocation is springing a crash caused by the memory leak / overwriting.
Yes, the code on the repository right now exhibits the problem. Thanks for the pointer to object file comparison and .elf investigation; I was not aware of this approach before. I'll read up on this and see what I can do with it.

How big can the (contents of the) variable iwrap_autocall_index get? (eg. 42)
Depending on the number of paired devices, this can be between 0 and 15. In most applications, it will not go above perhaps 2 or 3. In my test case here, it does not go above 0 because there are no paired devices. The autocall algorithm never kicks in since iwrap_pairings equals 0, and the condition on line 566 won't pass.

You can not initialize variables in a header file. What that did was make the function a pure virtual function. Which means that, if the code compiled, some other class inherits from the class you modified, AND actually implements the function. So, you are NOT looking at the code that actually gets executed.
Maybe I am misunderstanding something about the Arduino build environment, but doesn't this only apply when you are using classes? All of the functions and function pointers involved in this are not part of any class definitions. The kg_evt_bluetooth_inquiry_response function pointer is a global variable, and the my_iwrap_evt_name function is a global function, not a member of a class. Unless the Arduino IDE is wrapping everything in its own giant class, I think it is still allowed to declare and initialize this kind of value in a header file, right?
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: PaulS on Nov 04, 2014, 12:37 am
Quote
I think it is still allowed to declare and initialize this kind of value in a header file, right?
You have so many files and such useless comments that it's hard to keep track of what's what.

If the function is not in a class, then nothing I said about it applies.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 04, 2014, 01:22 am
You have so many files and such useless comments that it's hard to keep track of what's what.
Honestly? That's a bit harsh. I won't disagree on the project complexity alone though. Here's a map:

http://www.keyglove.net/docs/arduino/support__bluetooth2__iwrap_8h.html (http://www.keyglove.net/docs/arduino/support__bluetooth2__iwrap_8h.html)
http://www.keyglove.net/docs/arduino/files.html (http://www.keyglove.net/docs/arduino/files.html)

If the function is not in a class, then nothing I said about it applies.
Good to know.

I will continue investigating.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: nickgammon on Nov 04, 2014, 02:06 am
I sussed out your approach which is a bit unusual, but in itself I don't think would cause a problem*. You have a lot of code in .h files which is a bit of a novel approach, however since they are only included once, I don't think would be the issue.

More conventionally, you would have a lot of .cpp files, and manage calls from one to the other by using function prototypes in a .h file.

What device are you running this on? A Teensy++?

Is that the one with a AT90USB1286 processor? 8192 bytes of RAM, 130048 bytes of flash?

I seem to recall some issues with the Atmega2560 where code (or data?) crosses a 64 kB boundary.

* Having said that, your .h file approach effectively produces a monolithic object file which may possibly have problems compared to separate compilation units.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 04, 2014, 02:37 am
I sussed out your approach which is a bit unusual, but in itself I don't think would cause a problem...more conventionally, you would have a lot of .cpp files, and manage calls from one to the other by using function prototypes in a .h file. ... *Having said that, your .h file approach effectively produces a monolithic object file which may possibly have problems compared to separate compilation units.
I agree, this is an unusual project structure. I originally took that approach because I ran into issues trying to get the Arduino IDE to compile multiple .cpp/.h files in the same sketch, way back when I started. Even though I managed to get it to compile and work generally, it has created more than a few challenges concerning #include order and which files can reference which other files at which points. At the time, I chalked it up to the way Arduino combined sketch files. I started in 2010 with this, long before v1.0 of the IDE, and maybe things have changed since--or maybe I just had no idea what I was doing when I started, which is more likely, and it always would have worked if I did it the right way.

In either case, if it is possible to use the standard approach of .cpp/.h file combinations today, I will absolutely put in the effort needed to restructure the project.

The monolithic object file is yet another thing that I had not considered, and another reason to make the above change. Thanks for the insight here.

What device are you running this on? A Teensy++? Is that the one with a AT90USB1286 processor? 8192 bytes of RAM, 130048 bytes of flash? ...I seem to recall some issues with the Atmega2560 where code (or data?) crosses a 64 kB boundary.
Yes, it's the Teensy++ 2.0 from PJRC, the AT90USB1286 variant.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: PaulS on Nov 04, 2014, 02:44 am
Code: [Select]
Yes, it's the Teensy++ 2.0 from PJRC (thanks PaulS)
Paul Stoffregen, the maker of the teensy, posts under his complete name. I do not. You've got the wrong Paul S. there.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 04, 2014, 02:48 am
Paul Stoffregen, the maker of the teensy, posts under his complete name. I do not. You've got the wrong Paul S. there.
Ah. Oops. Sorry about that.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: nickgammon on Nov 04, 2014, 04:36 am
I originally took that approach because I ran into issues trying to get the Arduino IDE to compile multiple .cpp/.h files in the same sketch, way back when I started.
See: How to avoid the quirks of the IDE sketch file pre-preprocessing (http://www.gammon.com.au/forum/?id=12625)
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 04, 2014, 04:56 am
See: How to avoid the quirks of the IDE sketch file pre-preprocessing (http://www.gammon.com.au/forum/?id=12625)
I will definitely read through that prior to refactoring this project into a more normal format. Thanks!
^^^ EDIT: Good grief, this is fantastically simple. Amazing.

Follow-up on SRAM usage: according to Adafruit's quick RAM check function (https://learn.adafruit.com/memories-of-an-arduino/measuring-free-memory), I still have over 6kB of ram free out of the 8kB on the chip immediately prior to the offending lines of code, so it doesn't look like I'm running out of memory. That's what I thought, but all the same...good.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: darkroomsource on Nov 04, 2014, 12:33 pm
I am nowhere near the guru that many on here are, and what this reminds me of is something that I encountered about 30 years ago working on assembler on an IBM mainframe, but when a line of code stops or starts mis-behaving when another line of code is inserted (even a single blank assembly instruction), it would seem to me that there is some kind of memory bounds issue involved.
Not necessarily that there is not enough memory, but that one variable ends up writing over another variable.
Without comparing the decompiled code of the two versions (with and without braces), it would be impossible to figure out why. And even having the decompiled code would be extremely difficult.

Can you possibly re-order the sequence of functions within a portion of the code, so that this function is earlier by one or two functions, or later by one or two functions. If it continues to fail, you probably have something, somewhere, that is setting the pointer to that function (in the if statement), but setting it to something that does not act as a function, and thus it "hangs" or terminates in such a way as to mess things up. (If it doesn't continue to fail, then the code will probably fail elsewhere after having moved the function).

My guess, is that when you put the braces in, you have moved the problem to elsewhere within your code, because somewhere you are overwriting a bit of memory outside of where you think you are writing.
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: Jeff Rowberg on Nov 15, 2014, 07:54 pm
Quick update on this issue: I finished rearranging the code into standard C project form (https://github.com/jrowberg/keyglove/tree/master/keyglove), rather than one .ino and a million .h files. This has greatly simplified the code organization, especially regarding order-of-include challenges that I previously had a hard time solving.

I also noticed that at some point, my Arduino IDE had been set to run the Teensy++ at 16MHz (IDE default), rather than 8MHz. It's been modified with the 3.3v LDO, and per Paul Stoffregen's recommendation, that's a bad idea. An AT90USB1286 @ 3.3v is technically overclocked at 16MHz. Maybe the setting got reset when I updated to v1.0.6 of the IDE instead of the v1.0.5-r2 that I had been using...I really don't know how long it has been that way, but I put it back to 8MHz in the middle of the code rearrangement process.

The API event no longer freezes like it did before, despite not having some of the extra-safe adjustments to the code (braces around the conditional block, explicit NULL tests in "if" conditions, etc.). So far, things appear to be 100% reliable again.

I cannot say for sure what change fixed this behavior since there were many of them simultaneously, and I'm also not sure whether the problem is actually gone or only moved. It could still be an invalid memory dereference operation or buffer overflow that is once again invisible. In case it shows up again. I now have an AVR One debugger to attack the problem with. For the moment though, this case is closed. Thanks for all of your help and insight!
Title: Re: Non-executing code block freezes, but works if block is in { } braces
Post by: nickgammon on Nov 16, 2014, 03:20 am
Glad to help, and good to hear your code is better organized. :)