arduino-libraries / Ethernet

Ethernet Library for Arduino
http://arduino.cc/
260 stars 264 forks source link

Add `EthernetServer() {}` constructor #191

Open sz-coder opened 2 years ago

sz-coder commented 2 years ago

Looking at the source code, only EthernetServer(uint16_t port) : _port(port) { } is supported.

I don't see why it's needed to be specific about the port when constructing an object from EthernetServer.

This behaviour, however, makes it impossible to pool and re-use EthernetServer objects since the port must be known when constructing the object.

JAndrassy commented 2 years ago

duplicate of https://github.com/arduino-libraries/Ethernet/pull/39

per1234 commented 2 years ago

Thanks for pointing out #39 @jandrassy. Since that is a PR, we can keep this issue open. I have linked the PR to this issue so that if it is merged the issue will automatically be closed.

Even though we don't have a policy that every PR must address an issue (meaning the PR author must create an issue before the PR if there is not an existing one) as some other projects do, I do see some merit in that approach. That merit is outweighed by the burden such a policy places on the contributor IMO, but in cases where the issue is created independently there is no harm in having an issue to track the defect or enhancement request, with the PR serving as one proposal for resolving it.

JAndrassy commented 2 years ago

Per, while the PR is not closed, comments suggest it will not be merged and there is a workaround too.

sz-coder commented 2 years ago

Per, while the PR is not closed, comments suggest it will not be merged and there is a workaround too.

Why will this PR not be merged? It doesn't look like a breaking API change.

sz-coder commented 2 years ago

and there is a workaround too.

And what is that workaround?

JAndrassy commented 2 years ago

and there is a workaround too.

And what is that workaround?

server = EthernetServer(port); // change the port

sz-coder commented 2 years ago

and there is a workaround too.

And what is that workaround?

server = EthernetServer(port); // change the port

Not a feasible workaround for my purposes; I can't call the constructor twice.

JAndrassy commented 2 years ago

and there is a workaround too.

And what is that workaround?

server = EthernetServer(port); // change the port

Not a feasible workaround for my purposes; I can't call the constructor twice.

why not? it doesn't do anything only sets the port

sz-coder commented 2 years ago

Because I'm doing something similar to this:

    // pool of EthernetUDP instances
    static EthernetUDP _udp_pool[5];

    // ...

    EthernetUDP *getNextFromPool(int port) {
        int free_element = _getNextFreeIndex();

        if (0 > free_element) return NULL;

        EthernetUDP *i = &_udp_pool[free_element];

        // call i->end(); if was in use before

        i->begin(port);

        return i;
    }

This works fine for EthernetUDP since it has a begin() method we can pass the port to.

However this approach does not work with EthernetServer.

How exactly am I going to call a constructor for a global object again?

sz-coder commented 2 years ago

I want to make clear I want to reuse and not re-create instances of EthernetServer.

JAndrassy commented 2 years ago

I want to make clear I want to reuse and not re-create instances of EthernetServer.

how reuse? you have to call end() and begin() if you change the port. the server = EthernetServer(port); creates an object and copies its contents (port) into the global object. btw there is no end() so the chip will still listen on the port used with the previous begin()

sz-coder commented 2 years ago

how reuse?

Re-use in the sense that I don't have to create a new object instance to start listening and/or change the listening port.

I already gave an example with EthernetUDP: with this class, it's possible to just call end() and then begin() to start listening on a new port.

I'm aware that the instance itself isn't holding any important information and acts like an access wrapper to the W5x00 Chip. I get that.

C++ is a complex language and I don't wanna play around with things that are "weird" - if you know what I mean - just because something "works" doesn't mean it's correct. This is especially true for languages like C and C++.

I sincerely hope you'll implement this, otherwise I see myself forced to use my own implementation which would be a shame tbh.