Skip to content

Add DHCP hostname option #77

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
46 changes: 46 additions & 0 deletions examples/DhcpHostname/DhcpHostname.ino
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
DHCP-based hostname printer

Circuit:
Ethernet shield attached to pins 10, 11, 12, 13

created 10 Dec 2016
by mykh
*/

#include <Ethernet.h>

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

Choose a reason for hiding this comment

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

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

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

};
// Hostname
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

also that just acii charset is allowed

const char* hostname = "myarduino";

// Initialize the Ethernet client library
EthernetClient client;

void setup() {
// Open serial communications and wait for port to open:
Serial.begin(9600);
// this check is only needed on the Leonardo:
while (!Serial) {
;
}

// start the Ethernet connection:
Serial.println("Setup...");
while (Ethernet.begin(mac, hostname) == 0) {
Serial.println("Failed to configure Ethernet using DHCP");
delay(10000);
Serial.println("Reconnecting...");
}

// print your hostname:
Serial.print("My Hostname: ");
Serial.println(Ethernet.hostname());
}

void loop() {
Ethernet.maintain();
}
32 changes: 26 additions & 6 deletions src/Dhcp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of NULL better use nullptr

}

int DhcpClass::beginWithDHCP(uint8_t *mac, const char *hostname, unsigned long timeout, unsigned long responseTimeout)
{
_dhcpLeaseTime=0;
_dhcpT1=0;
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • should also check for 0 < hostname length
  • instead of NULL better use nullptr

{
strcpy(_dhcpHostname, HOST_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

should also use safe str func

int offset = strlen(HOST_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

should be '\0' ?

}
else
{
strlcpy(_dhcpHostname, hostname, MAX_HOST_NAME_LENGTH + 1);
}

memcpy((void*)_dhcpMacAddr, (void*)mac, 6);
_dhcp_state = STATE_DHCP_START;
return request_DHCP_lease();
Expand Down Expand Up @@ -188,12 +207,8 @@ void DhcpClass::send_DHCP_MESSAGE(uint8_t messageType, uint16_t secondsElapsed)

// OPT - host name
buffer[16] = hostName;
buffer[17] = strlen(HOST_NAME) + 6; // length of hostname + last 3 bytes of mac address
strcpy((char*)&(buffer[18]), HOST_NAME);

printByte((char*)&(buffer[24]), _dhcpMacAddr[3]);
printByte((char*)&(buffer[26]), _dhcpMacAddr[4]);
printByte((char*)&(buffer[28]), _dhcpMacAddr[5]);
buffer[17] = strlen(_dhcpHostname); // length of hostname
strcpy((char*)&(buffer[18]), _dhcpHostname);
Copy link
Contributor

Choose a reason for hiding this comment

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

should also use safe str func


//put data in W5100 transmit buffer
_dhcpUdpSocket.write(buffer, 30);
Expand Down Expand Up @@ -420,6 +435,11 @@ IPAddress DhcpClass::getDnsServerIp()
return IPAddress(_dhcpDnsServerIp);
}

const char* DhcpClass::getHostname() const
{
return _dhcpHostname;
}

void DhcpClass::printByte(char * buf, uint8_t n )
{
char *str = &buf[1];
Expand Down
3 changes: 2 additions & 1 deletion src/Dhcp.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@
#define MAGIC_COOKIE 0x63825363
#define MAX_DHCP_OPT 16

#define HOST_NAME "WIZnet"
#define HOST_NAME "WIZnet" //default host name
#define MAX_HOST_NAME_LENGTH 12
#define DEFAULT_LEASE (900) //default lease time in seconds

#define DHCP_CHECK_NONE (0)
Expand Down
12 changes: 10 additions & 2 deletions src/Ethernet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

bug: missing return

}

int EthernetClass::begin(uint8_t *mac, const char* hostname, unsigned long timeout, unsigned long responseTimeout)
{
static DhcpClass s_dhcp;
_dhcp = &s_dhcp;
Expand All @@ -39,7 +44,7 @@ int EthernetClass::begin(uint8_t *mac, unsigned long timeout, unsigned long resp
SPI.endTransaction();

// Now try to get our config info from a DHCP server
int ret = _dhcp->beginWithDHCP(mac, timeout, responseTimeout);
int ret = _dhcp->beginWithDHCP(mac, hostname, timeout, responseTimeout);
if (ret == 1) {
// We've successfully found a DHCP server and got our configuration
// info, so set things accordingly
Expand Down Expand Up @@ -230,7 +235,10 @@ void EthernetClass::setRetransmissionCount(uint8_t num)
SPI.endTransaction();
}


const char* EthernetClass::hostname() const
{
return _dhcp ? _dhcp->getHostname() : "";
}



Expand Down
6 changes: 6 additions & 0 deletions src/Ethernet.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@

#include <Arduino.h>
#include "Client.h"
#include "Dhcp.h"
#include "Server.h"
#include "Udp.h"

Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. breaking backwards compatibility
 	static int begin(uint8_t *mac, const char *hostname = nullptr, unsigned long timeout = 60000, unsigned long responseTimeout = 4000); 
  1. not breaking backwards compatibility
 	static int begin(uint8_t *mac, unsigned long timeout = 60000, unsigned long responseTimeout = 4000, const char *hostname = nullptr); 
  1. 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);

static int maintain();
static EthernetLinkStatus linkStatus();
static EthernetHardwareStatus hardwareStatus();
Expand All @@ -96,6 +98,7 @@ class EthernetClass {
static IPAddress subnetMask();
static IPAddress gatewayIP();
static IPAddress dnsServerIP() { return _dnsServerAddress; }
const char* hostname() const;

void setMACAddress(const uint8_t *mac_address);
void setLocalIP(const IPAddress local_ip);
Expand Down Expand Up @@ -296,6 +299,7 @@ class DhcpClass {
unsigned long _lastCheckLeaseMillis;
uint8_t _dhcp_state;
EthernetUDP _dhcpUdpSocket;
char _dhcpHostname[MAX_HOST_NAME_LENGTH + 1];

int request_DHCP_lease();
void reset_DHCP_lease();
Expand All @@ -310,8 +314,10 @@ class DhcpClass {
IPAddress getGatewayIp();
IPAddress getDhcpServerIp();
IPAddress getDnsServerIp();
const char* getHostname() const;

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

int checkLease();
};

Expand Down