Use of #ifdef not processed consistently

[TD-er]

Solutions

1. Convert INO files to CPP

2. Manual prototype declaration

#ifdef DO_NOT_USE
typedef int32_t delaytype;
void thisShouldNotAppearInTheBinary(delaytype timer); // aded manually
void thisShouldNotAppearInTheBinary(delaytype timer) {
  delay(timer);
}
#endif

---

PlatformIO Core.
As discussed here: esp8266/Arduino#6475

Configuration

Operating system:
Windows & Linux

PlatformIO Version (platformio --version):
PlatformIO, version 4.0.3

Description of problem

When generating forward declarations of functions in the .ino.cpp, the wrapping #ifdef statements are not taken into account.

Steps to Reproduce

Simple steps to reproduce: esp8266/Arduino#6475 (comment)

Not really a very simple project at hand here, but I guess this should be enough to have in the project.ino file.

Just use the https://github.com/esp8266/Arduino/blob/74819a763bfb6e9890a57411dcea4aba221a778d/libraries/esp8266/examples/Blink/Blink.ino

and add some function like this in the .ino file:

#ifdef DO_NOT_USE
void thisShouldNotAppearInTheBinary() {
  delay(1000);
}
#endif

As long as you don't have the DO_NOT_USE flag set, you should not see it in the generated .ino.cpp file. (it should be wrapped in the #ifdef statements)
Well at least, that's what I expect should happen.

Things will fail to build if you have some object used as parameter which is only defined when a flag is set. For example:

#ifdef DO_NOT_USE
typedef int32_t delaytype;
void thisShouldNotAppearInTheBinary(delaytype timer) {
  delay(timer);
}
#endif

[uzi18]

@TD-er

platformio-core/platformio/builder/tools/piomisc.py

Line 33 in 2c24294

class InoToCPPConverter(object):

[ivankravets]

Fixed in 4ee4315 and a separate branch. Let's wait when all tested will passed.

[TD-er]

That's quick!

[ivankravets]

Yes, we try to keep PIO Core in the bugs-free state as much as we can.

[TD-er]

That's a noble pursuit :)

[uzi18]

@ivankravets thank you for fast action!
@TD-er did you tried to test this patch?

[TD-er]

@uzi18 Nope, not yet.
I was hoping there will be some kind of beta/dev build to test, since I really have no clue how to make sure it is all based on his fixes, since I (still) don't know yet how all parts of PIO integrate into eachother.

[uzi18]

$ pwd
~/.local/lib64/python2.7/site-packages
$ wget https://github.com/platformio/platformio-core/commit/4ee4315c048d129b0fc2407f38a731cf60cb9287.patch
$ patch -p1 <4ee4315c048d129b0fc2407f38a731cf60cb9287.patch 
can't find file to patch at input line 18
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|From 4ee4315c048d129b0fc2407f38a731cf60cb9287 Mon Sep 17 00:00:00 2001
|From: Ivan Kravets <me@ikravets.com>
|Date: Mon, 2 Sep 2019 23:22:42 +0300
|Subject: [PATCH] Fixed an issue with preprocessing of ``*.ino`` when macros
| were not handled // Resolve #2972
|
|---
| HISTORY.rst                            |  1 +
| platformio/builder/tools/piolib.py     | 12 +++++++++---
| platformio/builder/tools/piomisc.py    | 12 +++++++-----
| platformio/builder/tools/platformio.py |  6 ++++--
| 4 files changed, 21 insertions(+), 10 deletions(-)
|
|diff --git a/HISTORY.rst b/HISTORY.rst
|index ead20705..da29b008 100644
|--- a/HISTORY.rst
|+++ b/HISTORY.rst
--------------------------
File to patch: 
Skip this patch? [y] 
Skipping patch.
1 out of 1 hunk ignored
patching file platformio/builder/tools/piolib.py
Hunk #1 succeeded at 818 (offset 2 lines).
patching file platformio/builder/tools/piomisc.py
Hunk #1 succeeded at 87 (offset -1 lines).
patching file platformio/builder/tools/platformio.py
Hunk #1 succeeded at 61 (offset -2 lines).
Hunk #2 succeeded at 316 with fuzz 1 (offset 28 lines).

