WiFiClientStream added

[jnsbyr]

Changes

• server related functions moved from WiFiStream into WiFiServerStream
• unified class interface for sketch file (no client/server defines)
• WiFi and TCP configuration and connect split into individual methods (TCP constructor, static IP config, WiFi begin and TCP maintain)
• WiFi only initialized once at startup (WiFi connections cannot be forced by repeatedly calling begin)
• convenience methods for combined WiFi/network initialization removed

Tests

• tested with Huzzah ESP8266 in client mode
• successfully builds for AVR architecture with legacy WiFi option

Todo

• decide if inheritance from new WiFiStream is preferable to copied code in two classes in regard to code efficiency, code size, platform compatibility and maintenance
• decide if a single client/server class would be preferable to separate classes in regard to code efficiency, code size and maintenance

The approach used in this PR could be applied to EthernetClientStream to provide the missing EthernetServerStream. Basically its the same without the WiFi parts.

[soundanalogous]

This is also reported in printWiFiStatus(). Do we really need it in both places?

[soundanalogous]

This is also misleading currently as I get the failed message if I am not fast enough in starting up my host application. Maybe the while loop should continue until a connection is made and/or use a much longer timeout). Previously the while loop only waited until a connection was made with the router.

[jnsbyr]

I didn't have a looked at the debug output so far.

Waiting until connected is a drawback if you add active logic that runs independently of the host. That is one reason why I wanted to introduce a hostConnectedCallback.

Regarding the timeout, I reduced it by about 30% because an ESP8266 with static IP typically connects in less than 1.5 seconds. Raising the timeout should help somewhat.

[jnsbyr]

#define MAX_CONN_ATTEMPTS           20  // [500 ms] -> 10 s

should be long enough for a WiFi connect + DHCP + TCP connect in most cases, but I know there are situations where it needs more time, especially if the signal strength is weak. I am still not sure if this is reason enough to use a higher timeout. An easy way to cope with different requirements would be to move the timeout value into the config file and assign a default.

[soundanalogous]

This looks awesome! You've been busy :)
Will test with WiFi 101 shield and MKR1000 and also ESP8266 in server mode.

[soundanalogous]

which compiler needs these?

[jnsbyr]

The Xtensa Compiler for the ESP8266 can't do without forward declarations if you want to call a function before it is defined. I havn't checked the dependencies. If there are no circular references reordering the functions can avoid this.

[jnsbyr]

After reordering the functions in the sketch the forward definitions can be removed. But there is also the question of context, of keeping certain functions in a textual proximity or order to each other.

[soundanalogous]

Strange I have not run into any compiler errors when using the ESP8266 Arduino core and I have successfully compiled each module that library supports (except ThaiEasyElec's ESPino which fails). At least not as of version 2.1.0.

[jnsbyr]

While compiling I see the version 1.20.0-26 for the Xtensa and a lot of compiler options that are handled by the Arduino IDE. Maybe one of them makes the difference (e.g. for Firmata.cpp):

-U__STRICT_ANSI__ -c  -Os -g -mlongcalls -mtext-section-literals -fno-exceptions -fno-rtti -falign-functions=4 -std=c++11 -MMD -ffunction-sections -fdata-sections -DF_CPU=80000000L   -DARDUINO=10607 -DARDUINO_ESP8266_ESP12 -DARDUINO_ARCH_ESP8266  -DESP8266

[soundanalogous]

I see, you are compiling via the command line rather than using the Arduino IDE.

[jnsbyr]

No, I just changed to verbosity via IDE settings to see what is really happening.

[soundanalogous]

WiFi client works for ESP8266, MKR1000 and WiFi 101 shield with Arduino Zero. I see you made a change however in that at startup it now waits for a TCP connection instead of waiting for the router connection. This makes sense, but I wonder if instead of having a max attempts, it should just loop until a connection is made and the debug messages are updated accordingly. Otherwise it appears to the user that the connection has failed ("failed to establish TCP connection").

Server mode does not work. The connection exceeds max attempts, however an IP address is assigned but I cannot establish a connection with the TCP client application.

[soundanalogous]

add: DEBUG_PRINT("."); after the delay as a progress indicator

[soundanalogous]

Tried digging into WiFi server issues. No luck. I don't get any errors so I'm not sure where the connection is failing.

[soundanalogous]

Working backwards from the old WiFiStream to something closer to what you have, but without inheriting from WiFiStream yet, the following works for the server implementation. The differences are subtle from what you have (minus the subclassing). I added _server = WiFiServer(_port); to the maintain method.

#ifndef WIFI_SERVER_STREAM_H
#define WIFI_SERVER_STREAM_H

#include <inttypes.h>
#include <Stream.h>

class WiFiServerStream : public Stream
{
private:
  WiFiServer _server = WiFiServer(23);
  WiFiClient _client;

  //configuration members
  IPAddress _local_ip;
  IPAddress _gateway;
  IPAddress _subnet;
  uint16_t _port = 0;
  uint8_t _key_idx = 0;               //WEP
  const char *_key = nullptr;         //WEP
  const char *_passphrase = nullptr;  //WPA
  char *_ssid = nullptr;
  bool _listening = false;

  inline bool connect_client()
  {
    if(_client && _client.connected()) return true;

    WiFiClient newClient = _server.available();
    if( !newClient ) return false;
    _client = newClient;

    return true;
  }

public:
  WiFiServerStream(uint16_t server_port) : _port(server_port) {}

#ifndef ESP8266
  // allows another way to configure a static IP before begin is called
  inline void config(IPAddress local_ip)
  {
    _local_ip = local_ip;
    WiFi.config( local_ip );
  }
#endif

  // allows another way to configure a static IP before begin is called
  inline void config(IPAddress local_ip, IPAddress gateway, IPAddress subnet)
  {
    _local_ip = local_ip;
    _gateway = gateway;
    _subnet = subnet;
#ifndef ESP8266
    WiFi.config( local_ip, IPAddress(0, 0, 0, 0), gateway, subnet );
#else
    WiFi.config( local_ip, gateway, subnet );
#endif
  }

  // get DCHP IP
  inline IPAddress getLocalIP()
  {
    return WiFi.localIP();
  }

  /**
   * @return true if connected
   */
  inline bool maintain()
  {
    if( connect_client() ) return true;

    stop();

    if( !_listening && WiFi.status() == WL_CONNECTED )
    {
      // start TCP server after first WiFi connect
      _server = WiFiServer(_port);
      _server.begin();
      _listening = true;
    }

    return false;
  }

/******************************************************************************
 *           Connection functions with DHCP
 ******************************************************************************/

  //OPEN networks
  inline int begin(char *ssid)
  {
    _ssid = ssid;

    WiFi.begin(ssid);
    return WiFi.status();
  }

#ifndef ESP8266
  //WEP-encrypted networks
  inline int begin(char *ssid, uint8_t key_idx, const char *key)
  {
    _ssid = ssid;
    _key_idx = key_idx;
    _key = key;

    WiFi.begin(ssid, key_idx, key);
    return WiFi.status();
  }
#endif

  //WPA-encrypted networks
  inline int begin(char *ssid, const char *passphrase)
  {
    _ssid = ssid;
    _passphrase = passphrase;

    WiFi.begin(ssid, passphrase);
    return WiFi.status();
  }

/******************************************************************************
 *           Connection functions without DHCP
 ******************************************************************************/

// ESP8266 requires gateway and subnet so the following functions are not compatible
#ifndef ESP8266
  //OPEN networks with static IP
  inline int begin(char *ssid, IPAddress local_ip)
  {
    _ssid = ssid;
    _local_ip = local_ip;

    WiFi.config( local_ip );
    WiFi.begin(ssid);
    return WiFi.status();
  }

  //WEP-encrypted networks with static IP
  inline int begin(char *ssid, IPAddress local_ip, uint8_t key_idx, const char *key)
  {
    _ssid = ssid;
    _local_ip = local_ip;
    _key_idx = key_idx;
    _key = key;

    WiFi.config( local_ip );
    WiFi.begin(ssid, key_idx, key);
    return WiFi.status();
  }

  //WPA-encrypted networks with static IP
  inline int begin(char *ssid, IPAddress local_ip, const char *passphrase)
  {
    _ssid = ssid;
    _local_ip = local_ip;
    _passphrase = passphrase;

    WiFi.config( local_ip );
    WiFi.begin(ssid, passphrase);
    return WiFi.status();
  }
#endif

/******************************************************************************
 *             Stream implementations
 ******************************************************************************/

  inline int available()
  {
    return connect_client() ? _client.available() : 0;
  }

  inline void flush()
  {
    if( _client ) _client.flush();
  }

  inline int peek()
  {
    return connect_client() ? _client.peek(): 0;
  }

  inline int read()
  {
    return connect_client() ? _client.read() : -1;
  }

  inline void stop()
  {
    _client.stop();
  }

  inline size_t write(uint8_t byte)
  {
    if( connect_client() ) _client.write( byte );
  }
};

#endif //WIFI_SERVER_STREAM_H

[jnsbyr]

Interesting. I would have prefered to do the sever tests myself but didn't find the time to setup something like you already suggested so far.

I wondered how the WiFi connection is triggered in server mode because there is no activity that initiates outgoing connections. Reopening the server port again and again will probably do the same as the client connection. This is the approach of the original WiFiStream. I will put the WiFiServer(_port) into maintain.

[jnsbyr]

Had a closer look at what happens if you place WiFiServer(_port) into maintain(). This is probably the TCP bind of the server port to the WiFi interface. If one does this already in the constructor, there is probably no interface (or at least no IP) to bind to. When waiting for the WiFi connect in maintain() at least the interface exists. When using DHCP it depends on timing if the IP is assigned before the WiFiServer(_port) in maintain() or not but I am not sure if the IP assignment is a requirement, because default port binding does not need an IP address.

[soundanalogous]

This is the bug that is preventing WiFi server from working.
change !_client to !newClient, otherwise the client was never being set since we already know at this point that _client is null (or otherwise not assigned).

[jnsbyr]

Great, you found it. Thx for testing. Have pushed the fixed version.

[soundanalogous]

I've tested both client and server on all of my WiFi boards and shields and both are working well.

However this block doesn't make sense when using as a TCP Server since a client application could connect at any time, even hours or days later vs running the board as a TCP client where you expect server application to already be running on the host on startup:

  while (!streamConnected && ++connectionAttempts <= MAX_CONN_ATTEMPTS) {
    delay(500);
    DEBUG_PRINT(".");
    streamConnected = stream.maintain();
  }
  if (streamConnected) {  
    DEBUG_PRINTLN( "TCP connection established" );
  } else {
    DEBUG_PRINTLN( "failed to establish TCP connection" );    
  }

I think we may need to do something like this since when running as a TCP server we really want to know if the server is ready rather than if a client happens to connect within the first 10 seconds:

#ifdef SERVER_IP
  while (WiFi.status() != WL_CONNECTED && ++connectionAttempts <= MAX_CONN_ATTEMPTS) {
    delay(500);
    DEBUG_PRINT(".");
  }
#else
  while (!streamConnected && ++connectionAttempts <= MAX_CONN_ATTEMPTS) {
    delay(500);
    DEBUG_PRINT(".");
    streamConnected = stream.maintain();
  }
  if (streamConnected) {  
    DEBUG_PRINTLN( "TCP connection established" );
  } else {
    DEBUG_PRINTLN( "failed to establish TCP connection" );    
  }
#endif

[soundanalogous]

correction (erroneously had #ifdef SERVER_IP previously):

#ifndef SERVER_IP
  while (WiFi.status() != WL_CONNECTED && ++connectionAttempts <= MAX_CONN_ATTEMPTS) {
    delay(500);
    DEBUG_PRINT(".");
  }
#else
  while (!streamConnected && ++connectionAttempts <= MAX_CONN_ATTEMPTS) {
    delay(500);
    DEBUG_PRINT(".");
    streamConnected = stream.maintain();
  }
  if (streamConnected) {  
    DEBUG_PRINTLN( "TCP connection established" );
  } else {
    DEBUG_PRINTLN( "failed to establish TCP connection" );    
  }
#endif

Also, the WiFi101 library seems to block on WiFi.begin until a connection state change is observed, but ESP8266 does not so the WiFi.status() loop above would be required. Otherwise I would suggest simply skipping the streamConnected loop for TCP server.

[jnsbyr]

What you suggest would work, but I think the right way to do it is to use the host connection callback.

One should not need to care if one uses a client or a sever. All that counts is that the TCP connection is established - and why wait at all? Just move most of the code from setup() after the stream.begin() call into the host connection callback.

[soundanalogous]

There is a lot of one time init code in setup that has nothing to do with the transport that wouldn't make sense to run a second time (at least not when the board is a TCP server). I was thinking the host connection callback controls whether or not the code in the loop function runs or is skipped entirely and the buffer is appropriately managed.

[jnsbyr]

The host connection callback notifies when the TCP link state changes between connected and not connected. Its up to the sketch what to do with these events. I suggest splitting the current setup() into at least 2 parts, one that initializes the board after boot in setup() as it does and one that needs to run after the initial host connect using the host connection callback.

One could easily add a third state to the host connection callback handling, so that you get connected, disconnected and reconnected. But maybe it is better to let the sketch decide on the application layer if the initial setup by the host was really completed so that the next connect is treated as a reconnect. If you use this flag in the connected event you can decide when to do a special setup with the 1st host connect.

I would typically not skip any logic processing in the main loop while not connected. The Firmata transmit buffers is something different though.

[soundanalogous]

I think the following should only be called once (if connect is a one time event - vs reconnect than that is okay as well).

• Firmata.setFirmwareVersion(...);
• All of the the pin ignore code
• Firmata.begin(stream);
• systemResetCallback(); // since you may not want to reinitialize when temporarily losing connection
• printWiFiStatus();

You'd have to block loop completely until the setup code runs.

It may make sense to attache and detach callbacks on each disconnect / reconnect. I'm not sure if this would cause any memory leaks or not.

The stream connection status could be reported on connect, disconnect and reconnect.

[soundanalogous]

You'd have to block loop completely until the setup code runs.

Or at least block Firmata.processInput if the callbacks are not attached because processInput is the code in Firmata.cpp that calls the callback functions.

update - maybe that doesn't matter actually since if no callback is registered there is no attempt to call it.

[soundanalogous]

Can you draft a version with the host connection callback or would you rather do that in a separate PR?

[jnsbyr]

It's hard for me to catch up right now :-)

I agree that the things you listed above should be one timers that should stay in setup(). But why do you worry about Firmata.processInput? The WiFiStream abstraction will return -1 while not connected and nothing will happen and when the connection event occurs the host connection callback will be the first to be called.

The callback is already included in the current WiFiStream. If you want to give it a try add stream.attach(hostConnectionCallback) somewhere before calling stream.begin() and add your callback that might start like that:

void hostConnectionCallback(byte state)
{
  byte port, portConfigDigital, pin, pinMode, analogPin;

  switch (state) {
    case HOST_CONNECTION_CONNECTED:
....
  }
}

The details of the host connection callback implementation in the sketch file are good for a separate PR. I leave it up to you if you want to initiate this PR yourself because for moving code in the sketch file there are things to consider that you are more aware of than me.

[soundanalogous]

Okay I will merge this into the main wifi-client branch since I think it's in a good state and then I'll work on adding the connection host callback to StandardFirmataWiFi in a separate PR.

[soundanalogous]

It's now clear to me what the issue is with deferred initialization after having a go at it. The problem is that until Firmata.begin(stream) is called, the FirmataStream pointer in Firmata.cpp remains uninitialized. This means when loop runs and any of the stream methods are called, there is no actual stream object to reference so the sketch will crash. Once the stream is initialized however, as you pointed out, the connect_client() call in each of the stream methods ensures the client buffer is neither read nor written to so it should be safe even if there is no easy way to defer Firmata setup until after the TCP connection is established. In this case the host connection callback is probably most useful for indicating state to the user (if DEBUG is enabled) and managing the buffer or flags that may be useful.

[soundanalogous]

The host connection callback is never called when using the ESP8266 as a TCP server even though the TCP connection is established. It is called however when using the WiFi101 library. When using the ESP8266 as a TCP client, host connection callback is only called for the CONNECTED state and only ever once (even though disconnect and reconnect actually works). It's called for both states when using WiFi101 as a TCP client and called each time the client disconnect and reconnects to the server.

[soundanalogous]

Here are the changes I made: #290

[jnsbyr]

The host connection callback is never called when using ...

I would not have expected the platforms to be that different. But there is probably a way to detect a connect and a disconnect on all platforms. Will have a look at the ESP8266 specifics but I am not sure if I can provide any answers until the weekend.

[soundanalogous]

No problem. I typically only have time to work on Firmata on the weekend as well.