Diaoul / arduino-ESP8266

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

ESP8266Server class #11

Closed lasselukkari closed 9 years ago

lasselukkari commented 9 years ago

Currently the usage of ESP8622Client is hard when runnig as server because the ESP8622Client writes to the default channel if the channel id is not specified in the constructor.

The new ESP8622Server class returns a ESP8622Client instance with the correct channel id set when data is available.

You probably had something similar already in mind. Please let me know if you want me to change something.

Diaoul commented 9 years ago

Can you provide an example to illustrate how one would use this?

lasselukkari commented 9 years ago

I'm using it like this with a HTTP server component:

  ESP8266Client * espClient = espServer.available();

  if (espClient){
    httpServer.process(espClient);
  }

The EthernetServer class works similarly altough it didn't actually support multiple sockets the last time I checked.

For some reason the latest update to the ESP8266 class send functions broke my webserver writing.

Diaoul commented 9 years ago

That shouldn't have happened, @jonathanchristison any idea regarding backward compatibility of your changes? Is this working fine on all Arduino IDE version?

jonathanchristison commented 9 years ago

@Diaoul I've only tested these changes with Arduino 1.0.5 (avr-g++ 4.8.3), Feel free to revert the changes until I do some further testing based on the information I get from @lasselukkari . I cant think of any reason it wouldn't "just work".

@lasselukkari Could you provide a bit more information what has broke w/regards to the templated send functions? Is it a compilation error, or runtime error? Could you provide some snippits how you are using .send() ?

lasselukkari commented 9 years ago

I haven't dig into the actual AT commmans that are sent yet. But the symtoms are that instead of returning a HTML page the browser (Chrome) starts to download the content as file. When I open the file with a text editor I get this:

