-
Notifications
You must be signed in to change notification settings - Fork 82
Ethernet: examples and readme update #342
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.
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
Conversation
Memory usage change @ d4b1340
Click for full report table
Click for full report CSV
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Just some thought impulses re naming.
|
||
/* Portenta H7 + Ethernet shield */ | ||
#if defined(BOARD_HAS_ETHERNET) | ||
#define SECRET_OPTIONAL_IP "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are those secrets called _OPTIONAL_
?
- https://github.com/arduino-libraries/ArduinoIoTCloud#what | ||
*/ | ||
|
||
#include "arduino_secrets.h" | ||
#include "thingProperties.h" | ||
|
||
#if defined(ESP32) | ||
#if !defined(LED_BUILTIN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's much better 🤣 I'm afraid I was the author of the original implementation =)
#elif defined(BOARD_HAS_ETHERNET) | ||
/* DHCP mode */ | ||
//EthernetConnectionHandler ArduinoIoTPreferredConnection; | ||
/* Manual mode. It will fallback in DHCP mode if SECRET_OPTIONAL_IP is invalid or equal to "0.0.0.0" */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, the parameter is optional when using DHCP. But maybe a better naming would be SECRET_MANUAL_IP
. Also its not really a secret, just a configuration item. Maybe ETHERNET_MANUAL_CONFIG_IP
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your clarification. I thought so. In this case there's not much you do I suppose 🤷.
Thanks @aentinger . I've used the same naming used when exporting a sketch from the cloud editor. This defines are indeed not secrets, but it wasn't possible to remove the Another 🤔 fact is they are defined into the arduino_secrets.h file and the filename is not really appropriate in case of ethernet configuration parameter. I feel like renaming only, would not be the best solution: maybe we should also move them in the thingProperties.h file. What do you think?
|
Add EthernetConnectionHandler in library examples and the list of ethernet enabled boards in the readme.
Bonus:
0802445 Make Basic example build for all ESP32 boards
7c4eb09 Align header format of library examples