Add DHCP hostname option #77

shoop · 2018-11-21T09:39:16Z

Updated version of PR #50 as well as arduino/Arduino#5701.
Code taken from mykh/ArduinoEthernetLibrary@3625ad9 , lightly edited to apply to current master.
Not intended to take credit, just wanted to contribute.

greyltc · 2020-03-01T13:18:13Z

Can anyone say why seemingly great PRs like this never get merged or even commented on by the maintainers here?

PaulStoffregen · 2020-03-01T13:44:30Z

Except for very small edits, the reality is nobody is currently maintaining this library. I was maintaining it a couple years ago, which led to the 2.0.0 release. But since then I became very busy working on Teensy 4.0 (which pays my bills... work on this library is unpaid, so please understand every minute I've spent on Ethernet has been funded by Teensy sales). I intended to get back to Ethernet later this year or maybe in 2021. That may still happen. But right now I am extremely busy with a very long list of features people want on Teensy 4. FWIW, one of those features is the built-in ethernet hardware, which has bus master DMA and TCP/IP acceleration hardware.

Arduino also changed their policies, now requiring 2FA for access to any of their repositories. I have not yet set that up on my account. It's on a very long list of lower-priority things I'm too busy to worry about right now. So I no longer have access to commit to this library, even if I did have the time to do so.

Maybe someone else will start maintaining this library? It's an unpaid and mostly thankless job. The Arduino developers also tend to be cautious, so building enough trust to be allowed to maintain such a widely used library takes time.

I wish I had a better answer for you. But hopefully an honest answer is better than a "nice" one?

greyltc · 2020-03-01T14:58:14Z

Thanks for taking the time to respond and also for your volunteer efforts to make the library better for all of us! I'm struggling to understand why you're not (and seemingly nobody is) getting paid to maintain this library. This library drives sales. Why wouldn't the entity behind the Arduino brand spend the money to at least review and integrate features the community is adding for free to their products? Today it feels like they're rejecting free labor from qualified contributors in the community.

JAndrassy · 2020-03-01T15:31:24Z

it is very risky to change something in so widely used multiplatform library supporting 3 chip versions on many different shield and modules. For the 2.0.0 version Paul did tests which may took longer then to make the changes in the code,

Misiu · 2020-06-24T19:18:57Z

@per1234 @Rotzbua maybe this can be merged instead of #50?

Rotzbua

@Misiu there also some space for improvements

Rotzbua · 2020-06-25T11:53:50Z

src/Ethernet.cpp

@@ -27,6 +27,11 @@ IPAddress EthernetClass::_dnsServerAddress;
 DhcpClass* EthernetClass::_dhcp = NULL;

 int EthernetClass::begin(uint8_t *mac, unsigned long timeout, unsigned long responseTimeout)
+{
+	begin(mac, NULL, timeout, responseTimeout);


bug: missing return

Rotzbua · 2020-06-25T14:24:07Z

src/Dhcp.cpp

@@ -18,6 +23,20 @@ int DhcpClass::beginWithDHCP(uint8_t *mac, unsigned long timeout, unsigned long
 	memset(_dhcpMacAddr, 0, 6);
 	reset_DHCP_lease();

+	if (NULL == hostname)
+	{
+		strcpy(_dhcpHostname, HOST_NAME);


should also use safe str func

Rotzbua · 2020-06-25T14:28:06Z

src/Dhcp.cpp

+	if (NULL == hostname)
+	{
+		strcpy(_dhcpHostname, HOST_NAME);
+		int offset = strlen(HOST_NAME);


if somebody modifies constant HOST_NAME, here we get a too high value which cause a buffer overwrite the following lines.

Rotzbua · 2020-06-25T21:27:28Z

src/Ethernet.h


 	int beginWithDHCP(uint8_t *, unsigned long timeout = 60000, unsigned long responseTimeout = 4000);
+	int beginWithDHCP(uint8_t *, const char *, unsigned long timeout = 60000, unsigned long responseTimeout = 4000);


this class is just used internally(?), so in my opinion could be changed even if it breaks backwards compatibility.

Rotzbua · 2020-06-25T21:35:52Z

src/Dhcp.cpp

@@ -7,6 +7,11 @@
 #include "utility/w5100.h"

 int DhcpClass::beginWithDHCP(uint8_t *mac, unsigned long timeout, unsigned long responseTimeout)
+{
+	return beginWithDHCP(mac, NULL, timeout, responseTimeout);


instead of NULL better use nullptr

Rotzbua · 2020-06-25T21:39:12Z

examples/DhcpHostname/DhcpHostname.ino

+
+// MAC address
+byte mac[] = {
+  0x00, 0xAA, 0xBB, 0xCC, 0xDE, 0x02


most examples use:
0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED

my pr to use this mac in every example is pending.

Rotzbua · 2020-06-25T21:40:26Z

examples/DhcpHostname/DhcpHostname.ino

+byte mac[] = {
+  0x00, 0xAA, 0xBB, 0xCC, 0xDE, 0x02
+};
+// Hostname


add the max length of hostname to comment, because most people wont get in touch with the source code.

also that just acii charset is allowed

Rotzbua · 2020-06-25T21:41:37Z

src/Dhcp.cpp

+		printByte((char*)&(_dhcpHostname[offset + 0]), mac[3]);
+		printByte((char*)&(_dhcpHostname[offset + 2]), mac[4]);
+		printByte((char*)&(_dhcpHostname[offset + 4]), mac[5]);
+		_dhcpHostname[offset + 6] = 0;


should be '\0' ?

Rotzbua · 2020-06-25T21:52:14Z

src/Ethernet.h

@@ -80,6 +81,7 @@ class EthernetClass {
 	// gain the rest of the configuration through DHCP.
 	// Returns 0 if the DHCP configuration failed, and 1 if it succeeded
 	static int begin(uint8_t *mac, unsigned long timeout = 60000, unsigned long responseTimeout = 4000);
+	static int begin(uint8_t *mac, const char *hostname, unsigned long timeout = 60000, unsigned long responseTimeout = 4000);


overloading a function with optional arguments is a bad design pattern and should not be done.

a solution without overloading is in this case not so elegant:

breaking backwards compatibility

static int begin(uint8_t *mac, const char *hostname = nullptr, unsigned long timeout = 60000, unsigned long responseTimeout = 4000);

not breaking backwards compatibility

static int begin(uint8_t *mac, unsigned long timeout = 60000, unsigned long responseTimeout = 4000, const char *hostname = nullptr);

introduce extra function

setHostname(const char *hostname = nullptr)

must be called before calling begin()
and keep unchanged:

static int begin(uint8_t *mac, unsigned long timeout = 60000, unsigned long responseTimeout = 4000);

Rotzbua · 2020-06-25T21:54:53Z

src/Dhcp.cpp

-	printByte((char*)&(buffer[26]), _dhcpMacAddr[4]);
-	printByte((char*)&(buffer[28]), _dhcpMacAddr[5]);
+	buffer[17] = strlen(_dhcpHostname); // length of hostname
+	strcpy((char*)&(buffer[18]), _dhcpHostname);


should also use safe str func

Misiu · 2020-06-29T10:35:15Z

@shoop any chances you might look into this? 🙂

shoop · 2020-07-01T20:19:37Z

hi @Rotzbua , thank you for the review.

I don't pretend that my C code is very good, but I tried to address all your comments.
I chose to break backwards compatibility in the interface because (as seen in the example) I find the consuming version better with the MAC, HOSTNAME order before the optional timeout arguments. But if you would like to be backwards compatible I can try and do another version.

Note this is still only very lightly tested.

CLAassistant · 2021-04-09T13:39:53Z

All committers have signed the CLA.

thunermay · 2025-05-12T13:27:30Z

@Rotzbua Any reasons why this isn't merged? I could really use this feature :)
If I can help out anyhow, feel free to ask :)
BR Leon

Rotzbua · 2025-05-23T11:26:16Z

@thunermay I am not a maintainer of this repository, so I can not merge.

thunermay · 2025-05-25T10:53:40Z

@Rotzbua who is? You are marked as "Contributor", but I guess that does not mean, that you can merge?!

Add DHCP hostname option

0f48f5b

Rotzbua reviewed Jun 25, 2020

View reviewed changes

Fixes after review

adbe3fc

DRSDavidSoft mentioned this pull request May 8, 2023

Request setting custom hostName Networking-for-Arduino/EthernetENC#49

Closed


		int beginWithDHCP(uint8_t *, unsigned long timeout = 60000, unsigned long responseTimeout = 4000);
		int beginWithDHCP(uint8_t , const char , unsigned long timeout = 60000, unsigned long responseTimeout = 4000);

Add DHCP hostname option #77

Are you sure you want to change the base?

Add DHCP hostname option #77

Uh oh!

Conversation

shoop commented Nov 21, 2018

Uh oh!

greyltc commented Mar 1, 2020

Uh oh!

PaulStoffregen commented Mar 1, 2020

Uh oh!

greyltc commented Mar 1, 2020

Uh oh!

JAndrassy commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Misiu commented Jun 24, 2020

Uh oh!

Rotzbua left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Misiu commented Jun 29, 2020

Uh oh!

shoop commented Jul 1, 2020

Uh oh!

CLAassistant commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thunermay commented May 12, 2025

Uh oh!

Rotzbua commented May 23, 2025

Uh oh!

thunermay commented May 25, 2025

Uh oh!

Uh oh!

JAndrassy commented Mar 1, 2020 •

edited

Loading

CLAassistant commented Apr 9, 2021 •

edited

Loading