ssilverman / QNEthernet

An lwIP-based Ethernet library for Teensy 4.1 and possibly some other platforms
GNU Affero General Public License v3.0
86 stars 24 forks source link

consider adding `status()` in EthernetClient #52

Closed JAndrassy closed 1 year ago

JAndrassy commented 1 year ago

Hello. I did a research on Arduino networking libraries and I noticed your EthernetClient class doesn't have a status() method. I think it is easy to implement it over LwIP.

https://github.com/JAndrassy/Arduino-Networking-API/blob/main/ArduinoNetAPILibs.md#client-getters-and-setters

btw: I maintain EthernetENC, the other library which doesn't have status() in EthernetClient. I plan to add it.

ssilverman commented 1 year ago

Thanks for doing this work. I think collected summaries like this are a good idea (that is, if they’re maintained).

Could you point me to a spec for EthernetClient::status()? This link doesn’t have the information.

I also noticed a few mistakes for my library. I’ll file some issues in your repo.

JAndrassy commented 1 year ago

other lwip based libraries return state from tcp_pcb from status. so it is enum tcp_state. the Ethernet library returns values for constants with same name (most of them)

ssilverman commented 1 year ago

I was doing some digging, and found these links:

  1. RFC9293 states
  2. Diagram
  3. w5100.h

The W5100 values are returned from that library’s client status() call. I noticed it has an extra INIT state and doesn’t have two FIN-WAIT states.

As I’m finishing this message, I noticed you responded; I haven’t read it yet.

ssilverman commented 1 year ago

other lwip based libraries return state from tcp_pcb from status. so it is enum tcp_state. the Ethernet library returns values for constants with same name (most of them)

Do you plan on returning an enum or a uint8_t? In my opinion, an enum would be best. Could you point me to the enum definition you’re going to use? Or are you just defining your own with the 11 TCP states, and differing from w5100.h’s definition? i.e. removing INIT and adding a second FIN-WAIT?

ssilverman commented 1 year ago

I just filed this issue for corrections, changes, and additions: https://github.com/JAndrassy/Arduino-Networking-API/issues/1

Thanks again for this work!

ssilverman commented 1 year ago

other lwip based libraries return state from tcp_pcb from status. so it is enum tcp_state. the Ethernet library returns values for constants with same name (most of them)

Do you plan on returning an enum or a uint8_t? In my opinion, an enum would be best. Could you point me to the enum definition you’re going to use? Or are you just defining your own with the 11 TCP states, and differing from w5100.h’s definition? i.e. removing INIT and adding a second FIN-WAIT?

Oh, wait... I just realized you were referring to lwIP's internal enum.

JAndrassy commented 1 year ago

all libraries have `uint8_t' as return value. I think the list of states from LwIP is the best choice. Many of the libraries are based on LwIP. I don't expect the actual returned values to be the same in all llibraries, but constants with the same name should exist, so

  if (client.status() == ESTABLISHED) {

works

ssilverman commented 1 year ago

@JAndrassy Do you find that EthernetClient::status() is used much in the wild? Do you use it in your own projects? I'm just wondering how popular it is or would be.

JAndrassy commented 1 year ago

I don't use it because I didn't know before the research that almost all libraries have it. status() tells the true state of the connection. connected() is still true if data are available for read. that is not good if you want to send data. With accept() the sketch can/should manage the client object so I imagine more detail about the connection may be useful in some advanced use.

I have this in a project.

int modbusConnection() {
  while (modbus.read() != -1); // clean the buffer
  if (!modbus.connected()) {
    modbus.stop();
    if (!modbus.connect(symoAddress, 502))
      return MODBUS_CONNECT_ERROR;
    modbus.setTimeout(2000);
    msg.print(F(" MB_reconnect"));
  }
  return 0;
}

status() would be better than to have to read all the data that arrived late after my read function timeout out.

ssilverman commented 1 year ago

I agree that it could be useful for more advanced uses, for sure.

One thing about the Arduino ecosystem, in my opinion, is that there's lots of cargo culting, where simple momentum keeps APIs the way they are.

JAndrassy commented 1 year ago

simple momentum keeps APIs the way they are.

it looks like the API doesn't have an 'owner' at Arduino. That is why I try to start a community driven 'standardization' with an RFC process. But first I try to just extend the API for useful functions which already exist in many of the libraries so there is already a consensus.

ssilverman commented 1 year ago

I had added that function, by the way, just haven't pushed yet... I appreciate the suggestion and your efforts.

ssilverman commented 1 year ago

Added in ba0736dc6e0bca6062c5c6a19267074bbd1063c9.

JAndrassy commented 1 year ago

If you wan to add more compatibility functions, setDNS and dnsIP(n) are a good candidates. Ethernet libraries have historically setDNSServerIP and dnsServerIP. WiFi libraries have setDNS with one and two parameters and dnsIP(n=0) was recently first used in official Arduino library (WiFiS3), but esp8266 and esp32 WiFi libraries had it long time.

I added setDNS a dnsIP(n) into my EthernetENC library and I will do a PR for the Ethernet library.

ssilverman commented 1 year ago

I have equivalent functions in the DNSClient class. I’m not too fond of putting everything in the Ethernet object.

JAndrassy commented 1 year ago

@ssilverman I added "Arduino networking API guide" to my https://github.com/JAndrassy/Arduino-Networking-API repository.