Clean warnings when all warning enabled #2112

luc-github · 2018-11-27T14:15:43Z

Not used variables / functions due to debug log

Dual define with different values :
cores\esp32/binary.h
#define B110 6
#define B1000000 64

tools/sdk/include/newlib/sys/termios.h
#define B110 3
#define B1000000 23

Local variable returned in WiFiclient Secure

Not used variables / functions due to debug log Dual define with different values : cores\esp32/binary.h #define B110 6 #define B1000000 64 tools/sdk/include/newlib/sys/termios.h #define B110 3 #define B1000000 23 Local variable returned in WiFiclient Secure

atanisoft · 2018-11-27T15:15:18Z

libraries/WiFi/src/WiFiClient.cpp

@@ -439,6 +439,9 @@ uint8_t WiFiClient::connected()
    if (_connected) {
        uint8_t dummy;
        int res = recv(fd(), &dummy, 0, MSG_DONTWAIT);
+        if(res < 0) {
+            log_e("%d", errno);


logging the errno is not meaningful without a message to say why it is being logged. It would also be beneficial to log the "res" value here possibly.

Yes you are right should be res and not errno - will update it

changed to : log_e("RES: %d", res);

atanisoft · 2018-11-27T15:21:03Z

cores/esp32/esp32-hal-i2c.c

@@ -306,9 +307,12 @@ static void i2cDumpDqData(i2c_t * i2c)
        }
        a++;
    }
+#else
+    log_i("Debug Buffer not Enabled");


it would be better to not log anything at all here so as not to clutter the console

Except, if debug level is at "info" I assume you actually want to see information?
The debug buffer in hal-i2c takes up 2570 bytes of ram if enabled.
That is why this message exists. If you set debug_level at or above "info" I expect you have a problem you are trying to solve.

@stickbreaker this function isn't called from what I can tell if the level isn't at least DEBUG.

I have tried to follow same as others functions - should I change it ?

My bad, I got confused with the add/subtract highlighting. I just though you were questioning the "enable" message. My only excuse is that I hadn't had my coffee before I read your Post 😃

I don't have any problems with your changes.

One other question on the debug subject: I added a an app level control for debug output with Wire.setDebugFlags( uint32_t setBits, uint32_t resetBits);, but I did not document how/when to use it. Is there any standard for such documentation?

I used it around calls to a "recalcitrant" i2c devices when I get unexpected responses. I don't know if it would be good to put "debug" conditionals around all levels of this code (i.e. Wire() and hal-i2c).

What do you think?

@stickbreaker @atanisoft I followed same as line 354 - it means, I need to remove original 354 as well ? Or I misunderstood ?
thanks for your review

@luc-github Let me spend some time looking through it. The "debug_buffer" only houses interrupt activity(used inside the ISR). I just scanned through i2cDumpDqData() and don't know why I had it conditional on "debug_buffer"?

@stickbreaker thank you - I did not wanted to break anything that is why I have mimic existing code

For some reason github won't let me create an edited version of your version? So, I'm going to create an edited version the current espressif version in my repo? (might work? very confusing to me anyway!)

me-no-dev · 2018-11-27T20:32:39Z

libraries/WiFi/src/WiFiMulti.cpp

-                    mac = WiFi.BSSID();
+                    {
+#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_DEBUG
+                    IPAddress ip = WiFi.localIP();


you can not declare new variables inside the case. Add another #if for their definitions above.

the variable is not necessary at all even, the log_d line can be updated to just:

log_d("[WIFI] IP: %s", WiFi.localIP().toString().c_str());

same for BSSID probably with WiFi.BSSIDstr()

that is not a bad idea :)

Ok I will do
For my personnal knowledge - as I have set brace, why it is not correct to declare new local variables inside the case ?

me-no-dev · 2018-11-27T20:34:14Z

libraries/WiFi/src/WiFiSTA.cpp

