Skip to content

Clean warnings when all warning enabled #2112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 29, 2018
Merged

Clean warnings when all warning enabled #2112

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3421590
Clean warnings when all warning enabled
Nov 27, 2018
10486fe
change due to deprecated function
Nov 27, 2018
65ee625
Update with proper variable and label
Nov 27, 2018
d5ad89a
Update esp32-hal-i2c.c
Nov 27, 2018
684e7ba
Apply changes requested
Nov 28, 2018
6a180ee
Fix warnings due to #define conflict thanks @atanisoft
Nov 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cores/esp32/Esp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ static uint32_t sketchSize(sketchSize_t response) {
.size = running->size,
};
data.start_addr = running_pos.offset;
esp_image_load(ESP_IMAGE_VERIFY, &running_pos, &data);
esp_image_verify(ESP_IMAGE_VERIFY, &running_pos, &data);
if (response) {
return running_pos.size - data.image_len;
} else {
Expand Down
22 changes: 14 additions & 8 deletions cores/esp32/esp32-hal-i2c.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,9 +248,10 @@ static void IRAM_ATTR i2cDumpCmdQueue(i2c_t *i2c)

/* Stickbreaker ISR mode debug support
*/
#if (ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_INFO)
static void i2cDumpDqData(i2c_t * i2c)
{
#if (ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_INFO)&&(defined ENABLE_I2C_DEBUG_BUFFER)
#if defined (ENABLE_I2C_DEBUG_BUFFER)
uint16_t a=0;
char buff[140];
I2C_DATA_QUEUE_t *tdq;
Expand Down Expand Up @@ -306,9 +307,12 @@ static void i2cDumpDqData(i2c_t * i2c)
}
a++;
}
#else
log_i("Debug Buffer not Enabled");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

#endif
}

#endif
#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_INFO
static void i2cDumpI2c(i2c_t * i2c)
{
log_e("i2c=%p",i2c);
Expand All @@ -332,11 +336,12 @@ static void i2cDumpI2c(i2c_t * i2c)
i2cDumpDqData(i2c);
}
}
#endif

#if (ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_INFO)
static void i2cDumpInts(uint8_t num)
{
#if (ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_INFO) && (defined ENABLE_I2C_DEBUG_BUFFER)

#if defined (ENABLE_I2C_DEBUG_BUFFER)
uint32_t b;
log_i("%u row\tcount\tINTR\tTX\tRX\tTick ",num);
for(uint32_t a=1; a<=INTBUFFMAX; a++) {
Expand All @@ -349,9 +354,10 @@ static void i2cDumpInts(uint8_t num)
log_i("Debug Buffer not Enabled");
#endif
}
#endif

static void IRAM_ATTR i2cDumpStatus(i2c_t * i2c){
#if (ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_INFO)&&(defined ENABLE_I2C_DEBUG_BUFFER)
static void IRAM_ATTR i2cDumpStatus(i2c_t * i2c){
typedef union {
struct {
uint32_t ack_rec: 1; /*This register stores the value of ACK bit.*/
Expand All @@ -377,11 +383,11 @@ static void IRAM_ATTR i2cDumpStatus(i2c_t * i2c){
sr.val= i2c->dev->status_reg.val;

log_i("ack(%d) sl_rw(%d) to(%d) arb(%d) busy(%d) sl(%d) trans(%d) rx(%d) tx(%d) sclMain(%d) scl(%d)",sr.ack_rec,sr.slave_rw,sr.time_out,sr.arb_lost,sr.bus_busy,sr.slave_addressed,sr.byte_trans, sr.rx_fifo_cnt, sr.tx_fifo_cnt,sr.scl_main_state_last, sr.scl_state_last);
#endif
}
#endif

static void i2cDumpFifo(i2c_t * i2c){
#if (ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_INFO)&&(defined ENABLE_I2C_DEBUG_BUFFER)
static void i2cDumpFifo(i2c_t * i2c){
char buf[64];
uint16_t k = 0;
uint16_t i = fifoPos+1;
Expand Down Expand Up @@ -422,8 +428,8 @@ if(i != fifoPos){// actual data
}
}while( i!= fifoPos);
}
#endif
}
#endif

static void IRAM_ATTR i2cTriggerDumps(i2c_t * i2c, uint8_t trigger, const char locus[]){
#if (ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_INFO)&&(defined ENABLE_I2C_DEBUG_BUFFER)
Expand Down
3 changes: 3 additions & 0 deletions libraries/WiFi/src/WiFiClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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("RES: %d", res);
}
switch (errno) {
case EWOULDBLOCK:
case ENOENT: //caused by vfs
Expand Down
10 changes: 6 additions & 4 deletions libraries/WiFi/src/WiFiMulti.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,17 +118,19 @@ uint8_t WiFiMulti::run(uint32_t connectTimeout)
status = WiFi.status();
}

IPAddress ip;
uint8_t * mac;
switch(status) {
case 3:
ip = WiFi.localIP();
mac = WiFi.BSSID();
{
#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_DEBUG
IPAddress ip = WiFi.localIP();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for BSSID probably with WiFi.BSSIDstr()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is not a bad idea :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

uint8_t * mac = WiFi.BSSID();
#endif
log_i("[WIFI] Connecting done.");
log_d("[WIFI] SSID: %s", WiFi.SSID().c_str());
log_d("[WIFI] IP: %d.%d.%d.%d", ip[0], ip[1], ip[2], ip[3]);
log_d("[WIFI] MAC: %02X:%02X:%02X:%02X:%02X:%02X", mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
log_d("[WIFI] Channel: %d", WiFi.channel());
}
break;
case 1:
log_e("[WIFI] Connecting Failed AP not found.");
Expand Down
4 changes: 4 additions & 0 deletions libraries/WiFi/src/WiFiSTA.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

#endif
log_d("Type: %s", sc_type_strings[*type % 3]);
} else if (status == SC_STATUS_LINK) {
wifi_sta_config_t *sta_conf = reinterpret_cast<wifi_sta_config_t *>(result);
Expand All @@ -694,7 +696,9 @@ void WiFiSTAClass::_smartConfigCallback(uint32_t st, void* result) {
_smartConfigDone = true;
} else if (status == SC_STATUS_LINK_OVER) {
if(result){
#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_DEBUG
ip4_addr_t * ip = (ip4_addr_t *)result;
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

log_d("Sender IP: " IPSTR, IP2STR(ip));
}
WiFi.stopSmartConfig();
Expand Down
11 changes: 5 additions & 6 deletions libraries/WiFiClientSecure/src/WiFiClientSecure.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D Please at least try to put some style.

if(dest) {
  free(dest);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

dest = (char*)malloc(size);
if (!dest) {
return nullptr;
}
if (size != stream.readBytes(dest, size)) {
free(dest);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes correct

return nullptr;
}
char ret[size+1];
snprintf(ret, size, "%s", dest);
free(dest);
return ret;
return dest;
}

bool WiFiClientSecure::loadCACert(Stream& stream, size_t size) {
Expand Down Expand Up @@ -290,4 +289,4 @@ int WiFiClientSecure::lastError(char *buf, const size_t size)
void WiFiClientSecure::setHandshakeTimeout(unsigned long handshake_timeout)
{
sslclient->handshake_timeout = handshake_timeout * 1000;
}
}
2 changes: 1 addition & 1 deletion tools/sdk/include/vfs/esp_vfs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
#include <sys/reent.h>
#include <sys/stat.h>
#include <sys/time.h>
#include <sys/termios.h>
//#include <sys/termios.h>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#include <dirent.h>
#include <string.h>
#include "sdkconfig.h"
Expand Down