3 participants
any possibility of this getting merged in? even when setting a static ip, my host is going offline as soon as a lease expires.
Can you please try it out an get back with your results. With more tests it will be easier to get it in.
i had to back down to pre-1.0 for my production hardware, will try to set up another arduino/shield over the weekend with this patch.
Thanks for implementing this. It doesn't seem to have broken anything - I'll report back whether it works in a few days after the lease has expired on the test Arduino I've just set up :-)
I'll also read through the code properly then too, but in the meantime, a few thoughts or items for further discussion that come to mind after a quick look at it...
1) I think it would be useful if Ethernet::maintain() returned a value to let the user know if the IP address had been renewed. Most of the time people would ignore it, but it's possible that a sketch would have read the IP address and so letting them know that it should be re-read is useful to have.
2) Do we need to keep the DhcpClass object around in the Ethernet object all the time? I can see why you've done that, as it keeps the DHCP logic all nicely self-contained, but it also means that any sketch using DHCP will lose around 50 bytes of RAM, which seems a lot when the ideal solution would just store a timeout value. I don't have an obvious solution, but thought I'd start the discussion and then revisit it when I'm doing a more detailed code review.
i am seeing much better behavior with this changeset.
Nice to see that it worked.
1) I agree to 100%. Something for next week... Should it differentiate between a renewal and rebind ( new ip ) or just one return value regardless?
2) I was also annoyed with having the Dhcp object around but I couldn't find a clean and nice way of implementing it differently. C/C++ is not my native language so I might miss something obvious. If someone do have a way of fix it then please do it or describe how to do it.
Still haven't had chance to dig into the code, but thought I'd at least report back on my testing so far - haven't actually seen it renew a lease :-/ Obviously that's not a very useful bug report, more of a comment on my investigatons. When I get time to look through the code properly I'll also
1) I don't mind particularly about differentiating between the two, but if it's easy to do then we might as well. If 0 is the "didn't do anything" return then in the trivial case where you don't care which it was out of renewal or rebind, you can just do "if (Ethernet.maintain())"
2) Again, will see if I can come up with something when I run through the code.
I'm closing this pull request and refer to the issue at http://code.google.com/p/arduino/issues/detail?id=716
The work is till in progress, just not in this pull request.
kmpm
closed this
Showing 4 changed files with 160 additions and 25 deletions. Show Diff Stats
@@ -11,13 +11,32 @@ |
||
| 11 | 11 |
|
| 12 | 12 |
int DhcpClass::beginWithDHCP(uint8_t *mac, unsigned long timeout, unsigned long responseTimeout) |
| 13 | 13 |
{
|
| 14 |
- uint8_t dhcp_state = STATE_DHCP_START; |
|
| 15 |
- uint8_t messageType = 0; |
|
| 16 |
- |
|
| 17 |
- // zero out _dhcpMacAddr, _dhcpSubnetMask, _dhcpGatewayIp, _dhcpLocalIp, _dhcpDhcpServerIp, _dhcpDnsServerIp |
|
| 18 |
- memset(_dhcpMacAddr, 0, 26); |
|
| 14 |
+ _dhcpLeaseTime=0; |
|
| 15 |
+ _dhcpT1=0; |
|
| 16 |
+ _dhcpT2=0; |
|
| 17 |
+ _lastCheck=0; |
|
| 18 |
+ _timeout = timeout; |
|
| 19 |
+ _responseTimeout = responseTimeout; |
|
| 20 |
+ |
|
| 21 |
+ // zero out _dhcpMacAddr |
|
| 22 |
+ memset(_dhcpMacAddr, 0, 6); |
|
| 23 |
+ reset_DHCP_lease(); |
|
| 19 | 24 |
|
| 20 | 25 |
memcpy((void*)_dhcpMacAddr, (void*)mac, 6); |
| 26 |
+ _dhcp_state = STATE_DHCP_START; |
|
| 27 |
+ return request_DHCP_lease(); |
|
| 28 |
+} |
|
| 29 |
+ |
|
| 30 |
+void DhcpClass::reset_DHCP_lease(){
|
|
| 31 |
+ // zero out _dhcpSubnetMask, _dhcpGatewayIp, _dhcpLocalIp, _dhcpDhcpServerIp, _dhcpDnsServerIp |
|
| 32 |
+ memset(_dhcpLocalIp, 0, 20); |
|
| 33 |
+} |
|
| 34 |
+ |
|
| 35 |
+int DhcpClass::request_DHCP_lease(){
|
|
| 36 |
+ |
|
| 37 |
+ uint8_t messageType = 0; |
|
| 38 |
+ |
|
| 39 |
+ |
|
| 21 | 40 |
|
| 22 | 41 |
// Pick an initial transaction ID |
| 23 | 42 |
_dhcpTransactionId = random(1UL, 2000UL); |
@@ -35,55 +54,75 @@ int DhcpClass::beginWithDHCP(uint8_t *mac, unsigned long timeout, unsigned long |
||
| 35 | 54 |
|
| 36 | 55 |
unsigned long startTime = millis(); |
| 37 | 56 |
|
| 38 |
- while(dhcp_state != STATE_DHCP_LEASED) |
|
| 57 |
+ while(_dhcp_state != STATE_DHCP_LEASED)
|
|
| 39 | 58 |
{
|
| 40 |
- if(dhcp_state == STATE_DHCP_START) |
|
| 59 |
+ if(_dhcp_state == STATE_DHCP_START)
|
|
| 41 | 60 |
{
|
| 42 | 61 |
_dhcpTransactionId++; |
| 43 | 62 |
|
| 44 | 63 |
send_DHCP_MESSAGE(DHCP_DISCOVER, ((millis() - startTime) / 1000)); |
| 45 |
- dhcp_state = STATE_DHCP_DISCOVER; |
|
| 64 |
+ _dhcp_state = STATE_DHCP_DISCOVER;
|
|
| 46 | 65 |
} |
| 47 |
- else if(dhcp_state == STATE_DHCP_DISCOVER) |
|
| 66 |
+ else if(_dhcp_state == STATE_DHCP_REREQUEST){
|
|
| 67 |
+ _dhcpTransactionId++; |
|
| 68 |
+ send_DHCP_MESSAGE(DHCP_REQUEST, ((millis() - startTime)/1000)); |
|
| 69 |
+ _dhcp_state = STATE_DHCP_REQUEST; |
|
| 70 |
+ } |
|
| 71 |
+ else if(_dhcp_state == STATE_DHCP_DISCOVER) |
|
| 48 | 72 |
{
|
| 49 | 73 |
uint32_t respId; |
| 50 |
- messageType = parseDHCPResponse(responseTimeout, respId); |
|
| 74 |
+ messageType = parseDHCPResponse(_responseTimeout, respId);
|
|
| 51 | 75 |
if(messageType == DHCP_OFFER) |
| 52 | 76 |
{
|
| 53 | 77 |
// We'll use the transaction ID that the offer came with, |
| 54 | 78 |
// rather than the one we were up to |
| 55 | 79 |
_dhcpTransactionId = respId; |
| 56 | 80 |
send_DHCP_MESSAGE(DHCP_REQUEST, ((millis() - startTime) / 1000)); |
| 57 |
- dhcp_state = STATE_DHCP_REQUEST; |
|
| 81 |
+ _dhcp_state = STATE_DHCP_REQUEST;
|
|
| 58 | 82 |
} |
| 59 | 83 |
} |
| 60 |
- else if(dhcp_state == STATE_DHCP_REQUEST) |
|
| 84 |
+ else if(_dhcp_state == STATE_DHCP_REQUEST)
|
|
| 61 | 85 |
{
|
| 62 | 86 |
uint32_t respId; |
| 63 |
- messageType = parseDHCPResponse(responseTimeout, respId); |
|
| 87 |
+ messageType = parseDHCPResponse(_responseTimeout, respId);
|
|
| 64 | 88 |
if(messageType == DHCP_ACK) |
| 65 | 89 |
{
|
| 66 |
- dhcp_state = STATE_DHCP_LEASED; |
|
| 90 |
+ _dhcp_state = STATE_DHCP_LEASED;
|
|
| 67 | 91 |
result = 1; |
| 92 |
+ //use default lease time if we didn't get it |
|
| 93 |
+ if(_dhcpLeaseTime == 0){
|
|
| 94 |
+ _dhcpLeaseTime = DEFAULT_LEASE; |
|
| 95 |
+ } |
|
| 96 |
+ //calculate T1 & T2 if we didn't get it |
|
| 97 |
+ if(_dhcpT1 == 0){
|
|
| 98 |
+ //T1 should be 50% of _dhcpLeaseTime |
|
| 99 |
+ _dhcpT1 = _dhcpLeaseTime >> 1; |
|
| 100 |
+ } |
|
| 101 |
+ if(_dhcpT2 == 0){
|
|
| 102 |
+ //T2 should be 87.5% (7/8ths) of _dhcpLeaseTime |
|
| 103 |
+ _dhcpT2 = _dhcpT1 << 1; |
|
| 104 |
+ } |
|
| 105 |
+ _renewInSec = _dhcpT1; |
|
| 106 |
+ _rebindInSec = _dhcpT2; |
|
| 68 | 107 |
} |
| 69 | 108 |
else if(messageType == DHCP_NAK) |
| 70 |
- dhcp_state = STATE_DHCP_START; |
|
| 109 |
+ _dhcp_state = STATE_DHCP_START;
|
|
| 71 | 110 |
} |
| 72 | 111 |
|
| 73 | 112 |
if(messageType == 255) |
| 74 | 113 |
{
|
| 75 | 114 |
messageType = 0; |
| 76 |
- dhcp_state = STATE_DHCP_START; |
|
| 115 |
+ _dhcp_state = STATE_DHCP_START;
|
|
| 77 | 116 |
} |
| 78 | 117 |
|
| 79 |
- if(result != 1 && ((millis() - startTime) > timeout)) |
|
| 118 |
+ if(result != 1 && ((millis() - startTime) > _timeout))
|
|
| 80 | 119 |
break; |
| 81 | 120 |
} |
| 82 | 121 |
|
| 83 | 122 |
// We're done with the socket now |
| 84 | 123 |
_dhcpUdpSocket.stop(); |
| 85 | 124 |
_dhcpTransactionId++; |
| 86 |
-
|
|
| 125 |
+ |
|
| 87 | 126 |
return result; |
| 88 | 127 |
} |
| 89 | 128 |
|
@@ -302,8 +341,26 @@ uint8_t DhcpClass::parseDHCPResponse(unsigned long responseTimeout, uint32_t& tr |
||
| 302 | 341 |
} |
| 303 | 342 |
} |
| 304 | 343 |
break; |
| 305 |
- |
|
| 344 |
+ |
|
| 345 |
+ case dhcpT1value : |
|
| 346 |
+ opt_len = _dhcpUdpSocket.read(); |
|
| 347 |
+ _dhcpUdpSocket.read((uint8_t*)&_dhcpT1, sizeof(_dhcpT1)); |
|
| 348 |
+ _dhcpT1 = ntohl(_dhcpT1); |
|
| 349 |
+ break; |
|
| 350 |
+ |
|
| 351 |
+ case dhcpT2value : |
|
| 352 |
+ opt_len = _dhcpUdpSocket.read(); |
|
| 353 |
+ _dhcpUdpSocket.read((uint8_t*)&_dhcpT2, sizeof(_dhcpT2)); |
|
| 354 |
+ _dhcpT2 = ntohl(_dhcpT2); |
|
| 355 |
+ break; |
|
| 356 |
+ |
|
| 306 | 357 |
case dhcpIPaddrLeaseTime : |
| 358 |
+ opt_len = _dhcpUdpSocket.read(); |
|
| 359 |
+ _dhcpUdpSocket.read((uint8_t*)&_dhcpLeaseTime, sizeof(_dhcpLeaseTime)); |
|
| 360 |
+ _dhcpLeaseTime = ntohl(_dhcpLeaseTime); |
|
| 361 |
+ _renewInSec = _dhcpLeaseTime; |
|
| 362 |
+ break; |
|
| 363 |
+ |
|
| 307 | 364 |
default : |
| 308 | 365 |
opt_len = _dhcpUdpSocket.read(); |
| 309 | 366 |
// Skip over the rest of this option |
@@ -322,6 +379,39 @@ uint8_t DhcpClass::parseDHCPResponse(unsigned long responseTimeout, uint32_t& tr |
||
| 322 | 379 |
return type; |
| 323 | 380 |
} |
| 324 | 381 |
|
| 382 |
+int DhcpClass::checkLease(){
|
|
| 383 |
+ unsigned long now = millis(); |
|
| 384 |
+ signed long snow = (long)now; |
|
| 385 |
+ int rc=0; |
|
| 386 |
+ if (_lastCheck != 0){
|
|
| 387 |
+ if ( snow - (long)_secTimeout >= 0 ){
|
|
| 388 |
+ _renewInSec -= 1; |
|
| 389 |
+ _rebindInSec -= 1; |
|
| 390 |
+ _secTimeout = snow + 1000; |
|
| 391 |
+ } |
|
| 392 |
+ |
|
| 393 |
+ //if we have a lease but should renew, do it |
|
| 394 |
+ if (_dhcp_state == STATE_DHCP_LEASED && _renewInSec <=0){
|
|
| 395 |
+ _dhcp_state = STATE_DHCP_REREQUEST; |
|
| 396 |
+ rc = 1 + request_DHCP_lease(); |
|
| 397 |
+ } |
|
| 398 |
+ |
|
| 399 |
+ //if we have a lease or is renewing but should bind, do it |
|
| 400 |
+ if( (_dhcp_state == STATE_DHCP_LEASED || _dhcp_state == STATE_DHCP_START) && _rebindInSec <=0){
|
|
| 401 |
+ //this should basically restart completely |
|
| 402 |
+ _dhcp_state = STATE_DHCP_START; |
|
| 403 |
+ reset_DHCP_lease(); |
|
| 404 |
+ rc = 3 + request_DHCP_lease(); |
|
| 405 |
+ } |
|
| 406 |
+ } |
|
| 407 |
+ else{
|
|
| 408 |
+ _secTimeout = snow + 1000; |
|
| 409 |
+ } |
|
| 410 |
+ |
|
| 411 |
+ _lastCheck = now; |
|
| 412 |
+ return rc; |
|
| 413 |
+} |
|
| 414 |
+ |
|
| 325 | 415 |
IPAddress DhcpClass::getLocalIp() |
| 326 | 416 |
{
|
| 327 | 417 |
return IPAddress(_dhcpLocalIp); |
@@ -45,6 +45,13 @@ |
||
| 45 | 45 |
#define MAX_DHCP_OPT 16 |
| 46 | 46 |
|
| 47 | 47 |
#define HOST_NAME "WIZnet" |
| 48 |
+#define DEFAULT_LEASE (900) //default lease time in seconds |
|
| 49 |
+ |
|
| 50 |
+#define DHCP_CHECK_NONE (0) |
|
| 51 |
+#define DHCP_CHECK_RENEW_FAIL (1) |
|
| 52 |
+#define DHCP_CHECK_RENEW_OK (2) |
|
| 53 |
+#define DHCP_CHECK_REBIND_FAIL (3) |
|
| 54 |
+#define DHCP_CHECK_REBIND_OK (4) |
|
| 48 | 55 |
|
| 49 | 56 |
enum |
| 50 | 57 |
{
|
@@ -139,8 +146,19 @@ class DhcpClass {
|
||
| 139 | 146 |
uint8_t _dhcpGatewayIp[4]; |
| 140 | 147 |
uint8_t _dhcpDhcpServerIp[4]; |
| 141 | 148 |
uint8_t _dhcpDnsServerIp[4]; |
| 149 |
+ uint32_t _dhcpLeaseTime; |
|
| 150 |
+ uint32_t _dhcpT1, _dhcpT2; |
|
| 151 |
+ signed long _renewInSec; |
|
| 152 |
+ signed long _rebindInSec; |
|
| 153 |
+ signed long _lastCheck; |
|
| 154 |
+ unsigned long _timeout; |
|
| 155 |
+ unsigned long _responseTimeout; |
|
| 156 |
+ unsigned long _secTimeout; |
|
| 157 |
+ uint8_t _dhcp_state; |
|
| 142 | 158 |
EthernetUDP _dhcpUdpSocket; |
| 143 | 159 |
|
| 160 |
+ int request_DHCP_lease(); |
|
| 161 |
+ void reset_DHCP_lease(); |
|
| 144 | 162 |
void presend_DHCP(); |
| 145 | 163 |
void send_DHCP_MESSAGE(uint8_t, uint16_t); |
| 146 | 164 |
|
@@ -153,6 +171,7 @@ class DhcpClass {
|
||
| 153 | 171 |
IPAddress getDnsServerIp(); |
| 154 | 172 |
|
| 155 | 173 |
int beginWithDHCP(uint8_t *, unsigned long timeout = 60000, unsigned long responseTimeout = 4000); |
| 174 |
+ int checkLease(); |
|
| 156 | 175 |
}; |
| 157 | 176 |
|
| 158 | 177 |
#endif |
@@ -10,7 +10,8 @@ |
||
| 10 | 10 |
|
| 11 | 11 |
int EthernetClass::begin(uint8_t *mac_address) |
| 12 | 12 |
{
|
| 13 |
- DhcpClass dhcp; |
|
| 13 |
+ _dhcp = new DhcpClass(); |
|
| 14 |
+ |
|
| 14 | 15 |
|
| 15 | 16 |
// Initialise the basic info |
| 16 | 17 |
W5100.init(); |
@@ -18,15 +19,15 @@ int EthernetClass::begin(uint8_t *mac_address) |
||
| 18 | 19 |
W5100.setIPAddress(IPAddress(0,0,0,0).raw_address()); |
| 19 | 20 |
|
| 20 | 21 |
// Now try to get our config info from a DHCP server |
| 21 |
- int ret = dhcp.beginWithDHCP(mac_address);
|
|
| 22 |
+ int ret = _dhcp->beginWithDHCP(mac_address);
|
|
| 22 | 23 |
if(ret == 1) |
| 23 | 24 |
{
|
| 24 | 25 |
// We've successfully found a DHCP server and got our configuration info, so set things |
| 25 | 26 |
// accordingly |
| 26 |
- W5100.setIPAddress(dhcp.getLocalIp().raw_address()); |
|
| 27 |
- W5100.setGatewayIp(dhcp.getGatewayIp().raw_address()); |
|
| 28 |
- W5100.setSubnetMask(dhcp.getSubnetMask().raw_address()); |
|
| 29 |
- _dnsServerAddress = dhcp.getDnsServerIp(); |
|
| 27 |
+ W5100.setIPAddress(_dhcp->getLocalIp().raw_address()); |
|
| 28 |
+ W5100.setGatewayIp(_dhcp->getGatewayIp().raw_address()); |
|
| 29 |
+ W5100.setSubnetMask(_dhcp->getSubnetMask().raw_address()); |
|
| 30 |
+ _dnsServerAddress = _dhcp->getDnsServerIp(); |
|
| 30 | 31 |
} |
| 31 | 32 |
|
| 32 | 33 |
return ret; |
@@ -66,6 +67,28 @@ void EthernetClass::begin(uint8_t *mac, IPAddress local_ip, IPAddress dns_server |
||
| 66 | 67 |
_dnsServerAddress = dns_server; |
| 67 | 68 |
} |
| 68 | 69 |
|
| 70 |
+void EthernetClass::maintain(){
|
|
| 71 |
+ if(_dhcp != NULL){
|
|
| 72 |
+ //we have a pointer to dhcp, use it |
|
| 73 |
+ switch (_dhcp->checkLease() ){
|
|
| 74 |
+ case DHCP_CHECK_NONE: |
|
| 75 |
+ //nothing done |
|
| 76 |
+ break; |
|
| 77 |
+ case DHCP_CHECK_RENEW_OK: |
|
| 78 |
+ case DHCP_CHECK_REBIND_OK: |
|
| 79 |
+ //we might have got a new IP. |
|
| 80 |
+ W5100.setIPAddress(_dhcp->getLocalIp().raw_address()); |
|
| 81 |
+ W5100.setGatewayIp(_dhcp->getGatewayIp().raw_address()); |
|
| 82 |
+ W5100.setSubnetMask(_dhcp->getSubnetMask().raw_address()); |
|
| 83 |
+ _dnsServerAddress = _dhcp->getDnsServerIp(); |
|
| 84 |
+ break; |
|
| 85 |
+ default: |
|
| 86 |
+ //this is actually a error, it will retry though |
|
| 87 |
+ break; |
|
| 88 |
+ } |
|
| 89 |
+ } |
|
| 90 |
+} |
|
| 91 |
+ |
|
| 69 | 92 |
IPAddress EthernetClass::localIP() |
| 70 | 93 |
{
|
| 71 | 94 |
IPAddress ret; |
@@ -6,12 +6,14 @@ |
||
| 6 | 6 |
#include "IPAddress.h" |
| 7 | 7 |
#include "EthernetClient.h" |
| 8 | 8 |
#include "EthernetServer.h" |
| 9 |
+#include "Dhcp.h" |
|
| 9 | 10 |
|
| 10 | 11 |
#define MAX_SOCK_NUM 4 |
| 11 | 12 |
|
| 12 | 13 |
class EthernetClass {
|
| 13 | 14 |
private: |
| 14 | 15 |
IPAddress _dnsServerAddress; |
| 16 |
+ DhcpClass* _dhcp; |
|
| 15 | 17 |
public: |
| 16 | 18 |
static uint8_t _state[MAX_SOCK_NUM]; |
| 17 | 19 |
static uint16_t _server_port[MAX_SOCK_NUM]; |
@@ -23,6 +25,7 @@ class EthernetClass {
|
||
| 23 | 25 |
void begin(uint8_t *mac_address, IPAddress local_ip, IPAddress dns_server); |
| 24 | 26 |
void begin(uint8_t *mac_address, IPAddress local_ip, IPAddress dns_server, IPAddress gateway); |
| 25 | 27 |
void begin(uint8_t *mac_address, IPAddress local_ip, IPAddress dns_server, IPAddress gateway, IPAddress subnet); |
| 28 |
+ void maintain(); |
|
| 26 | 29 |
|
| 27 | 30 |
IPAddress localIP(); |
| 28 | 31 |
IPAddress subnetMask(); |
Tip: You can add notes to lines in a file. Hover to the left of a line to make a note
Requires a periodic call to the new method Ethernet.maintain()
http://code.google.com/p/arduino/issues/detail?id=716