@@ -683,7 +683,9 @@ void WiFiSTAClass::_smartConfigCallback(uint32_t st, void* result) {
    smartconfig_status_t status = (smartconfig_status_t) st;
    log_d("Status: %s", sc_status_strings[st % 5]);
    if (status == SC_STATUS_GETTING_SSID_PSWD) {
+#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_DEBUG
        smartconfig_type_t * type = (smartconfig_type_t *)result;


you should also include the call to log_d in the #if. Same for the one below. Makes the reason for the #if clearer.

me-no-dev · 2018-11-27T20:37:20Z

libraries/WiFiClientSecure/src/WiFiClientSecure.cpp

@@ -232,18 +232,17 @@ bool WiFiClientSecure::verify(const char* fp, const char* domain_name)
 }

 char *WiFiClientSecure::_streamLoad(Stream& stream, size_t size) {
-  char *dest = (char*)malloc(size);
+  static char *dest = nullptr;
+  if(dest)free(dest);


:D Please at least try to put some style.

if(dest) { free(dest); }

atanisoft · 2018-11-27T20:37:46Z

libraries/WiFi/src/WiFiSTA.cpp

            ip4_addr_t * ip = (ip4_addr_t *)result;
+#endif


move the log_d line inside the if/endif block as well

me-no-dev · 2018-11-27T20:37:55Z

libraries/WiFiClientSecure/src/WiFiClientSecure.cpp

-  char *dest = (char*)malloc(size);
+  static char *dest = nullptr;
+  if(dest)free(dest);
+  dest = (char*)malloc(size);
  if (!dest) {
    return nullptr;
  }
  if (size != stream.readBytes(dest, size)) {
    free(dest);


set dest to null here, or free will be called again on this region and bad things might happen :P

Yes correct

atanisoft · 2018-11-27T20:39:18Z

tools/sdk/include/vfs/esp_vfs.h

@@ -27,7 +27,7 @@
 #include <sys/reent.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <sys/termios.h>
+//#include <sys/termios.h>


this comes from ESP-IDF and should be fixed on that side instead

Well that was my problem as in esp_arduino it is the solution in current context - but in IDF it may be necessary , I do not use idf and this change could be rejected

me-no-dev

I found some things here and there :P please take care :)

luc-github

I did the requested changes - including putting back :

#include <sys/termios.h>

But requested this change in IDF has big chance to be rejected, No ?
Or any other solution ? for :

Dual define with different values :
cores\esp32/binary.h
#define B110 6
#define B1000000 64

tools/sdk/include/newlib/sys/termios.h
#define B110 3
#define B1000000 23

is binary.h a must ?

luc-github · 2018-11-28T08:27:10Z

libraries/WiFi/src/WiFiSTA.cpp

            ip4_addr_t * ip = (ip4_addr_t *)result;
+#endif


luc-github · 2018-11-28T08:28:45Z

libraries/WiFiClientSecure/src/WiFiClientSecure.cpp

-  char *dest = (char*)malloc(size);
+  static char *dest = nullptr;
+  if(dest)free(dest);
+  dest = (char*)malloc(size);
  if (!dest) {
    return nullptr;
  }
  if (size != stream.readBytes(dest, size)) {
    free(dest);


Yes correct

luc-github · 2018-11-28T08:31:21Z

tools/sdk/include/vfs/esp_vfs.h

@@ -27,7 +27,7 @@
 #include <sys/reent.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <sys/termios.h>
+//#include <sys/termios.h>


Well that was my problem as in esp_arduino it is the solution in current context - but in IDF it may be necessary , I do not use idf and this change could be rejected

luc-github · 2018-11-28T09:07:40Z

libraries/WiFi/src/WiFiMulti.cpp

-                    mac = WiFi.BSSID();
+                    {
+#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_DEBUG
+                    IPAddress ip = WiFi.localIP();


Ok I will do
For my personnal knowledge - as I have set brace, why it is not correct to declare new local variables inside the case ?

me-no-dev · 2018-11-28T10:42:31Z

we can not touch sdk files :) the binary strings are still used here and there in libs :( if we get rid of them I expect other issues to come

me-no-dev · 2018-11-28T10:44:21Z

so is this ready to be merged now? all i2c things resolved?

luc-github · 2018-11-28T10:47:37Z

No I still wait for @stickbreaker

What could be the workaround for #define conflict - it is source of potential issue also I think

me-no-dev · 2018-11-28T12:32:08Z

where is esp_vfs included that it causes this issue?

luc-github · 2018-11-28T12:44:52Z

@me-no-dev it is here : https://github.com/espressif/arduino-esp32/blob/master/libraries/SD/src/sd_diskio.cpp#L19

stickbreaker · 2018-11-28T13:21:21Z

@luc-github keep the debug_enable conjunction, that way just setting debug level doesn't cause serial monitor spamming. The two step process, hopefully we wont use i2c debugging constantly.

I had forgotten my reasoning for the double level activation.

Chuck

me-no-dev · 2018-11-28T13:32:48Z

@luc-github in this case you undefine the offending arduino bin definitions before you include the file. That file does not care for Arduino binary definitions ;)

luc-github · 2018-11-28T13:49:22Z

so we left as it is ? as a known issue ?If it is agreed I am fine with it ^_^ - so PR is done

In file included from C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0/tools/sdk/include/vfs/esp_vfs.h:30:0,

                 from C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0\libraries\SD\src\sd_diskio.cpp:19:

C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0/tools/sdk/include/newlib/sys/termios.h:97:0: warning: "B110" redefined

 #define B110        3

 ^

In file included from C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0\cores\esp32/Arduino.h:40:0,

                 from C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0\libraries\SD\src\sd_diskio.h:17,

                 from C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0\libraries\SD\src\sd_diskio.cpp:14:

C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0\cores\esp32/binary.h:65:0: note: this is the location of the previous definition

 #define B110 6

 ^

In file included from C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0/tools/sdk/include/vfs/esp_vfs.h:30:0,

                 from C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0\libraries\SD\src\sd_diskio.cpp:19:

C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0/tools/sdk/include/newlib/sys/termios.h:118:0: warning: "B1000000" redefined

 #define B1000000   23

 ^

In file included from C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0\cores\esp32/Arduino.h:40:0,

                 from C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0\libraries\SD\src\sd_diskio.h:17,

                 from C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0\libraries\SD\src\sd_diskio.cpp:14:

C:\Users\luc\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.0\cores\esp32/binary.h:277:0: note: this is the location of the previous definition

 #define B1000000 64

 ^

me-no-dev · 2018-11-28T22:05:39Z

@luc-github what will happen if before https://github.com/espressif/arduino-esp32/blob/master/libraries/SD/src/sd_diskio.cpp#L19 you add #undef B1000000 and so on?

atanisoft · 2018-11-28T22:44:21Z

@luc-github what will happen if before https://github.com/espressif/arduino-esp32/blob/master/libraries/SD/src/sd_diskio.cpp#L19 you add #undef B1000000 and so on?

is esp_vfs.h even used? from a cursory glance at sd_diskio.cpp it doesn't appear to be

luc-github · 2018-11-29T08:11:55Z

@me-no-dev seems avoid issue

#undef B110 
#undef B1000000

@atanisoft seems not as commented compilation succeed and no warning

extern "C" {
    #include "diskio.h"
    #include "ffconf.h"
    #include "ff.h"
    //#include "esp_vfs.h"
    #include "esp_vfs_fat.h"
    char CRC7(const char* data, int length);
    unsigned short CRC16(const char* data, int length);
}

@me-no-dev @atanisoft I think commenting #include "esp_vfs.h" is the way to go
what do you think ?

me-no-dev · 2018-11-29T09:14:36Z

sure! go ahead :)

luc-github · 2018-11-29T10:11:27Z

I think everything is clear now - thank you guys ^_^

Luc added 2 commits November 27, 2018 15:15

change due to deprecated function

10486fe

atanisoft reviewed Nov 27, 2018

View reviewed changes

Luc added 2 commits November 27, 2018 18:06

Update with proper variable and label

65ee625

Update esp32-hal-i2c.c

d5ad89a

me-no-dev reviewed Nov 27, 2018

View reviewed changes

atanisoft reviewed Nov 27, 2018

View reviewed changes

me-no-dev reviewed Nov 27, 2018

View reviewed changes

atanisoft reviewed Nov 27, 2018

View reviewed changes

me-no-dev requested changes Nov 27, 2018

View reviewed changes

Apply changes requested

684e7ba

luc-github commented Nov 28, 2018

View reviewed changes

me-no-dev approved these changes Nov 28, 2018

View reviewed changes

Fix warnings due to #define conflict thanks @atanisoft

6a180ee

me-no-dev merged commit ce340fa into espressif:master Nov 29, 2018

Clean warnings when all warning enabled #2112

Clean warnings when all warning enabled #2112

Conversation

luc-github commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

me-no-dev left a comment

Choose a reason for hiding this comment

luc-github left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

me-no-dev commented Nov 28, 2018

me-no-dev commented Nov 28, 2018

luc-github commented Nov 28, 2018

me-no-dev commented Nov 28, 2018

luc-github commented Nov 28, 2018

stickbreaker commented Nov 28, 2018

me-no-dev commented Nov 28, 2018

luc-github commented Nov 28, 2018

me-no-dev commented Nov 28, 2018

atanisoft commented Nov 28, 2018

luc-github commented Nov 29, 2018 • edited

me-no-dev commented Nov 29, 2018

luc-github commented Nov 29, 2018

luc-github left a comment •

edited

luc-github commented Nov 29, 2018 •

edited