sstaub / Ethernet3

Ethernet library for Arduino and Ethernetshield2 / WIZ550io / WIZ850io / USR-ES1 with Wiznet W5500 chip
Other
76 stars 34 forks source link

Allow caller to provide DHCP instance to EthernetClass #40

Closed jamessynge closed 3 years ago

jamessynge commented 3 years ago

The existing code uses operator new to create a DHCP instance if EthernetClass::begin is called without specifying an IP address for the device. This PR adds a setDhcp method so that the caller can provide a statically allocated DHCP instance, thus avoiding all the issues that come with dynamically allocating memory on devices with as little as 2KB of RAM.

jamessynge commented 3 years ago

I can remove the .gitignore modifications from this PR, though they're useful for me.

sstaub commented 3 years ago

you should remove .gitignore

jamessynge commented 3 years ago

I edited via the github UI (while in the waiting room at my docs), so produced 3 commits to edit the .gitignore file. Sigh. I recommend that you "squash and merge" when merging this PR.

sstaub commented 3 years ago

I don't see any sense of this PR. DHCP is needed if there is no static IP. It seems that it is not backward compatible.

jamessynge commented 3 years ago

Agreed, DHCP is needed. What is problematic is dynamic memory allocation (i.e. _dhcp = new DhcpClass();). In my sketches I prefer that the only variable memory consumption is the stack. That helps to avoid problems, including memory leaks. With the existing code, if the program were to call Ethernet.begin() twice, it will leak a DhcpClass instance.

With this change the caller can do the following:

// Allocate the DHCP instance:
static DhcpClass dhcp;

void setup() {
  Ethernet.setDhcp(&dhcp);
  Ethernet.begin(...);  // No memory allocation.
}

There is no impact to existing users from this change because if they don't call setDhcp then a new instance of DhcpClass will be allocated when they call begin, just as before... except if they call it twice, in which case they'll reuse the initially allocated instance (i.e. they won't leak memory).

jamessynge commented 3 years ago

Stefan, have you had a chance to consider my argument in favor of this change?

sstaub commented 3 years ago

I will think about when doing the next development cycle (support for W6100). I think that the allocation of the heap is not the problem you might consider, The problem is the defragmentation of the heap wich is caused from extensive use of the String library.

jamessynge commented 3 years ago

No problem, I'll just keep using my fork. I was already tempted to redesign the API away from the approach used by the official Arduino library.