Auto prototype generation with function as argument to class instantiation

[per1234]

Using Arduino IDE 1.6.7 2015/11/07 12:42 with Windows 7 64 bit

I apologize if my terminology is off on the issue title, this is what I'm referring to:

#include "CallbackBug.h"
Task t1(&t1Callback);
void t1Callback() {}
void setup() {}
void loop() {}

CallbackBug.h:

#include <Arduino.h>
class Task {
  public:
    Task(void (*aCallback)()) {};
};

Compiling gives the error:

CallbackBug:3: error: 't1Callback' was not declared in this scope
 Task t1(&t1Callback);

If I declare the function prototype for t1Callback() it compiles fine.
The code compiles fine with Arduino IDE 1.6.5r5
Example of a library where this breaks the examples: https://github.com/arkhipenko/TaskScheduler
Originally reported at: http://forum.arduino.cc/index.php?topic=357312.msg2468586#msg2468586

[matthijskooijman]

The prototype for the t1Callback function is generated after the t1 variable. It seems the idea is to generate the prototypes just before the first function declaration, so they come after any type definitions they need (see https://github.com/arduino/arduino-builder/blob/master/src/arduino.cc/builder/prototypes_adder.go#L61). It seems this approach backfires for this testcase, unfortunately. I'm not sure if we can fix this automatically, since properly generating prototypes would require dependency analysis between all the elements in the source file and some complicated algorithm to find the proper order for them. I suspect this will become a wontfix issue, but I'll leave that to @ffissore.

ctags output:

t1Callback      /tmp/build5221f5af9dbac9dab29dc89b936a83f8.tmp/preproc/ctags_target.cpp /^Task t1(&t1Callback);$/;"     kind:variable   line:3
t1Callback      /tmp/build5221f5af9dbac9dab29dc89b936a83f8.tmp/preproc/ctags_target.cpp /^void t1Callback() {}$/;"      kind:function   line:4  signature:()  returntype:void
setup   /tmp/build5221f5af9dbac9dab29dc89b936a83f8.tmp/preproc/ctags_target.cpp /^void setup() {}$/;"   kind:function   line:5  signature:()    returntype:voidloop    /tmp/build5221f5af9dbac9dab29dc89b936a83f8.tmp/preproc/ctags_target.cpp /^void loop() {}$/;"    kind:function   line:6  signature:()    returntype:void

Preprocessed file:

#include <Arduino.h>
#line 1
#line 1 "/tmp/arduino_5221f5af9dbac9dab29dc89b936a83f8/sketch_nov09a.ino"
#include "Test.h"
Task t1(&t1Callback);
void t1Callback();
void setup();
void loop();
#line 3
void t1Callback() {}
void setup() {}
void loop() {}

[ffissore]

Yes indeed that's the source of the problem. I'm working on it. I've chatted with @facchinm and he told me that I can look at ctags "code" section and if it contains a "&" + one of the collected function names I can consider that a function pointer and use that line instead of "setup" one

[per1234]

I suspect this will become a wontfix issue

I kind of thought that might be the case, the user will just have to declare the prototype but I decided I'd at least report it so that you can put it on the list.

if it contains a "&" + one of the collected function names

I probably don't understand correctly but just in case this is any help, here's another sketch that was reported as being broken by 1.6.6, in this case they didn't use & before the function argument:

#include <TimedAction.h>

TimedAction timedAction = TimedAction(1000, blink);

void setup(){
  timedAction.enable();
}

void loop(){
  timedAction.check();
}

void blink(){
  // do stuff
}

That is using http://playground.arduino.cc/Code/TimedAction
Reported at: http://forum.arduino.cc/index.php?topic=357312.msg2469047#msg2469047

[ffissore]

Thank you @per1234 indeed that one won't work with that hack

[ffissore]

First type of function used as callback now works. Fix available with hourly builds. Second type however will NOT work. The only way to make it work is understanding that "blink" is a function pointer, which means having an Abstract Syntax Tree and/or C++ parser, which we don't have

[per1234]

Using Arduino IDE 1.6.7 2015/11/10 12:55
The following sketch still doesn't compile:

#include "CallbackBug.h"
// This doesn't compile:
Task t1(1000, &t1Callback);

// These all compile:
//Task t1 = Task(1000, &t1Callback);
//const unsigned long interval = 1000;
//Task t1(interval, &t1Callback);
//Task t1((unsigned long)1000, &t1Callback);

void t1Callback() {}
void setup() {}
void loop() {}

CallbackBug.h

class Task {
  public:
    Task(unsigned long aInterval, void (*aCallback)()) {};
};

Error:

CallbackBug:3: error: 't1Callback' was not declared in this scope
 Task t1(1000, &t1Callback);
                ^

This causes example sketches in the TaskScheduler library to not compile.

[NicoHood]

I think you need to remove the &. Look at the attach pin interrupt examples. They also dont need that. If this doesnt work, the taskscheduler library is probably coded wrong (while the bug might still exist, but the & syntax is bad for beginners anyways).

[matthijskooijman]

@NicoHood, that's not the case, the problem here is wrong ordering of prototypes, not the code itself. For functions, foo and &foo both result in the address of the function.

[per1234]

@ffissore I just wanted to make sure the issue I reported in my last comment didn't slip through the cracks. Is it a wontfix?

[per1234]

@matthijskooijman I never managed to get an answer regarding the issue I reported in my previous comment. Is it wontfix/invalid, should this issue be reopened, or should I create a new issue for it?

PS: I apologize if you're the wrong person to tag on this. I don't know who is responsible for it now that ffissore is gone and you are already somewhat familiar with the issue.

[matthijskooijman]

@per1234 No problem tagging me. I probably won't fix it, but I can at least have a look :-)

It seems that using &name_of_function in a variable initialization after the = triggers the prototype generation above that line, but when used as constructor arguments in the Class variable(&name_of_function) style, it does not. Looking at the code, the handling of the first case happens here:
https://github.com/arduino/arduino-builder/blob/master/src/arduino.cc/builder/ctags_to_prototypes.go#L82

The latter case probably has an empty tag.Code field, so this special case does not trigger. I'm not sure if expanding this special case further is a good idea, though, since it is a bit of a hack, that's bound to get false positive matches for some other cases, especially if we make it broader.

For now, I'd say it's a wontfix, but hopefully we can improve the prototype generation at some point by doing proper parsing of the source (e.g. using clang) and cases like this one can be properly handled.

@cmaglie, I wonder if it would make sense to add a collection of testcases like these (and probably also testcases that do compile) to the arduino-builder test-suite? The tester could then just verify that each testcase does or does not compile, without having to check the exact output of the compilation (which makes it easier to maintain the testcases.

[cmaglie]

I wonder if it would make sense to add a collection of testcases like these (and probably also testcases that do compile) to the arduino-builder test-suite?

Yes, I've exactly this testcase almost ready in my local repo, I've to refine some details.
And yeah, we should definitely accumulate these test cases even if they are failing or won't fix.

[ReanimationXP]

@per1234 @ffissore Please reopen this issue. If I'm reading correctly, it's not fixed and any library that uses functions either as callbacks or protothreads (the popular pubsub MQTT library and TimedAction.h protothreading library are two examples), now requires their function definitions be in the proper order as to not have the prototypes created out-of-order and thus cause scope issues.

I believe the previous commenters have provided good test cases for this and they were not all fixed correctly. If you need further details I can provide them, but without this fixed,

(a) code prior to 1.6.6 will not compile in these instances due to undeclared functions / scope issues,
(b) Arduino users typically aren't so familiar with C as to know what prototypes are and how to use them properly,
(c) these collectively result in "messy" code by having to define library class instances in the middle of the sketch after functions have been defined instead of cleanly defining them at the top of the sketch as one usually would, and
(d) while I haven't run into this yet, I would assume this could easily turn into in a catch-22 situation where two different functions or class instantiations are dependent of each other and thus simply reordering them would not fix the situation.

Really surprised this has continued this long without being reopened and fixed properly after breaking sketches and causing hassle for tons of libraries.

[ReanimationXP]

C is most important to me personally, as this quickly becomes both messy and a nightmare:

//where variables and instantiations should be

void firealarm(){
  //do stuff, this function has to be placed here since callback uses it
}
void callback(){
  firealarm();
}
//...etc

//now has to be placed here due to callback not being defined if placed at the top
EthernetClient ethClient;
PubSubClient pubsub(brokerIP, 1883, callback, ethClient);  

void setup(){
}

void loop(){
}

[ffissore]

@ReanimationXP Sorry you missed the news, but I no longer work for arduino https://groups.google.com/a/arduino.cc/forum/#!msg/developers/RTdVlpcUTSo/KqCHTgZRCAAJ

[matthijskooijman]

@ReanimationXP, it's not that we disagree that this is a problem, but it is pretty much impossible to fix with the current ctags-based approach. If we switch to full parsing (e.g. using clang, which is something that is being explored), this usecase will certainly be fixed. However, to reduce clutter in the list of open issues, I'd rather leave this issue closed.

[ReanimationXP]

I'm confused as to what would take priority over something that is effectively a code-breaking compiler change It's been a year and change already.

[matthijskooijman]

I'm not sure if I can explain this properly quickly, but I believe that this particular change is caused by changes to the preprocessing, which improves compilation for a lot of sketches, but makes it worse for a few, such as this one. So it's not a matter of fixing a bug that was introduced, there's some tradeoffs involved because the current preprocessing isn't ideal.

[ReanimationXP]

It seems like there are a lot of what ifs and the issue isn't being reopened. It'd be nice to at least have the discussion and weigh the pros and cons, particularly what were we fixing/improving before by moving to a different compilation method, what did we gain, and if it wasn't significant enough to warrant breaking code, is reversing this change possible? The benefits the change brought about may not justify the means. I believe this is causing an issue for enough people that it should at least be open.