Inhibit protocol names for not-included protocols

[tonhuisman]

Resolves: #1851

To solve the request of how to determine at runtime what protocols are actually compiled in, after disabling unneeded protocols, I chose to replace the protocol name by a question mark (empty string can't work in a \0x0 separated and \0x0\0x0 terminated string sequence). The shortest protocol name found so far is 'LG'.
To avoid having to do a double/double check, for both IR and AC, and both SEND and RECEIVE protocols, and usually both SEND and RECEIVE for a used protocol are expected to be included, checking for a valid name seems appropriate.
Used this code in the project I'm working on, for building a list of available protocols:

  int size = static_cast<int>(decode_type_t::kLastDecodeType) + 1;
  std::vector<String>decodeTypes;
  std::vector<int>decodeTypeOptions;

  int protocolCount = 0;

  for (int i = 0; i < size; i++) {
    const String protocol = typeToString(static_cast<decode_type_t>(i), false);

    if (protocol.length() > 1) {
      decodeTypeOptions.push_back(i);
      decodeTypes.push_back(protocol);
      protocolCount++;
    }
  }

The vectors are later used to populate a set of combo-boxes in the UI.

I recognize the somewhat less than ideal solution of not having a bool-returning function like isProtocolAvailable(protocol), but to avoid the maintenance burden of having multiple places in code to reflect the list of active protocols, I've implemented this compromise.

[NiKiZe]

This looks quite messy, but I realize there is not much better solutions
If the current approach/style is taken then we probably want a commend on #endif

With that said, any chance we can switch to a macro that takes protocol name and enabled defines, which then expands to the #if #else #endif
Should "\0" be included in macro or be put after?

[tonhuisman]

we probably want a commend on #endif

I'll add that

any chance we can switch to a macro that takes protocol name and enabled defines, which then expands to the #if #else #endif

That only seems to be possible by using a 2-pass pre-processor trick, as a macro is a simple text replacement, and the #if #else #endif statements need to be on a separate line, but the macro will expand to a single line only. That seems to end in some snake-pit I'd prefer to avoid

[crankyoldgit]

It screams "macro me" but I can't figure out how to do it cleanly (or at all).

Let me think about this some more before proceeding.
Also some protocols are "enabled" even if they are expressly disabled, as they are used by other protocols.
e.g.

IRremoteESP8266/src/ir_NEC.cpp

Lines 17 to 19 in 376fde2

	// This protocol is used by a lot of other protocols, hence the long list.
	#if (SEND_NEC || SEND_SHERWOOD || SEND_AIWA_RC_T501 || SEND_SANYO || \
	SEND_MIDEA24)

[tonhuisman]

Also some protocols are "enabled" even if they are expressly disabled, as they are used by other protocols.

That shouldn't be a problem, as by default everything is enabled, and we want to have the option to leave out as much unused code as possible by specifying what we don't need. If the code is still there, that's just our 'loss', but we won't use the protocol, so don't want to see it in the list.
Leaving in any re-used code is a Good Thing (™️😉), so nothing will break.

[NiKiZe]

Could we generate this from the enums instead? It would still be committed as code, but should be easy to regenerate on any change.

Could we have somehow have unit tests for any of these?

My C brain is mush atm so please excuse my mistakes here...
I guess ( DECODE_RC5 || SEND_RC5 ? D_STR_RC5 : D_STR_UNSUPPORTED) "\x0" don't work due to undefined? But if it does The compiler should still be able to optimize this? (And it can be made into Macro)

[tonhuisman]

I guess ( DECODE_RC5 || SEND_RC5 ? D_STR_RC5 : D_STR_UNSUPPORTED) "\x0" don't work due to undefined?

That condition is going to be part of a const char variable initializer, but the compiler doesn't/can't optimize that down to the desired string before inserting it in the sequence, so will return a compilation error. (error: expression cannot be used as a function)

It would work like this:

#if DECODE_RC5 || SEND_RC5
    #define CD_STR_RC5    D_STR_RC5
#else
    #define CD_STR_RC5    D_STR_UNSUPPORTED
#endif  // if DECODE_RC5 || SEND_RC5

and then later:

IRTEXT_CONST_BLOB_DECL(kAllProtocolNamesStr) {
    D_STR_UNUSED "\x0"
    CD_STR_RC5 "\x0"
//...
};

But that IMHO has somewhat of a maintenance disadvantage, as you are maintaining (nearly) the same list in 1 more place.

