RP2040 support added#118
Conversation
MathewHDYT
left a comment
There was a problem hiding this comment.
General feedback for the Pull Request:
The added defines can be kept I would move them into Constants.h tough, mainly because the ThingsBoardHttp.h class relies on the same methods.
Additionally I would add another check, because setting snprintf_P to snprintf, might cause problems when actually using PROGMEM. Furthermore if the device doesn't support the methods to read PROGMEM strings it shouldn't use PROGMEM strings.
So something like this:
...
// Overwrite snprintf_P (snprintf alternative for PROGMEM strings) and vsprintf_P, if the method was not defined.
// Additionally we need to ensure to only overwrite if PROGMEM is disabled, because trying to read PROGMEM string,
// without the correct PROGMEM methods will cause issues.
#ifndef snprintf_P && !THINGSBOARD_ENABLE_PROGMEM
#define snprintf_P snprintf
#endif // snprintf_P && !THINGSBOARD_ENABLE_PROGMEM
#ifndef vsnprintf_P && !THINGSBOARD_ENABLE_PROGMEM
#define vsnprintf_P vsnprintf
#endif // vsnprintf_P && !THINGSBOARD_ENABLE_PROGMEM
#endif // Constants_hFor the <stdarg.h> include that should be fine because it is in the c-standard library it shouldn't cause devices that do not support the C++ STL.
Additionally it makes sense to include it anyway, when a va_list with an exapandable argument ... is used. See inline static const uint8_t detectSize(const char *msg, ...).
Therefore I would keep that simply the same as well but simply also add it to ThingsBoardHttp, because the same method is defined in that class.
So something like this:
// Library includes.
#include <stdarg.h>
#include <ArduinoHttpClient.h>For the last point are you sure the complete <Arduino> header needs to be included in the ThingsBoardLogger.h file. On the ESP32 the additional include <WString.h> should be enough to fix the issue of the missing F() macro. (If that doesn't work including Arduino is fine)
Additionally I would also include the Configuration.h, as it is currently not taken into consideration, for the checked #define in the .cpp file.
So something like this:
// Header include.
#include <ThingsBoardDefaultLogger.h>
// Library include.
#include <WString.h>
#include <HardwareSerial.h>
// Local include.
#include "Configuration.h"
void ThingsBoardDefaultLogger::log(const char *msg) {
#if THINGSBOARD_ENABLE_PROGMEM
Serial.print(F("[TB] "));
#else
Serial.print("[TB] ");
#endif // THINGSBOARD_ENABLE_PROGMEM
Serial.println(msg);
}
Temporary fix:
From what you said I think this board does support the C++ STL, therefore it might be a good idea to make the THINGSBOARD_ENABLE_STL configurable as well, at least for the time being.
So instead of checking if the includes exist we additionally check if they are already defined and only define them then, this would make it possible to define them before including the ThingsBoard.h file and then simply use the non STL version of the libraries.
To do that you would need to change the Configuration.h file.
So something like this:
# ifdef __has_include
# if __has_include(<string>) && __has_include(<functional>) && __has_include(<vector>) && __has_include(<iterator>) && __has_include(<cassert>)
# ifndef THINGSBOARD_ENABLE_STL
# define THINGSBOARD_ENABLE_STL 1
# endif
# else
# ifndef THINGSBOARD_ENABLE_STL
# define THINGSBOARD_ENABLE_STL 0
# endif
# endif
# else
# ifdef ARDUINO
# ifndef THINGSBOARD_ENABLE_STL
# define THINGSBOARD_ENABLE_STL 0
# endif
# else
# ifndef THINGSBOARD_ENABLE_STL
# define THINGSBOARD_ENABLE_STL 1
# endif
# endif
# endifThen it can be simply added that if C++ STL causes issues, it can be disabled before including ThingsBoard.h, when adding this #define THINGSBOARD_ENABLE_STL 0.
This would need to be added to the documentation as well, but you can simply copy how the documentation on #define THINGSBOARD_ENABLE_PROGMEM 0 was done and adjust it for THINGSBOARD_ENABLE_STL.
|
About the Perhaps it would be needed to make an own new fork of the library including some of the fixes and improvements? I added the fix onto my own Fork for now. The best thing would still be if it got merged tough... |
Added support for RP2040 chip (tested on Arduino Nano Connect (RP2040)).
Requires fix for pubsubclient (for callbacks) - knolleary/pubsubclient#993
Hi, @MathewHDYT, how do you think, how do we can pass this point? (I mean workaround for this issue, until fix will be merged)