Diaoul / arduino-ESP8266

An Arduino library to manage the ESP8266.
MIT License
68 stars 46 forks source link

Implement the Stream interface #2

Closed lasselukkari closed 9 years ago

lasselukkari commented 9 years ago

The library is only a few functions short of implementing the Stream interface.

The only missing are: int peek(); void flush(); size_t write(uint8_t);

I can make the changes and create a pull request if you find this reasonable too.

Diaoul commented 9 years ago

For available and read the implementation was a bit tricky to get right and i'm still not sure this is correct. I think it's a good idea to implement the stream interface so go ahead I'll merge.

lasselukkari commented 9 years ago

They seem to be working fine but I have only tested them with single connection client mode. I haven't tested this with your code but problems usually come with these chips when you have multiple incoming connections. For example when the device is in server mode and you start sending data back after reading the incoming data. You are then trying to read the ACK (">") but because there is another connection coming in you start to read it insteand of the ACK.

There is a hardware level handshaking control also but the pins are not available in most versions sold currently.

Diaoul commented 9 years ago

Maybe then check for available before trying to search for ACK? It normally takes less than 20ms to get the ACK: https://github.com/Diaoul/arduino-ESP8266/blob/master/ESP8266.cpp#L481

lasselukkari commented 9 years ago

I tried that too with my own tests. It doesn't really help as it still can (and will) happen in the small timeframe between the availability check and the request to write. I have seen this many times when a browser creates multiple simultaneus connectios for example when it's trying to get favicon.ico and the index page at the same time. And you can't even disable the mutiple connections mode when running as server. As a workaroud I modified the current AT firmware to buffer the requests in the ESP8266. You will then have use a commmand AT+CIPREAD to check if there is any incoming data available.

lasselukkari commented 9 years ago

What do you think. If we implement the write function is it any good if we don't split the send function in two like my idea was before. You can't use the write with anything if you can't open the connection first without sending the data.

Diaoul commented 9 years ago

The firmware can surely benefit from a few patches like this one :) If we make a write function I'm OK to split the send function. That makes sense. However, I'm not sure if the write function should call the send function if necessary pretty much like the read function calls the available function.

I'm open to suggestions

lasselukkari commented 9 years ago

I think that would make it really slow because then it would have to wait for the "send start" with each char you are sending.

Diaoul commented 9 years ago

The alternative is to have a begin send function like you did but you need to know the number of bytes you want to send beforehand which isn't really friendly I think.

To be consistent we could rename the current send methods to write, have them call a send method and remove the id from the parameters and use setId.

lasselukkari commented 9 years ago

If we keep everything as it is now and just implement the write the user has the option to either use the current send that does everything in one patch or manually begin the send and then use the write function from there. I just committed these changes to my master. Take a look an tell what you think.

I modified https://github.com/lasselukkari/HttpClient to use Streams instead of Client and attached callbacks to establish and close the connection. It seems to be working perfectly with your ESP8266 library now.

lasselukkari commented 9 years ago

Another option could be that we use a buffer for the write and send it when the it gets full and then send any remaining data with the flush. This way the write could also take care of opening the connection.

lasselukkari commented 9 years ago

But this isn't actually what the flush should be doing. If you read the documentation the Stream flush should wait until the putput buffer is written completely. In this case should it actually be the function that waits for the write ACK?

Diaoul commented 9 years ago

So about write, I've searched the EthernetClient for implementation and they do just that: open SPI connection, send the byte, close SPI connection. That's 100% inefficient but semantically correct as it actually sends a byte through the network. Your implementation does not.

If you don't want problems with performance, you need to use the send method with a buffer, not byte by byte.

Opening the connection is not the job of write. Write writes if it can, or fail if it cannot.

lasselukkari commented 9 years ago

I tried it that way this morning. Doesn't work. The chip will start to give you "busy s..." errors after a few sent chars. And atleast for me many times slower trasfer speed would be an issue anyway.

Diaoul commented 9 years ago

Sending a buffer doesn't work? It should be as efficient as it can be. I've finished my Client implementation and it works fine with the MQTT library for Arduino. I'll push soon.

lasselukkari commented 9 years ago

OK. Good to hear that you got it working. I wil try it out when you push the changes.

Diaoul commented 9 years ago

The way the AT commands work it's not possible to be sure we don't miss incoming data, there is no lock mechanism in the firmware so +IPD data can be mixed with any command result from what I understand: https://github.com/espressif/esp8266_at/blob/master/at/user/at_ipCmd.c#L413

Do you have a workaround? Using a custom firmware? I don't know if the ESP can afford it but buffering incoming data + provide overflow API + on demand data recv is how I would do it. Another idea is to use a GPIO pin to notify a command is following and callback should wait before ouputing to serial.

Do you know how to build a custom firmware?

lasselukkari commented 9 years ago

Yes. I made a modified firmware with 1kb buffers for each connection in the ESP8266. Firmware has a new AT-command AT+CIPREAD which makes the ESP8266 to print one buffer at time to the serial port. It's just something I have been playing around. No buffer overflow checks.

The guys at Espressif have also just released a new beta verision of the AT firmware: http://bbs.espressif.com/viewtopic.php?f=16&t=99&sid=f9f36e44522450a606c8de3656d7067b

Release notes are: 1.ADD UART TX BUFFER TO RECEIVE DATA FROM TCP PORT 2.ADD A UART TX FLOW CONTROL VIA MTCK( CTS )

Diaoul commented 9 years ago

I think outputing data straight to serial is OK but with a proper flow control mechanism like a XON/XOFF. From the ESP8266 point of view:

I will adapt the code when a new firmware is out with the appropriate features. Meanwhile collision can still happen.

Can you submit your patch to Espressif? I'd like the library to be rock solid and for that I need a rock solid ESP8266...

Diaoul commented 9 years ago

I've added a few missing function to support the Stream interface. I'm closing this one in favor of #7 for discussion about the flow control.