[tonhuisman]

And eventually I found this Reddit thread, by someone way more experienced in pre-processor macros then me.
The initial proposal works, the suggested shorter version doesn't handle combined conditions.

So we have a global mashup of these macros: (should IMHO be defined at some global level, just because of their usefulness 🤔)

#define NOTHING
#define EXPAND(...) __VA_ARGS__
#define STUFF_P(a, ...) __VA_OPT__(a)
#define STUFF(...) STUFF_P(__VA_ARGS__)
#define VA_TEST_P(a,...) __VA_OPT__(NO)##THING
#define VA_TEST(...) VA_TEST_P(__VA_ARGS__)
#define NEGATE(a) VA_TEST(a,a)
#define COND_P(cond,a,b) STUFF(a,cond)STUFF(b,NEGATE(cond))
#define COND(cond,a,b) EXPAND(COND_P(cond, a, b))

so we eventually can write it like this:

IRTEXT_CONST_BLOB_DECL(kAllProtocolNamesStr) {
    D_STR_UNUSED "\x0"
    COND(DECODE_RC5 || SEND_RC5, D_STR_RC5, D_STR_UNSUPPORTED) "\x0"
    COND(DECODE_RC6 || SEND_RC6, D_STR_RC6, D_STR_UNSUPPORTED) "\x0"
//...
};