P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!P!!!!!!!!!`!ääääW!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!W!

If I revert back to the old code everything works.

jonathanchristison commented 9 years ago

@lasselukkari You'll have to forgive my ignorance with regard to the ESP8266Client. Looks like it could be down to line 55 in ESP8266Client.cpp - https://gist.github.com/jonathanchristison/cbe568db583ca91acbd5

Could you give that a whirl, see if it fixes the problem? If not i'll need to test it more thoroughly tomorrow.

Cheers

lasselukkari commented 9 years ago

Didn't work.

Here is the AT command output from the original working code when sending the HTTP response:

AT+CIPSEND=0,32 HTTP/1.0 200 OK Access-Control-AT+CIPSEND=0,31 Allow-Origin: * Content-Type: AT+CIPSEND=0,1 tAT+CIPSEND=0,1 eAT+CIPSEND=0,1 xAT+CIPSEND=0,1 tAT+CIPSEND=0,1 /AT+CIPSEND=0,1 hAT+CIPSEND=0,1 tAT+CIPSEND=0,1 mAT+CIPSEND=0,1 lAT+CIPSEND=0,2

AT+CIPSEND=0,2

... continues

And here is the AT command output from the new version:

AT+CIPSEND=0,2 P!AT+CIPSEND=0,2 P!AT+CIPSEND=0,2 P!AT+CIPSEND=0,1 tAT+CIPSEND=0,1 eAT+CIPSEND=0,1 xAT+CIPSEND=0,1 tAT+CIPSEND=0,1 /AT+CIPSEND=0,1 hAT+CIPSEND=0,1 tAT+CIPSEND=0,1 mAT+CIPSEND=0,1 lAT+CIPSEND=0,2 ŠAT+CIPSEND=0,2 ŠAT+CIPSEND=0,2 W!AT+CIPSEND=0,2 W!AT+CIPSEND=0,2 ... continues

I will try to debug this more tomorrow.

lasselukkari commented 9 years ago

I have this simple sketch:

#include <ESP8266.h>

ESP8266 wifi = ESP8266(Serial);

void setup() {
  Serial.begin(115200);
}

void loop(){
  char * testString = "testing";  
  wifi.send(testString);
}

And I have commented out the send ACKs so that I can see the AT commands it's trying to send even it doesn't get any reply.

With old version it's seding

‹AT+CIPSEND=7
testing

And with the new version it sends

‹AT+CIPSEND=2
// nothing

The environment is Arduino IDE 1.0.6 with OS X Yosemite.

lasselukkari commented 9 years ago

The g++ that ships with the OS X version of Arduino IDE is 4.3.2.

$ /Applications/Arduino\ 1.0.6.app/Contents/Resources/Java/hardware/tools/avr/bin/avr-g++ --version

output:

avr-g++ (GCC) 4.3.2
Copyright (C) 2008 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
tprochazka commented 9 years ago

Current implementovat is not really good. I agree with @lasselukkari, that something like this should help.

For example when I have this

if (wifi.available()) {
    Serial.print(" > ");
    Serial.print(wifi.getId());
    Serial.print(" > ");
    Serial.println(wifi.readString());
}

And connect from two clients from the server and every client sent different letter, like A and B, ESP8266 send this:

 +IPD,1,1:A
 OK

 +IPD,0,1:B
 OK

but my code print

 > 0 > AB

And this is not good

Diaoul commented 9 years ago

Well, this is how readString works. It reads until timeout. If that's not what you want don't use readString. You can call available() times successive reads.

tprochazka commented 9 years ago

But it should read only one channel, not mix multiple.

@lasselukkari solution it is not best, because it create 5 instances of client at the start and fixs only one part of the problem. Because the same problem will be if I will use library only for connection to remote servers.

Now I can send multiple request by wifi.send(), where first parameter is channel id, but If booth answers come in the same time library will mix it.

Diaoul commented 9 years ago

With no buffer I don't see how that's possible not to mix channels. The low level library in ESP8266.h is only a wrapper around the AT command set. The Stream interface is implemented as a helper for a few inherited functions but does not guarantee to stop on channel id change. Should you consider the ESP8266 as a Stream, reading means you read everything, the Stream interface doesn't expose channel ids or anything similar.

I'm not sure we should change the semantics of the function while inheriting an interface even if that makes sense.

tprochazka commented 9 years ago

I think that ideal will be if timedRead() will always return -1 after number of bytes defined by y in +IPD,x,y So next readString() will alway return only one data block defined by +IPD And in next loop will be processed next +IPD.

lasselukkari commented 9 years ago

Yes, it creates 5 instances. That is the whole idea. You may have 5 different incoming connectios that you are handling at the same time. I can't see how you could implement the fuctionality in any other way if you want to keep it compatible with the Client interface.

tprochazka commented 9 years ago

You can create new instance on demand, in most cases will not be 5 incoming connections.

lasselukkari commented 9 years ago

The Client class has one pointer to the esp8266 instance, one boolean value and one int. I don't see the overhead as a problem. Generally dynamic memory allocation is a bad idea with Arduino although i don't think it would really be problem here.

In my projects when is use the ESP with a webserver usually the first page load creates atleast 3 simultaneus requests: html, favico and a rest api request.

jonathanchristison commented 9 years ago

@lasselukkari I think I fixed the issues you where having by adding a bunch of overloads for char arrays and string types, its a bit unclean but it seems to do the job.

The fault was a oversight on my behalf, sizeof() does not give the size of a string unless its a string literal. I had only tested this with a string literal.

See pull request 13 - https://github.com/Diaoul/arduino-ESP8266/pull/13

Diaoul commented 9 years ago

@lasselukkari: please add an example in examples/

lasselukkari commented 9 years ago

Is it ok if the example requires one additional library?

Diaoul commented 9 years ago

It is OK as long as it is a known library and covers a common use case I guess.

tprochazka commented 9 years ago

In my projects when is use the ESP with a webserver usually the first page load creates at least 3 simultaneous requests: html, favico and a rest api request.

So. I still don't understand one thing. How you use it. You have 5 instances which are able send data to different channel, but all of them read the same data, data from all incoming requests. So if you have 3 simultaneous incoming connection read() will read all of them as one stream? How do you handle it? Yes you can search for something like "GET " and url after it.... This is the same situation as we discuss in my pull request with multiple Stream implementation and ESP8622Client is Stream implemention...

lasselukkari commented 9 years ago

I don't think it's possible handle 5 different simultaneus connections unless you read them one by one. The idea of the Server class is to return a connected Client instance that has the correct id set so that you can write back to the correct channel by using the same client.

It doesn't try to solve any of the problems with the ESP8266 multiplexing.

tprochazka commented 9 years ago

It is not necessary to read 5 connection simultaneous, ESP8266 serialize them, it comes one by one, so I think that we actually can have 5 different streams, only one will be available at one time. But also solution with one stream which will be possible to read by one IPD block is enough I think.