Skip to content
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

No way to set SPI_ETHERNET_SETTINGS or MAX_SOCK_NUM without modifying the library #267

Open
joeyparrish opened this issue Jun 15, 2024 · 1 comment · May be fixed by #268, #217, #160 or #168
Open

No way to set SPI_ETHERNET_SETTINGS or MAX_SOCK_NUM without modifying the library #267

joeyparrish opened this issue Jun 15, 2024 · 1 comment · May be fixed by #268, #217, #160 or #168
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement

Comments

@joeyparrish
Copy link

joeyparrish commented Jun 15, 2024

My project needs high throughput, and I can achieve it with these settings:

#define ETHERNET_LARGE_BUFFERS
#define MAX_SOCK_NUM 1
#define SPI_ETHERNET_SETTINGS SPISettings(80000000, MSBFIRST, SPI_MODE0)

But there's no way to do this configuration purely from my project. I have to edit the installed Ethernet library, which is ugly and makes it harder for others to build my project.

Trying to set these on the command line, for example, with:

arduino-cli compile -b rp2040:rp2040:rpipicow --build-property "build.extra_flags=-DETHERNET_LARGE_BUFFERS -DMAX_SOCK_NUM=1 \"-DSPI_ETHERNET_SETTINGS=SPISettings(80000000, MSBFIRST, SPI_MODE0)\""

results in warning like these:

In file included from /home/joey/Arduino/libraries/Ethernet/src/Dhcp.cpp:5:
/home/joey/Arduino/libraries/Ethernet/src/Ethernet.h:39: warning: "MAX_SOCK_NUM" redefined
   39 | #define MAX_SOCK_NUM 8
      | 
<command-line>: note: this is the location of the previous definition

Then my command-line settings do nothing, because the header definitions occurred "later".

The solution is to make the defaults for MAX_SOCK_NUM and SPI_ETHERNET_SETTINGS conditional on there not being a previous definition. Would that be acceptable? For example:

diff --git a/src/Ethernet.h b/src/Ethernet.h
index 0045de8..45ec527 100644
--- a/src/Ethernet.h
+++ b/src/Ethernet.h
@@ -33,10 +33,12 @@
 // up to 4 sockets.  W5200 & W5500 can have up to 8 sockets.  Several bytes
 // of RAM are used for each socket.  Reducing the maximum can save RAM, but
 // you are limited to fewer simultaneous connections.
-#if defined(RAMEND) && defined(RAMSTART) && ((RAMEND - RAMSTART) <= 2048)
-#define MAX_SOCK_NUM 4
-#else
-#define MAX_SOCK_NUM 8
+#if !defined(MAX_SOCK_NUM)
+# if defined(RAMEND) && defined(RAMSTART) && ((RAMEND - RAMSTART) <= 2048)
+#  define MAX_SOCK_NUM 4
+# else
+#  define MAX_SOCK_NUM 8
+# endif
 #endif
 
 // By default, each socket uses 2K buffers inside the WIZnet chip.  If
diff --git a/src/utility/w5100.h b/src/utility/w5100.h
index b2e8ec8..b5b815a 100644
--- a/src/utility/w5100.h
+++ b/src/utility/w5100.h
@@ -18,7 +18,9 @@
 #include <SPI.h>
 
 // Safe for all chips
-#define SPI_ETHERNET_SETTINGS SPISettings(14000000, MSBFIRST, SPI_MODE0)
+#if !defined(SPI_ETHERNET_SETTINGS)
+# define SPI_ETHERNET_SETTINGS SPISettings(14000000, MSBFIRST, SPI_MODE0)
+#endif
 
 // Safe for W5200 and W5500, but too fast for W5100
 // Uncomment this if you know you'll never need W5100 support.
joeyparrish added a commit to joeyparrish/Ethernet that referenced this issue Jun 15, 2024
@joeyparrish
Copy link
Author

Sent a PR in case the above solution is acceptable.

@per1234 per1234 linked a pull request Jun 15, 2024 that will close this issue
@per1234 per1234 added topic: code Related to content of the project itself type: enhancement Proposed improvement labels Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment