-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Implement the Stream interface #2
Comments
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. |
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. |
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 |
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. |
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. |
The firmware can surely benefit from a few patches like this one :) I'm open to suggestions |
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. |
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. |
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. |
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. |
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? |
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. 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. |
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. |
Sending a buffer doesn't work? It should be as efficient as it can be. |
OK. Good to hear that you got it working. I wil try it out when you push the changes. |
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? |
Yes. I made a modified firmware with 1kb buffers for each connection in the ESP8266. 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: |
I think outputing data straight to serial is OK but with a proper flow control mechanism like a XON/XOFF.
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... |
I've added a few missing function to support the Stream interface. |
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.
The text was updated successfully, but these errors were encountered: