only allow __FlashStringHelper on AVR #11

wmarkow · 2021-04-19T20:02:59Z

I'm creating a new PR related to fix #5

With my changes:

a set of architecture independent methods are introduced:

void error(const char *vendorId, const char *message, ...);
void warn(const char *vendorId, const char *message, ...);
void info(const char *vendorId, const char *message, ...);
void debug(const char *vendorId, const char *message, ...);
void log(uint8_t logLevel, const char *vendorId, const char *message, ...);

on AVR a set of these methods are available:

void error(const __FlashStringHelper *vendorId, const __FlashStringHelper *message, ...);
void warn(const __FlashStringHelper *vendorId, const __FlashStringHelper *message, ...);
void info(const __FlashStringHelper *vendorId, const __FlashStringHelper *message, ...);
void debug(const __FlashStringHelper *vendorId, const __FlashStringHelper *message, ...);
void log(uint8_t logLevel, const __FlashStringHelper *vendorId, const __FlashStringHelper *message, ...);

templates are gone as they are not needed now
a direct usage of methods above (especially with __FlashStringHelper) may cause problems on some non avr boards. because of this a introduce a RF24LOGGER_info macro which on AVR will wrap the texts in F().

This is just a preliminary work to show what options we have in this topic.

Any suggestions are welcome.

wmarkow

Just put some useful comments to explain code pieces.

wmarkow · 2021-04-19T20:04:38Z

examples/BasicSerialLogger/BasicSerialLogger.ino

-  rf24Logger.info((const __FlashStringHelper*) vendorID, F("RF24Log/examples/BasicSerialLogger"));
+  rf24Logger.info((const __FlashStringHelper*) vendorIDFlash, F("RF24Log/examples/BasicSerialLogger"));
+
+  RF24LOGGER_info("RF24LogExample", "Log message here");


A call of RF24LOGGER_info() macro is independent on the used architecture. No FlashStringHelper here.
This macro (instead of rf24Logger.info() method) must be used for logging purposes.

wmarkow · 2021-04-19T20:05:34Z

src/RF24LogHandler.h

@@ -54,6 +55,7 @@ class RF24LogHandler
                     const __FlashStringHelper *vendorId,
                     const __FlashStringHelper *message,
                     va_list *args);
+#endif


This will be available only on AVR.
All log handlers are updated as well.

wmarkow · 2021-04-19T20:07:23Z

src/RF24LogHandler.h

@@ -38,10 +38,11 @@ class RF24LogHandler
     * same order for which they appear in the @p message
     */
    virtual void log(uint8_t logLevel,
-                     const __FlashStringHelper *vendorId,
+                     const char *vendorId,


This method is available for all architectures. However we could exclude it on AVR.
All log handlers are updated as well.

wmarkow · 2021-04-19T20:08:26Z

src/RF24Logger.h

+#if defined (ARDUINO_ARCH_AVR)
+  #define RF24LOG_FLASHIFY(A) F(A)
+  #define RF24LOGGER_info(vendorId, message, ...) (rf24Logger.log(RF24LogLevel::INFO, RF24LOG_FLASHIFY(vendorId), RF24LOG_FLASHIFY(message), ##__VA_ARGS__))


On AVR RF24LOGGER_info() macro will wrap the provided texts in F() macro. This will save RAM and store the texts in FLASH. Usage of this macro doesn't depend on architecture. The macro will be resolved during compilation time. This macro (instead of rf24Logger.info() method) must be used for logging purposes.

wmarkow · 2021-04-19T20:10:16Z

src/RF24Logger.h

+  #define RF24LOGGER_info(vendorId, message, ...) (rf24Logger.log(RF24LogLevel::INFO, vendorId, message, ##__VA_ARGS__))
+#endif


On non AVR architectures the macro will pass the arguments just as they are, so as a char pointers (I hope - this is not tested yet).

wmarkow · 2021-04-19T20:12:14Z

src/mem_usage.txt

+00000d4c l     F .text	000000a6 _ZN20RF24StreamLogHandler5writeEhPKcS1_PPv
+00000f36 l     F .text	00000080 _GLOBAL__I_65535_0_RF24AbstractLogHandler.cpp.o.1928
+008003f2 l     O .bss	00000005 rf24SerialLogHandler
+00800141 l     O .data	0000000e _ZTV20RF24StreamLogHandler


It is not seen here but the memory usage by the library components is still the same. I just replaced the templates into a methods with well known arguments.

wmarkow · 2021-04-19T20:31:09Z

I'm just not so happy with the following usage:

RF24LOGGER_info("RF24LogExample", "Log message 1");
RF24LOGGER_info("RF24LogExample", "Log message 2");
RF24LOGGER_info("RF24LogExample", "Log message 3");

On AVR the macro will be translated into:

rf24Logger.info(RF24LogLevel::INFO, F("RF24LogExample"), F("Log message 1"));
rf24Logger.info(RF24LogLevel::INFO, F("RF24LogExample"), F("Log message 2"));
rf24Logger.info(RF24LogLevel::INFO, F("RF24LogExample"), F("Log message 3"));

The vendor id is the same across the library (so it will not change) but with this solution it will be stored in FLASH three times, so FLASH will be a bit wasted here.

We could make a requirement that on AVR platforms a __FlashStringHelper* should be passed here (this should be documented somewhere). Something like here:

const char PROGMEM vendorIDFlash[] = "RF24LogExample";

void setup()
{
    RF24LOGGER_info(vendorIDFlash, "Log message here");
}

then we just need to rework the macro definition so on AVR it will acst the argument to __FlashStringHelper*:

#if defined (ARDUINO_ARCH_AVR)
  #define RF24LOG_FLASHIFY(A) F(A)
  #define RF24LOGGER_info(vendorId, message, ...) (rf24Logger.log(RF24LogLevel::INFO, (const __FlashStringHelper*)(vendorId), RF24LOG_FLASHIFY(message), ##__VA_ARGS__))
#elif
  #define RF24LOGGER_info(vendorId, message, ...) (rf24Logger.log(RF24LogLevel::INFO, vendorId, message, ##__VA_ARGS__))
#endif

I have retested with Arduino Nano and it seems to be working.

I have the feeling that on non AVR architectures the PROGMEM is defained as empty (to be compatible with AVR), so on non AVR the vendorID argument will be treated as const char*. Will that work on non AVR?

2bndy5 · 2021-04-19T21:29:17Z

PROGMEM is also defined without a value in the RF24 lib for compatibility with Linux (do a search for PROGMEM keyword in that repo to see how it's used).

src/RF24Logger.h

2bndy5 · 2021-04-20T02:39:52Z

I thought that I read somewhere that the const keyword automatically allocates a variable into flash on non-AVR boards.

For simplicity, we could just do away with info(), warn(), error(), & debug(), and just work with log() until the lib is ready for polish. I still have concerns about scoping when using a macro-wrapped method. Imagine the user does roll-their-own handler, wouldn't that exclude the ability to use a macro-wrapped function call (unless the user re-defines it - but then the compiler would likely throw a warning about re-defining a macro)?

Replace '#elif' with '#else'.

wmarkow · 2021-04-20T07:15:36Z

For simplicity, we could just do away with info(), warn(), error(), & debug(), and just work with log()

For this I already reported #6

I still have concerns about scoping when using a macro-wrapped method. Imagine the user does roll-their-own handler, wouldn't that exclude the ability to use a macro-wrapped function call (unless the user re-defines it - but then the compiler would likely throw a warning about re-defining a macro)?

I'm not quite sure how it will behave when a user redefines macro or creates a new one. To support multi-platform (to be transparent for __FlashStringHelper) only those macro(s) should be used for logging purposes. The direct usage of rf24Logger object should be prohibited or at least not allowed by adding an information in the docs.

Maybe C/C++ offers some mechanism for scoping the variables, so rf24Logger will not be available for the direct usage? We just expose the macro(s).

src/handlers/RF24StreamLogHandler.h

2bndy5 · 2021-04-20T08:05:14Z

For this I already reported #6

Sorry I forgot about that thread.

I'm not quite sure how it will behave when a user redefines macro or creates a new one

Usually (in my experience), the compiler will output a warning when a macro is redefined. We could wrap the macro definitions with

#ifndef RF24LOGGER_info
    #define RF24LOGGER_info() //...
#endif

This way someone could define the macro-wrapped function before including the src file that defines the lib's default definition. I imagine this would only be useful for a person that wants to make all their logged INFO messages to use a INFO sublevel instead.

wmarkow · 2021-04-20T09:16:21Z

I imagine this would only be useful for a person that wants to make all their logged INFO messages to use a INFO sublevel instead.

We need also to expose void RF24LOGGER::log(uint8_t logLevel, const char *vendorId, const char *message, ...) with a macro. With this macro it will be possible to supply a custom log level (like INFO sublevel).

However I can imagine that the developer can still redefine macro. This may be dangerous because a macro provided by RF24Log library is an API method. There can be a situation where an end project uses two different libraries (from a different maintainers). Imagine now: first library uses our RF24Log macros and second library redefines macro (introduces a new sublevel parameter) and changes the API. The second library may compile and work correctly while the first one may not compile because the macro is now changed and the passed arguments doesn't match.

2bndy5 · 2021-04-20T10:46:40Z

Maybe C/C++ offers some mechanism for scoping the variables, so rf24Logger will not be available for the direct usage? We just expose the macro(s).

We could encapsulate internal-only members with a namespace. As far as documenting the prohibited members, I think doxygen will show a lib-specific namespace in the "class list" (I need to test this though) which might be enough to signify that the encapsulated members are for internal-only usage (namespaces get a docstring just like any other data structure) - we could also add a @note or @warning declaring as much.

…ed under ARDUINO_ARCH_AVR.

2bndy5 · 2021-04-20T17:53:22Z

Ok. Now the arduino CI is outputting an error related to all examples that use explicit calls to functions that accept __FlashStringHelper as parameters' datatypes. However, there is the arduino namespace that I haven't mentioned:

/home/runner/work/RF24Log/RF24Log/examples/BasicSerialLogger/BasicSerialLogger.ino:43:102: error: no matching function for call to 'RF24Logger::info(const arduino::__FlashStringHelper*, const arduino::__FlashStringHelper*)'

It's possible that the previously seen error from the RF24LogHandler.h was because it wasn't using namespace arduino; like it should've. Note that the arduino namespace is only used for non-AVR boards.

/home/runner/Arduino/libraries/RF24Log/src/RF24LogHandler.h:41:28: error: '__FlashStringHelper' does not name a type

Or it could been from not including String.h (not WString.h) because that's where __FlashStringHelper is declared for non-AVR boards.

wmarkow · 2021-04-20T20:04:27Z

Does it relates the builds for non AVR architectures? Like i.e.

arduino:mbed:nano33ble
arduino:samd:nano_33_iot
arduino:samd:adafruit_circuitplayground_m0
etc.

Those platforms may have not support for __FlashStringHelper while our examples still use the rf24Logger methods that accepts __FlashStringHelper as argument.

If we go with the solution with RF24LOGGER_info(vendorId, message, ...) macro then I can rework all of our examples to use those kinds of macros (RF24LOGGER_error(), RF24LOGGER_warn(), RF24LOGGER_info() and RF24LOGGER_debug()). Then our examples should be not dependent on __FlashStringHelper and those errors should be gone. Let's give it a try.

2bndy5 · 2021-04-20T20:32:56Z

The more I think about it, I really like the idea of the user only using macros. That usage exactly matches the current IF_SERIAL_DEBUG macro that all RF24* libs currently use.

2bndy5 · 2021-04-20T21:17:04Z

Those platforms may have not support for __FlashStringHelper while our examples still use the rf24Logger methods that accepts __FlashStringHelper as argument.

That's what is perplexing. The platforms in question all use arduino/ArduinoCore-API (which defines __FlashStringHelper within an arduino namespace in the String.h file). The arduino/ArduinoCore-avr repo has __FlashStringHelper defined in WString.h file (not encapsulated within an arduino namespace).

2bndy5 · 2021-04-21T08:12:14Z

src/RF24Logger.h

@@ -18,10 +18,25 @@
 #ifndef SRC_RF24LOGGER_H_
 #define SRC_RF24LOGGER_H_

-#include <WString.h>
+#include <String.h>


We could do a

#if defined(ARDUINO_API_VERSION) #include <String.h> #else #include <WString.h> #endif

You mean that sometimes must use WString.h and in other cases String.h?
Interesting.
I have both files for Arduino NANO:

packages\arduino\hardware\avr\1.8.3\cores\arduino\Wstring.h packages\arduino\tools\avr-gcc\7.3.0-atmel3.6.1-arduino7\avr\include\string.h

However the second file is string.h with lowercase. Does it matter when I use <String.h> or <string.h>?

Let's try with your suggestion.

It is much better no. Thanks @2bndy5. On some non AVR platforms it still doesn't work:

In file included from /home/runner/work/RF24Log/RF24Log/examples/AllLogLevelsLogger/AllLogLevelsLogger.ino:18:0: /home/runner/Arduino/libraries/RF24Log/src/RF24Logger.h:22:10: fatal error: String.h: No such file or directory #include <String.h> ^~~~~~~~~~ compilation terminated.

Hm. I have built it locally against arduino:samd:nano_33_iot and it was working for me with #include <String.h>

Does it matter when I use <String.h> or <string.h>?

Hah, it matters. Now it looks nice when I included <string.h> (lowercase).

2bndy5 · 2021-04-21T10:00:52Z

examples/LogLevelsLogger/LogLevelsLogger.ino

  Serial.println("Set log level to ALL");
  rf24SerialLogHandler.setLogLevel(RF24LogLevel::ALL);
-  logSimpleRamMessage();
+  logSimpleMessage();


I love this example, but it could be simplified (and much more interactive) if we allow the user to change the logLevel using the Serial terminal. That way we could dynamically demonstrate filtering.

Yes, with new changes it is a bit irrelevant where the message will be stored (RAM or FLASH) - from the point of the unified logging library - as it will be decided at compile time depending on the architecture (AVR or non-AVR).

Interactive examples is a cool idea. I have created #12 for this.

2bndy5 · 2021-04-21T10:54:14Z

@wmarkow I pushed a change to master so that the Doxygen action should not fail on pull-requests. Can you please re-base your branch on current master? Its not important really, but this could be the 1^st PR to pass all checks 🥇

2bndy5 · 2021-04-21T19:38:09Z

Just realized I can rebase these commits when merging... @wmarkow @Avamander any objections? I very much like the direction this PR takes.

wmarkow · 2021-04-22T10:31:32Z

@2bndy5 if you can merge this PR then go ahead.

You suggested me to make a rebase on my local branch and the push the changes to the remote branch. I'm not quite convinced if this is good solution because after rebasing (rebase changes the git history tree) my local branch and remote branch will diverge, so they will not have a common root commit. I think I needed then to make a force push to the remote branch which may have its negative impact in some cases. I have no idea what is the best Github flow for this kind of situations. Just needed to read some docs about it.

You can just merge the PR, it should be then fine.

wmarkow · 2021-04-22T14:37:13Z

I'm also testing RF24Log library with ESP32 board where PROGMEM is defined empty and __FlashStringHelper is also defined but in my opinion just for backward compatibility. Actually nothing is read from FLASH and the strings are created in RAM (or heap?).

It looks correct; it compiles and run nice.

wmarkow added 3 commits April 19, 2021 08:35

Preliminary work: check if ARDUINO_ARCH_AVR defined.

4da9211

Introduce RF24LOGGER_info macro.

bd04577

Replace '#ifdef' with '#if defined'.

a459bef

wmarkow commented Apr 19, 2021

View reviewed changes

2bndy5 requested changes Apr 20, 2021

View reviewed changes

src/RF24Logger.h Outdated Show resolved Hide resolved

Improve RF24LOGGER_info macro so it saves more FLASH on AVR.

6028a49

Replace '#elif' with '#else'.

2bndy5 requested changes Apr 20, 2021

View reviewed changes

src/handlers/RF24StreamLogHandler.h Show resolved Hide resolved

2bndy5 changed the title ~~Fix #5~~ only allow __FlashStringHelper on AVR Apr 20, 2021

Fix RF24StreamLogHandler::appendFormattedMessage - it should be defin…

7879708

…ed under ARDUINO_ARCH_AVR.

wmarkow added 3 commits April 21, 2021 08:35

Introduce more macros. Adopt examples.

8f92fbd

In examples set indent spaces to 2.

1cddd3e

Another fix in example indentation.

bf650e6

2bndy5 reviewed Apr 21, 2021

View reviewed changes

wmarkow added 2 commits April 21, 2021 09:23

Fix WString vs String inclusion.

30e9a34

Let's try to include string.h (lowercase).

d348f9b

2bndy5 approved these changes Apr 21, 2021

View reviewed changes

2bndy5 merged commit 3e3dfc4 into nRF24:main Apr 22, 2021

wmarkow mentioned this pull request Apr 22, 2021

Consider replacing error(), warn(), info(), debug() with macros. #6

Closed

		#define RF24LOGGER_info(vendorId, message, ...) (rf24Logger.log(RF24LogLevel::INFO, vendorId, message, ##__VA_ARGS__))
		#endif

only allow __FlashStringHelper on AVR #11

only allow __FlashStringHelper on AVR #11

Uh oh!

Conversation

wmarkow commented Apr 19, 2021 • edited by 2bndy5 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wmarkow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmarkow Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmarkow Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

wmarkow Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

wmarkow Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmarkow Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmarkow Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

wmarkow commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2bndy5 commented Apr 19, 2021

Uh oh!

Uh oh!

2bndy5 commented Apr 20, 2021

Uh oh!

wmarkow commented Apr 20, 2021

Uh oh!

Uh oh!

2bndy5 commented Apr 20, 2021

Uh oh!

wmarkow commented Apr 20, 2021

Uh oh!

2bndy5 commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2bndy5 commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wmarkow commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2bndy5 commented Apr 20, 2021

Uh oh!

2bndy5 commented Apr 20, 2021

Uh oh!

2bndy5 Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

wmarkow Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

wmarkow Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmarkow Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

2bndy5 Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmarkow Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

wmarkow commented Apr 19, 2021 •

edited by 2bndy5

Loading

wmarkow left a comment •

edited

Loading

wmarkow Apr 19, 2021 •

edited

Loading

wmarkow Apr 19, 2021 •

edited

Loading

wmarkow Apr 19, 2021 •

edited

Loading

wmarkow commented Apr 19, 2021 •

edited

Loading

2bndy5 commented Apr 20, 2021 •

edited

Loading

2bndy5 commented Apr 20, 2021 •

edited

Loading

wmarkow commented Apr 20, 2021 •

edited

Loading

wmarkow Apr 21, 2021 •

edited

Loading

2bndy5 Apr 21, 2021 •

edited

Loading

2bndy5 commented Apr 21, 2021 •

edited

Loading