firmata / arduino

Firmata firmware for Arduino
GNU Lesser General Public License v2.1
1.55k stars 518 forks source link

no version without query with Ethernet Firmata #280

Open jnsbyr opened 8 years ago

jnsbyr commented 8 years ago

Firmata.begin pushes the protocol and firmware version on the Stream regardless of its connected state. With USB powered devices using a serial host link this is no problem because booting Firmata and registering the USB driver on the host takes approximately the same time so the serial host typically receives the protocol and firmware versions without asking. With StandardFirmataEthernet this data gets lost because there is no connection when the data is written and you need to query the device explicitly for the versions.

At least with EthernetClient this can easily be amended by a simple change in the ino file by waiting for the initial connect:

void setup()
{
  ...
  // start up Network Firmata:
  while (!client.connected()) { // wait for connect to avoid loosing initial messages
    stream.flush(); 
  };  
  Firmata.begin(stream);
  systemResetCallback();  // reset to default config
}

This is a solution as long as the Firmata firmware has no custom code that needs to run regardless of the host connect.

Another way to do this would be a new "hostConnectedCallback" that could handle this regardless of the host connection type.

But with TCP Ethernet connections there is another aspect to consider: TCP connection can fail and can be reestablished completely independent of the Firmata device state. Some hosts would like to know if the Firmata device has been restarted or if the TCP connection has been reestablished. For this one could skip the automatic version transmit on reconnect or send a new telegram telling the host that this was just a reconnect.

soundanalogous commented 8 years ago

The reason there is typically no issue with USB is that when a client connects via serial, the board (unless it's an ATmega32u4 based board) hard resets.

A client can send a version query at any time so I'm not sure that waiting to transmit the version until a non-serial transport is established is all that important. However the ability to know if a client disconnected and then reconnected would be useful and has been requested several times in the past.

A "hostConnectedCallback" as you suggest sounds like an interesting approach. Do you want to draft something along those lines?

soundanalogous commented 8 years ago

Keep in mind there are now also WiFi and BLE transports for Firmata as well.

jnsbyr commented 8 years ago

Thanks for your feedback. As far as I can see most Arduino communication libraries do not notify so a hostConnectedCallback can only be done by polling connected for a state change. I will have a look at the BLE API because I haven't used it so far before making more detailed suggestions.

soundanalogous commented 8 years ago

At some point we'll be able to leverage the availableForWrite method once it's added to Ethernet (and hopefully other Stream client classes).

Also, for each transport there is a Stream wrapper for Firmata. In these wrapper classes we have the ability to check connection status (to the best that any transport class permits) as well as provide a callback to the user sketch if necessary.

jnsbyr commented 8 years ago

I have already seen this, e.g. in EthernetClientStream::maintain() a hostConnectedCallback could be added after DEBUG_PRINTLN("connected");. With WiFiStream and WiFi101Steam connect_client() could be modified appropriately.

soundanalogous commented 8 years ago

Tricky part is knowing when a disconnect event occurs...

jnsbyr commented 8 years ago

With EthernetClientStream the changes were more or less trivial. It is possible to signal connect and disconnect with a "hostConnectionCallbackFunction". To use this concept in all wrapper classes one could use inheritance by defining a single base class for all wrapper classes derived from Stream and implementing the attach method, the function pointer variable and possibly some more methods and variables that are used in all wrappers.

Please make a suggestion how we should proceed. Maybe a separate branch will help until the integration can be completed for Serial, Ethernet, WiFi and BLE. Maybe there is a chance to reduce the variants this way.

What I am missing is a WiFi version using a TCP client, that is needed for the FHEM project. Apart from the WiFi setup one can use the EthernetClientStream as a wrapper. I have this up and running and it reconnects within 5 seconds.

There is another point: What to do when the callback notifies a "connected" event? The default is to do nothing, but sending a special telegram to the host "hello, I just < booted | reconnected > " would be nice but that requires defining an appropriate Firmata protocol extension.

jnsbyr commented 8 years ago

Here is an example what a hostConnectionCallback can look like:

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

  switch (state) {
    case HOST_CONNECTION_CONNECTED:
      if (firstConnect) {
        // booted
        Firmata.printVersion();
        Firmata.printFirmwareVersion();
        firstConnect = false;

      } else {
        // reconnected
        Firmata.sendString("reconnected"); // @TODO maybe replace with specific telegram (Firmata procol extension)?

        // report state of all configured digital inputs
        for (port = 0; port < TOTAL_PORTS; port++) {
          portConfigDigital = 0;
          for (pin = 0; pin < 8; pin++) {
            pinMode = Firmata.getPinMode(8*port + pin);
            if (pinMode == INPUT || pinMode == INPUT_PULLUP) {
              portConfigDigital |= (1 << (pin & 7));
            }
          }
          if (portConfigDigital) outputPort(port, readPort(port, portConfigDigital), true);
        }

        // report state of all configured analog inputs
        for (pin = 0; pin < TOTAL_PINS; pin++) {
          pinMode = Firmata.getPinMode(pin);
          if (pinMode == PIN_MODE_ANALOG) {
            analogPin = PIN_TO_ANALOG(pin);
            if (analogInputsToReport & (1 << analogPin)) {
              Firmata.sendAnalog(analogPin, analogRead(analogPin));
            }
          }
        }
      }
      break;

    case HOST_CONNECTION_DISCONNECTED:
      break;
  }
}
soundanalogous commented 8 years ago

I'd rather create a new Firmata state management scheme for the protocol than send a string. Some other functions could be a ping and maybe it reports uptime.

Also regarding reporting the pin states, I think this would also be better as an extension to the protocol - reportAnalogInput, reportDigitalInput and the user can choose to call them upon receiving the reconnect event. Those calls would also be beneficial for users who choose to skip the configuration query (which is somewhat common - especially for BLE use cases).

soundanalogous commented 8 years ago

I need someone to contribute a client version of Wi-Fi and a server version of Ethernet. The user should be able to define whether to use client or server in the config file.

soundanalogous commented 8 years ago

I think it's clear that the following are needed:

Lets get proposals for firmata/protocol

soundanalogous commented 8 years ago

A solution here will also finally satisfy this request: https://github.com/firmata/protocol/issues/4

rwaldron commented 8 years ago

Busy day ;)

jnsbyr commented 8 years ago

Regarding state management: If you consider adding a ping type message one should think of its use as a connection watchdog, especially if the other side knows about the agreed period.

Regarding pin states: Looking back at the roots of the Firmata protocol a MIDI device was able to send its notes to the host or vice versa - and notes are more like outputs than inputs. Looking at the current Firmata protocol specifications sending the digital output back to the host is already covered. But to report the analog outputs (e.g. PWM) back to the host is currently not possible because the PWM outputs are typically addressed by their absolute pin number using PIN_TO_PWM while analog inputs are reported by relative pin number.

Why report outputs to the host? With a USB connection a Firmata device is more or less strictly controlled by the host (power, reset). But with Ethernet, WiFi and BLE you could add custom code claiming part of the available pins, directly modifying outputs or creating virtual inputs (e.g. counter values from digital inputs) and using Firmata as a transport protocol to send state and measurement data to the host when needed. This opens the door to a Smart Firmata extension, but I am not sure if this can be done in one step together with the things we have already discussed.

Please let me know how I can contribute to the protocol proposal. Opening separate issues for the the connection messages and the pin states will be probably help keeping focus.

Regarding the WiFi client: I will contribute a modified version of the StandardFirmataWiFi using the esp branch after #278 has been merged.

soundanalogous commented 8 years ago

Lets start a discussion around a protocol definition for Firmata connection state management here: https://github.com/firmata/protocol/issues/4.

Regarding pin states, the analog (including PWM value) or digital output state can currently be reported using the Pin State Query. However the pin state query is inefficient if the user wants to report all pins because they would need to send a separate request for each pin. An extension (drafted below) could add a report all pin states mechanism that would report all of the output states in a single loop board side.

There is currently no command for querying the input value (forcing the board to report current input values). This command would do exactly what your example code does - iterate over the analog and digital input pins and report their value. I understand why this is useful for non-serial transports but I question whether or not this should just happen upon reestablishment of a connection (and they will be reported regardless on the next iteration of loop - digital input or reporting loop - analog input) or we should enable the user to decide if they want the values reported (this is more important for BLE since the data packets are so small and must be sent at intervals > 7ms).

So this could add a new input reporting command and either extend the existing pin state query to report all or all analog or all digital in one shot.

// extension of Pin State Query to enable reporting all pins instead of
// ony individual pins.
// not sure this is a good idea, but it's one options for output state reporting
0  START_SYSEX        (0xF0)
1  pin state query    (0x6D)
2  pin                (0-127) // ignore if command length > 4
3  report all         0 = all pins, 1 = all digital, 2 = all analog
4  END_SYSEX

It may be cleaner to add a new query instead that can cover all cases:

0  START_SYSEX        (0xF0)
1  PIN_VALUE_QUERY    (TBD)
2  type                0 = all input, 1 = digital input, 2 = analog input,
                       3 = all output, 4 = digital output, 5 = analog output / pwm
3  END_SYSEX

Input values would be reported as regular input events. Output values would be reported using the pin state response internally.

jnsbyr commented 8 years ago

... whether or not this should just happen upon reestablishment of a connection ...

If a TCP connection gets lost the duration is undetermined and in this time inputs (and if you allow for custom code also outputs) can change. Digital inputs are only reported on change so this would stay hidden to the host. The fastest way to come back into sync for differential reporting is report immediately when the connection is reestablished. This avoids reporting new changes before the host gets updated on changes that already happened. I agree that the sync can be optional on the Firmata side if it can also be requested by the host or it could be limited to digital inputs.

I like your 2nd proposal where type has more options. Maybe you could add another option for "all inputs and outputs". This would save on round trips if you need everything anyway.

soundanalogous commented 8 years ago

Added an initial proposal for pin value query here: https://github.com/firmata/protocol/issues/59

soundanalogous commented 8 years ago

Lets continue the connection state management discussion here: https://github.com/firmata/protocol/issues/4#issuecomment-205046698