@TD-er just be sure where platfomio is installed, this should be easy with virtualenv

[ivankravets]

Sorry, but we don't have good news here. The issue was labeled with known issue and there is no easy solution here. Arduino builder uses ctag and other tools. We don't want to complicate our current INO to CPP converter which works fast and is fully independent.

We recommend using CPP instead of INO for advanced cases, like this. I did some progress in this branch https://github.com/platformio/platformio-core/tree/feature/issue-2972-ino2cpp but a few tests failed. So, switching to full output from GCC Pre-Processor makes more problems for us because all "includes" are expanded.

See updated solutions for this issue here #2972 (comment)

[TD-er]

Thanks for the update.
It does indeed look a bit more complicated.

Do you think it is possible to add an easy detection at least?
Then there can be a warning about this.

[uzi18]

@TD-er what about option on top of file header to disable prototypes auto prepending?

[TD-er]

@uzi18 The problem is mainly caused by using #ifdef statements for .ino files.

[uzi18]

but with this feature you can add proper prototypes in file manually inside #ifdefs

just add eg. #pragma platformio-proto-disable

I'm curious if our libs are also affected here

[ivankravets]

@uzi18 libraries are not affected because they are true C/C++

The problem only with INO sketch which has specific logic under ifdef guards. This is the first case for the last 5 years :)

[TD-er]

OK, this converting to .h/.cpp from .ino (combined with bad programming style of various programmers) is opening a number of boxes formally owned by someone named Pandora.

For example, in order to not be forced to modify +/- 200 files, I want to keep some stuff "global", like our settings.
But from a .cpp file, those global variables are not accessible.

Well let's define one outside the scope of a class or struct in a .h file.
This does work fine until.... you include that .h file a second time from another place. (from a .cpp file for example)
Even though the .h file is wrapped with guards like these:

#ifndef BLA_DIE_BLA_H
#define

struct bla {
  ...
};
bla _global_bla;

#endif // BLA_DIE_BLA_H

Include this blaDieBla.h from more than one location and then the compiler does not want to link because it does see multiple instances of this _global_bla.

I know it isn't the best practice in programming, but as an intermediate solution would be fine to have those global variables present.
So as a work-around I have some get functions (defined in an .ino file and forward declared in a separate .h file) to access these,
But I really would like to avoid having to change all plugin .ino files (a lot!).

Probably because of the time of day, but right now I really don't understand why the linker does see multiple instances of the same variables, since the .h file should be parsed only once.
If it were a static function defined in a .h file, then I would understand it, but right now I'm wondering if it is the same bug, or I am missing something?

This all started just to be able to exclude some parts of the code in some builds since we're hitting the max. sketch size on almost all builds :(

Error: The program size (1046204 bytes) is greater than maximum allowed (1044464 bytes)
DATA:    [====== * ** [checkprogsize] Explicit exit, status 1
  ]  57.4% (used 46984 bytes from 81920 bytes)
PROGRAM: [==========]  100.2% (used 1046204 bytes from 1044464 bytes)

[ivankravets]

PlatformIO INO->CPP converter adds only prototypes declarations for those which do not have them. So, if you add declaration under #ifdef it will work.

[TD-er]

Thanks for the reply. I already found out what was happening.
Maybe I should apply my own rule-of-thumb to posting replies like these as with merging commits... Don't do it after midnight ;)

"global" variables in a .h file will indeed lead to multiple declarations from where they are included.
If you really want to make them shared, the objects should either be static (not static declare in the .h!!), or I could create a single "global vars" object, which does include all these and get/set functions to access either the "global vars" object or for many separate objects.

So last night's reply was kind of a frustration caused by a PEBKAC :) (and lack of sleep by then)

[ivankravets]

Great! We plan to start soon work on new LDF. It's a plan to have chain+ by default. It will need a lot of refactoring but developers should be happy. So, that is why we need REAL cpp but not INO before LDF.

[TD-er]

but developers should be happy.

That's something I also firmly believe in :)

[TD-er]

PlatformIO INO->CPP converter adds only prototypes declarations for those which do not have them. So, if you add declaration under #ifdef it will work.

This is not entirely what I'm seeing here.
For example I have this macro in a .h file, which does create a number of forward declarations, a function (incl fwd declaration) and a template class object.

This macro is only called from within #ifdef statements, but still is forward declaration appears in the generated .ino.cpp file without the #ifdef wrapping statements and even worse, it also appears when the defines are not set.

The macro:

#define DEFINE_Cxxx_DELAY_QUEUE_MACRO(NNN, M)                                                                        \
  bool do_process_c##NNN####M##_delay_queue(int controller_number,                                                   \
                                           const C##NNN####M##_queue_element & element,                              \
                                           ControllerSettingsStruct & ControllerSettings);                           \
  ControllerDelayHandlerStruct<C##NNN####M##_queue_element>C##NNN####M##_DelayHandler;                               \
  void process_c##NNN####M##_delay_queue();                                                                          \
  void process_c##NNN####M##_delay_queue() {                                                                         \
    C##NNN####M##_queue_element *element(C##NNN####M##_DelayHandler.getNext());                                      \
    if (element == NULL) return;                                                                                     \
    MakeControllerSettings (ControllerSettings);                                                                     \
    LoadControllerSettings(element->controller_idx, ControllerSettings);                                             \
    C##NNN####M##_DelayHandler.configureControllerSettings(ControllerSettings);                                      \
    if (!WiFiConnected(10)) {                                                                                        \
      scheduleNextDelayQueue(TIMER_C##NNN####M##_DELAY_QUEUE, C##NNN####M##_DelayHandler.getNextScheduleTime());     \
      return;                                                                                                        \
    }                                                                                                                \
    START_TIMER;                                                                                                     \
    C##NNN####M##_DelayHandler.markProcessed(do_process_c##NNN####M##_delay_queue(M, *element, ControllerSettings)); \
    STOP_TIMER(C##NNN####M##_DELAY_QUEUE);                                                                           \
    scheduleNextDelayQueue(TIMER_C##NNN####M##_DELAY_QUEUE, C##NNN####M##_DelayHandler.getNextScheduleTime());       \
  }

And how it is called from within a .h file:

#ifdef USES_C015
DEFINE_Cxxx_DELAY_QUEUE_MACRO(0, 15)
#endif // ifdef USES_C015

It will add the first line in the macro in the generated .ino.cpp file, but the class it refers as a parameter does not exist in the code since it is also wrapped in the same #ifdef

Could it be this macro is preprocessed regardless the #ifdef wrapping it?

I also tried declaring this macro in the .ino file actually using it, but then another function defined in the macro (void process_c##NNN####M##_delay_queue();) is not yet available nor forward declared when it is called from another .ino file.

So I do seem to be in a catch-22 here, or I must strip a lot of this from the macro in order to be able to remove these objects from the built binary.

[TD-er]

As a work-around I now do all the forward declarations of the function with the templated class as argument, in the ino file instead of the macro.

It does look like this (in the .ino file)

bool do_process_c018_delay_queue(int controller_number, const C018_queue_element& element, ControllerSettingsStruct& ControllerSettings);

bool do_process_c018_delay_queue(int controller_number, const C018_queue_element& element, ControllerSettingsStruct& ControllerSettings) {
  bool  success = C018_data.txHexBytes(element.packed, ControllerSettings.Port);
  return success;
}

This allows me to have these templated objects all in the #ifdef wrappers.

[ivankravets]

Sorry for the delay and this issue. I'm going to close it. People think that we will fix it soon. To be honest, we don't plan to rewrite our INO to the CPP converter. I did a more detailed answer here #3706

So, let's be transparent with our community. There are 2 solutions:

1. Convert INO to CPP as described at https://docs.platformio.org/en/latest/faq.html#convert-arduino-file-to-c-manually
2. Use Arduino IDE.

We don't see any sense to invest in INO when no one supports it. It breaks all IDEs because they don't do this crazy converting on-the-fly for each INO file when doing parsing.