All I need now is a suggestion where to put those global macros 😃
(or I could just leave them right before the IRTEXT_CONST_BLOB_DECL(kAllProtocolNamesStr) declaration, so we could even #undef them after use)

[TD-er]

Could we generate this from the enums instead? It would still be committed as code, but should be easy to regenerate on any change.

That sounds tricky as it also means an enum value may have different int values on different builds.
Or am I missing something?

[NiKiZe]

Could we generate this from the enums instead? It would still be committed as code, but should be easy to regenerate on any change.

That sounds tricky as it also means an enum value may have different int values on different builds. Or am I missing something?

The enum stays. This is about generating the strings for those enums.
Currently when we add an enum we manually add it as string as well.
Maybe that part could be generated to avoid the issues related to manually keeping it synced and correct syntax. Especially if we add additional logic to this string.

[tonhuisman]

Especially if we add additional logic to this string.

The logic is eventually determined from the DECODE_? and SEND_? compiler defines, I'm currently 'abusing' the function that returns the protocol name to return ? if the protocol isn't actually/actively compiled in the binary, as that covers both the DECODE and SEND parts. 😃

[TD-er]

Maybe for inspiration, you can have a look at what I did here: https://github.com/letscontrolit/ESPEasy/blob/mega/lib/RN2483-Arduino-Library/src/rn2xx3_received_types.cpp
The literal enum value is then both a (flash) string and an enum.

This way you can have only one "ugly" part which is then the switch statement to loop over the enums and then call some macro to return the flash string version of that enum.

If you need to have quotes on your strings due to spaces, you can also use some tricks like I did here with STRINGIFY

N.B. please use some toString() function to loop over the enum, with return type const __FlashStringHelper*
This way you save lots of BIN space as it would otherwise need to wrap a string constructor on each return statement.

[crankyoldgit]

Could we generate this from the enums instead? It would still be committed as code, but should be easy to regenerate on any change.

That sounds tricky as it also means an enum value may have different int values on different builds. Or am I missing something?

I'm worried about that too (i.e. one of the proposed solutions), I don't want the number/enum of a protocol to ever change.

[crankyoldgit]

And eventually I found this Reddit thread, by someone way more experienced in pre-processor macros then me. The initial proposal works, the suggested shorter version doesn't handle combined conditions.

So we have a global mashup of these macros: (should IMHO be defined at some global level, just because of their usefulness 🤔)

#define NOTHING
#define EXPAND(...) __VA_ARGS__
#define STUFF_P(a, ...) __VA_OPT__(a)
#define STUFF(...) STUFF_P(__VA_ARGS__)
#define VA_TEST_P(a,...) __VA_OPT__(NO)##THING
#define VA_TEST(...) VA_TEST_P(__VA_ARGS__)
#define NEGATE(a) VA_TEST(a,a)
#define COND_P(cond,a,b) STUFF(a,cond)STUFF(b,NEGATE(cond))
#define COND(cond,a,b) EXPAND(COND_P(cond, a, b))

so we eventually can write it like this:

IRTEXT_CONST_BLOB_DECL(kAllProtocolNamesStr) {
    D_STR_UNUSED "\x0"
    COND(DECODE_RC5 || SEND_RC5, D_STR_RC5, D_STR_UNSUPPORTED) "\x0"
    COND(DECODE_RC6 || SEND_RC6, D_STR_RC6, D_STR_UNSUPPORTED) "\x0"
//...
};

All I need now is a suggestion where to put those global macros 😃 (or I could just leave them right before the IRTEXT_CONST_BLOB_DECL(kAllProtocolNamesStr) declaration, so we could even #undef them after use)

I like this approach better (I admit that is macro foo way above my skill level / pay grade :-).
It's cleaner and easier to read/maintain IMHO.

As for where to put it/them. Let's create a macros.h.

[tonhuisman]

As for where to put it/them. Let's create a macros.h.

I suggest to use IRmacros.h to stay in line with the other filenames.

[crankyoldgit]

As for where to put it/them. Let's create a macros.h.

I suggest to use IRmacros.h to stay in line with the other filenames.

I did think that initially, so let's go with that.

[crankyoldgit]

Some automated lint/documentation checks failed:

src/IRmacros.h:17:  Missing space after ,  [whitespace/comma] [3]
src/IRmacros.h:19:  Missing space after ,  [whitespace/comma] [3]
src/IRmacros.h:20:  Missing space after ,  [whitespace/comma] [3]
src/IRmacros.h:21:  Missing space after ,  [whitespace/comma] [3]
src/IRmacros.h:26:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]

&

+ doxygen ./Doxyfile
/github/workspace/src/IRmacros.h:13: error: found documented #define NOTHING but ignoring it because ENABLE_PREPROCESSING is NO.

Other than that, it looks good to me for an initial starting point as any.

[crankyoldgit]

I spoke to soon. This breaks a lot of unit tests.
e.g.
https://github.com/crankyoldgit/IRremoteESP8266/runs/7855242915?check_suite_focus=true#step:6:4766

[tonhuisman]

Euhm, what's the preferred way to skip this Docygen message: error: found documented #define NOTHING but ignoring it because ENABLE_PREPROCESSING is NO.

• add a cond TEST/endcond around it?
• enable preprocessing with a predefined setting?

[tonhuisman]

This breaks a lot of unit tests.

I must have dropped a ball somewhere, I'm on it.

[crankyoldgit]

Euhm, what's the preferred way to skip this Docygen message: error: found documented #define NOTHING but ignoring it because ENABLE_PREPROCESSING is NO.

• add a cond TEST/endcond around it?
• enable preprocessing with a predefined setting?

Not really fussed.
I think it might be because there is no doxygen comments in the file at all.
e.g. see:

/// @file IRtext.cpp

[crankyoldgit]

Euhm, what's the preferred way to skip this Docygen message: error: found documented #define NOTHING but ignoring it because ENABLE_PREPROCESSING is NO.

• add a cond TEST/endcond around it?
• enable preprocessing with a predefined setting?

Not really fussed. I think it might be because there is no doxygen comments in the file at all. e.g. see:

/// @file IRtext.cpp

Yeah, it looks like a:

/// @file IRmacros.h

is needed.

See: https://stackoverflow.com/questions/2356120/documenting-preprocessor-defines-in-doxygen#:~:text=The%20Doxygen%20documentation%20says%3A,%2F*!%20%5Cfile%20*%2F

[crankyoldgit]

This breaks a lot of unit tests.

I must have dropped a ball somewhere, I'm on it.

It looks like an "off by one" problem. I'm guessing you missed a protocol.

[crankyoldgit]

This breaks a lot of unit tests.

I must have dropped a ball somewhere, I'm on it.

It looks like an "off by one" problem. I'm guessing you missed a protocol.

It's happening at or before RC5X I think

[tonhuisman]

While I have been able to compile all of this macro-fiesta for ESP8266 fine for the past days, this morning I had to clean my PIO global cache folders, and now I can't get the compiler to accept the __VA_OPT__ macro anymore.

src\IRmacros.h:20:25: error: expected ',' or ';' before '__VA_OPT__'

That should be available for the gnu++11 compiler, as configured for IRremoteESP8266 (AFAICS), but currently I can only build it for ESP32.
Suggestions are welcome.

[crankyoldgit]

Looks good to me. All the tests pass .. so yay!

[Jason2866]

It is a breaking change. Issue #1880