Skip to content
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

consider adding status() in EthernetClient #52

Closed
JAndrassy opened this issue Sep 25, 2023 · 16 comments
Closed

consider adding status() in EthernetClient #52

JAndrassy opened this issue Sep 25, 2023 · 16 comments
Labels
enhancement New feature or request

Comments

@JAndrassy
Copy link

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

I just filed this issue for corrections, changes, and additions: Networking-for-Arduino/Arduino-Networking-API#1

Thanks again for this work!

@ssilverman
Copy link
Owner

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
Copy link
Author

JAndrassy commented Sep 25, 2023

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 ssilverman added the enhancement New feature or request label Sep 25, 2023
@ssilverman
Copy link
Owner

@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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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

@ssilverman
Copy link
Owner

Added in ba0736d.

@JAndrassy
Copy link
Author

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
Copy link
Owner

ssilverman commented Oct 3, 2023

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

@JAndrassy
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants