Packet overflow on WiFi network / fix

[ttww]

I am implementing an Network layer for the firmata4j project.
Everything is implemented and works "well" except that the current
transport layer sends every byte as a single WiFi packet over the air...
This leads to an unreliable connection because of packet overflows somewhere...

I have forked the ConfigurableFirmata project also and added some patches which collects the
bytes and flush it after an END_SYSEX, buffer full or end of reporting.

With this changes everything is fast and nice, but I don't oversee the side-effects to the normal serial communication and the procedure to update the other places (like in Arduino IDE packet manager...)

Can someone help me and do a review with the changes in https://github.com/ttww/ConfigurableFirmata/tree/Packet-Buffer?
I'm also unsure about the version number...
Thanks in advance

[soundanalogous]

One side effect is that this sort of buffering is already implemented in BLEStream, so this would result in double buffering for BLE. For this reason, it may be better to implement buffered write in WiFiStream.

[soundanalogous]

Also FirmataReporting is optional, a user may configure a sketch that does not include analog input or i2c or other features that use reporting and therefore FirmataReporting will not be included.

As for how to ensure the buffer is flushed appropriately, one option is to add a poll method to WiFiStream in a similar manner to how it is used in BLEStream. Also note the use of the timer in the BLEStream implementation. Something like this may be required to ensure that digitalWrite data if flushed in a timely manner since digital messages don't use Sysex and would otherwise only be sent when the buffer if full, which could lead to noticeable latency.

[soundanalogous]

And a configureable flush interval helps the user fine tune according to an amount of latency that is acceptable to the user: https://github.com/firmata/ConfigurableFirmata/blob/master/src/utility/BLEStream.h#L217.

[ttww]

Thanks for your answer.

The buffering is completely independent from FirmataReporting.
BLEStream is the wrong layer and an overkill for this problem (count the lines), it introduce more unclear timing situations (my point of view :-)).
Maybe putting a Firmata.flush() in the main loop is a more general and simple solution (and we didn't need the FirmataReporting.done() or SYSEX handling anymore).
I fully agree with you that sending when the buffer is full is not an option, it's a sign that something went wrong :-)
What's about removing the buffering from BLEStream and doing there only the Bluetooth things?
This would leave an out of the box buffering solution for all transport channels (serial, bluetooth, WiFi).
Putting the buffering inside WiFiStream is not a good solution. The problem is in the layer above and it would WiFiStream left to guess when to send. The buffering in ConfigurableFirmata is a benefit for all transport layers.
Please don't misunderstand me, I try to find a general out of the box solution without the need for special Streams :-)

[soundanalogous]

I agree, my thought for adding buffering to WiFiStream was a short term solution. It should be done at the higher layer though. It's just going to be a much bigger project. All of the transport streams were written by different authors so it's probably a good time to refactor for consistency.

[soundanalogous]

There is a complication here however, writes on the Serial transport are already buffered in the HardwareSerial implementation. However they are not in the WiFi and Ethernet implementations as you pointed out. This is just for AVR, I'd have to look at how the other Arduino-compatible architectures handle write as well.

[ttww]

Additionally, the Serial*-Buffers can't be reused right after write() call (don't know if the copy the data).
Would it make sense to make a pull request with the current state?
How should we go on?

[soundanalogous]

Serial.write returns immediately without transmitting any data because transmission is actually handled by an ISR, at least on some MCUs. Serial.flush waits until the TX buffer is eventually emptied by the ISR so Serial.write followed by Serial.flush is blocking while Serial.write alone is not.

As far as how to proceed, you can submit a PR but there is a lot of work remaining. First remove the changes to FirmataReporting.

[soundanalogous]

TODO:

• Refactor BLEStream to remove TX buffering
• Determine if Serial data should be buffered again or if the Serial transport should be an exception
• Ensure the buffering scheme works with Ethernet as well as WiFi
• Identify a scheme to ensure the TX buffer is flushed in a timely manner
• Determine if the TX buffer adds latency to digital write messages and the implications of this (especially when using the Serial transport)

[soundanalogous]

The more I think about this, the more I think it actually may be better to handle this in the individual transport streams. Another reason is the TX buffer size varies by transport, even with Serial, the buffer may be as small as 16 or as large as 256 bytes. The BLE TX buffer is only 20 bytes for the Arduino 101 and for the general purpose BLEPeripheral library.

Otherwise ConfigurableFirmata would need to be littered with architecture dependent #ifdefs. I'd rather push this complexity out to the individual stream transport wrappers.

One thing we could try is combining the WiFi and Ethernet stream wrappers into a single NetworkStream wrapper, that would at least remove some duplication.

I'm open to other ideas as well.

[soundanalogous]

@zfields I'm curious if you have any thoughts on a good approach here. The issue is that Arduino buffers serial/UART data in the various architecture-dependent HW serial implementations (you simply use stream.write(someData) and data is buffered in an ISR behind the scenes). However there is no such buffering behind the scenes when using a network (WiFi or Ethernet) stream, data is sent one byte at a time based on the way writes are currently implemented (example from WiFiClient implementation in WiFi101.cpp). Additionally, various streams have different TX buffer sizes. BLE for example uses a 20 byte buffer, serial can range from 16 to 256 bytes depending on the board architecture (but 64 bytes is probably the most common). A network TX buffer could be fixed (say 64 bytes), but perhaps give the user the ability to increase up to 256 bytes using a define.

I'm leaning towards adding buffering to the Firmata WiFi and Ethernet stream wrappers. A more ideal solution would be to combine these 2 stream wrappers into a single NetworkStream class. I'm curious if you have any other ideas. Ideally, as @ttww points out this could be handled at a lower level (a single buffering scheme to cover all streams), but due to the different buffer sizes and the Serial issue, I'm not exactly sure how to proceed in that direction, thus my suggestion to handle this in the network stream separately for now.

[ttww]

Hi,
finally, I have done some more changes which are not directly correlated to this packet buffering context.
Please tell me if you want to discuss this on a different place.

• I’ve added a new PIN_MODE named PIN_COUNTER. This is for defining pins as an IRQ source and you can define for each pin the signal type (RISING, FALLING, CHANGE...) and the integration time in ms. This allows IRQ counting of fork sensors signals to do measure rotation speeds. The values are currently transferred as PIN-Events after each integration interval.
• I had to make some changes to the IOEvent class to be a little more flexible (eg. for using it for error messages and new functions in the future). I'm a little bit unhappy with the current listener and event model, but I don't want to break things.
• Also, some changes to get informed about connect/reconnects where needed and some changes to trigger a new initialization/capabilities request. How was this done in the BLX implementation? There should be the same problem.
• I tried to separate my changes in a package, but this was not entirely possible (eg. constants in FirmataToken and adding Pin.setCounterMode()). The current architecture is not designed to be extensible (new HiLevel functionality). But if you integrate such things on the device-side, we should have at least an easy way to pass events with the data to the application layer.
• I've done some conservative rearrangement with packages (examples).
• I need to improve the documentation...

With all those changes you can completely control a small driving roboter with the Firmata protocol and develop on the host :-)

[soundanalogous]

I think that sounds like it would be a few separate pull requests. You should also look at the newer architecture being implemented in the firmata/arduino repo (note in particular the new classes FirmataParser, FirmataMarshaller, FirmataConstants, FirmataDefines). These changes will be migrated over to firmata/ConfigurableFirmata at some point.

[soundanalogous]

@ttww I've looked over your latest changes. You have several different changes that would need to be broken up as follows:

1. Proposal for a way to inquire about which pins on a board support interrupts.
2. Proposal to add a new PIN_COUNTER feature (the place to start here is firmata/protocol and submit a PR to add a new markdown file with your proposal.
3. Proposal to add a heartbeat feature.
4. Proposal to add buffering when using a network transport (the original request in this thread).

1 & 2 are related and could be submitted together, but I'd prefer if 3 and 4 were separate changes (and separate threads).

The other thing to consider is if there was a more general way to access pin change interrupts through firmata if you'd still be able to accomplish the counter part on the client side, or if the timing is too intricate and the only way to do it is on the firmware side.