Skip to content
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

Server Sent Events example - issue #7008 #7012

Merged
merged 14 commits into from
May 16, 2020
Merged

Conversation

ewaldc
Copy link
Contributor

@ewaldc ewaldc commented Jan 12, 2020

Illustrates the use of SSE using ESP8266WebServer

edit from maintainer: closes #7008

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 17, 2020

CI has this error (on continuous-integration-...detail button below)

/home/travis/build/esp8266/Arduino/libraries/ESP8266WebServer/examples/ServerSentEventsMultiClient/ServerSentEventsMultiClient.ino:161:22: error: comparison is always true due to limited range of data type [-Werror=type-limits]
       if (channel >= 0)
                      ^
cc1plus: all warnings being treated as errors

Style check can be solved by the script in tests/restyle.sh (then commit the differences)

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 20, 2020

You also need, in this PR, on your local repository, to remove the submodule update you added by error:

Probably with

git checkout upstream/master -- libraries/LittleFS/lib/littlefs

and libraries/SoftwareSerial, tools/sdk/ssl/bearssl, tools/esptool and tools/sdk/lwip2/builder libraries/ESP8266SdFat then commit then push as usual

@ewaldc
Copy link
Contributor Author

ewaldc commented Jan 21, 2020

Done, apologies for my mistake and for wasting developer cycles, there is so much I have to learn about GitHub...
Hopefully I did it right.

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 21, 2020

Did you push ?

@ewaldc
Copy link
Contributor Author

ewaldc commented Jan 23, 2020

Yes, https://github.com/ewaldc/Arduino is fully up to date and equal to my local repository.
The example, which is really the only thing I'd like to contribute, is here

@Martin-Laclaustra
Copy link

@ewaldc
Thank you. This was an inspiring example.
I found a couple of corrections for your code:
https://github.com/ewaldc/Arduino/blob/81ec6e8ae6a68d55e0435c3212dbd419cc9144f5/libraries/ESP8266WebServer/examples/ServerSentEvents/ServerSentEvents.ino#L86
to:
Serial.printf_P(PSTR("SSEKeepAlive - client is still listening on channel %d\n"), i);
https://github.com/ewaldc/Arduino/blob/81ec6e8ae6a68d55e0435c3212dbd419cc9144f5/libraries/ESP8266WebServer/examples/ServerSentEvents/ServerSentEvents.ino#L89
to:
Serial.printf_P(PSTR("SSEKeepAlive - client not listening on channel %d, remove subscription\n"), i);
(the variable for the printf was missing)
and
https://github.com/ewaldc/Arduino/blob/81ec6e8ae6a68d55e0435c3212dbd419cc9144f5/libraries/ESP8266WebServer/examples/ServerSentEvents/ServerSentEvents.ino#L146
to
if (val != newVal) SSEBroadcastState(name, val, newVal); // only broadcast if state is different
(values are interchanged)

@Martin-Laclaustra
Copy link

@ewaldc
I thought that I could add a javascript client side to the example to make it self-contained.
The html page is the attached file (as a header, to be served through the same script).
webpages.h.zip
And in the sketch, only two lines have to be added:
After line 41:
#include "webpages.h"
After:
https://github.com/ewaldc/Arduino/blob/81ec6e8ae6a68d55e0435c3212dbd419cc9144f5/libraries/ESP8266WebServer/examples/ServerSentEvents/ServerSentEvents.ino#L174
server.on("/", HTTP_GET, [](){server.send(200, "text/html", FPSTR(webpagesseclient));});
I hope you find this convenient.

@Martin-Laclaustra
Copy link

@ewaldc
I have a couple of extra suggestions:

  • Dealing with the channels with specific callbacks instead of using server.onNotFound(handleAll);. That way it allows coupling this example with other examples serving content from the filesystem. It also makes the sketch shorter. I already worked on this and have functioning code. Do not hesitate to ask me and I will share this with you.
  • Sending data without "event: event", so it can be received with the javascript .onmessage, which is the standard example you find online for using SSE. I also have this modification (only a couple of lines, but in both files .ino and .h).
  • Fix subscriptionCount to reflect actual used channels. Currently it never decreases.
  • Releasing subscriptions if they are not connected after some timeout (currently this only happens if there is at least one active connection and it can lead to running out of channels without possiblility of release).
  • Creating a simpler example without channels (only one connection per IP), so that the server works in a way compatible with most online javascript examples.
    (I am working in the later points).
    Best

Fix  missing variables in printf statments
Fix subscriptioncount not decreasing
Fix SSEBroadcastState (argument sequence wrong)
@ewaldc
Copy link
Contributor Author

ewaldc commented Feb 4, 2020

@Martin-Laclaustra,
thanks for the corrections, I have posted an update. The example is a simplified version of a more complex code posted here, which also works with HTML5 event listeners in Javascript and JAVA SSE from here maps.
On your suggestions:

  • Dealing with the channels with specific callbacks instead of using server.onNotFound(handleAll);.
    I could not make that work. The problem is that I did not find a call to unregister a callback: no way to invalidate a previous call to server.on() once a channel is invalid. In de real code, a channel is a unique UUID, and I ended up with hundreds of active callbacks on invalid UUID's, all consuming
    memory and slowing down processing of incoming web requests. I also started to use server.on() in a sparse way in general due to its use of strings and extensive memory footprint.
    But maybe you have a solution...

  • Sending data without "event: event", so it can be received with the javascript .onmessage, which is the standard example you find online for using SSE
    The EventSource standard recommends the use of messages to separate between events (of different types). Javascript EventSource supports data, id's and events, so I fail to see the issue with javascript... That said, you can of course, simplify the code to use events containing only data . One other reason why I decided the keep it was that initially I struggled with the keepAlive timer crashing my code. It turned out that I used the wrong Ticker call, hence I decided to add that in so others won't spend hours debugging multi-thread issues...

  • Fix subscriptionCount to reflect actual used channels. Currently it never decreases.
    Oversight on my side, it does of course in my full code... Fixed

  • Creating a simpler example without channels (only one connection per IP), so that the server works in a way compatible with most online javascript examples.
    Originally I posted two examples., with exactly those differences: only one registration per IP address and no channels. But as I was implementing changes to fix certain issues (e.g. WDT reset, nodelay causes crash etc. I decided to go back to one (the more elaborate one) as it was getting complex to update the GitHub PR's. That said, I still have both examples !

Originally I added the webclient, but I felt that I should then also post a Javascript example to make it all work, which in turn would not fit into one single .ino file... Hence I settled for just using curl and adding the HOW TO's just in comments. Maybe I took the lazy way out here :-)

Hope it helps.
Ewald

@Martin-Laclaustra
Copy link

Martin-Laclaustra commented Feb 5, 2020

The example is a simplified version of a more complex code posted here, which also works with HTML5 event listeners in Javascript and JAVA SSE from here maps.

Thank you. I will have a look.

*  _Dealing with the channels with specific callbacks instead of using server.onNotFound(handleAll);._
  I could not make that work.  The problem is that I did not find a call to unregister a callback: no way to invalidate a previous call to _server.on()_  once a channel is invalid.  In de real code, a channel is a unique UUID, and I ended up with hundreds of active callbacks on invalid UUID's, all consuming
  memory and slowing down processing of incoming web requests.  I also started to use server.on() in a sparse way in general due to its use of strings and extensive memory footprint.
  But maybe you have a solution...

It is true that you can not unregister in the ESP server, but that is not necessary. My solution pre-registers the channels (I am assuming they are a reasonable number). "Hundreds of active callbacks" Do you expect hundreds of concurrent channels handled by the ESP? My current example works with single figure channels but it can be easily extended to handle two figure channels.
Here there is a patch against your last update:
channel_handles.patch.zip

Originally I added the webclient, but I felt that I should then also post a Javascript example to make it all work, which in turn would not fit into one single .ino file... Hence I settled for just using curl and adding the HOW TO's just in comments. Maybe I took the lazy way out here :-)

Why do you not consider the one that I uploaded in my previous post? It is kind of a minimal-size example in a single file. You could paste it instead of the include (although I think it is clearer as a separate header file). It is only 58 lines. It can even be further shortened if some unnecessary html or javascript code is removed from it.

Thank you for your attention.

@ewaldc
Copy link
Contributor Author

ewaldc commented Feb 6, 2020

Subscription channels are rarely implemented with just a simple digit as in the example. For example in Eclipse Smarthome, OpenHab and several other IOT frameworks, a (unique) UUID is being used. Security is an important factor in their decision to do so (e.g. avoid unwanted listeners). This invalides pre-registration based solutions.

In addition, some clients register an event listener for each page of their GUI (to listen for object updates) and unregister after the user leaves that page (e.g. OpenHab Android/IOS client). So yes, in a typical day my application handles literally hundreds of subscriptions, some days closer to a thousand. Besides not being able to unregister (causing the ESP to run out of memory), the ESP web server will execute a string compare for every single registered subscription. When dealing with e.g. long paths (not almost in my control) ("http://myiotserver.myverylongdomainname.net:48888/rest/events/listener/") we found out (the hard way) that using server.on() for each subscription consumed not only a lot of RAM (e.g. due the use of the String class) but can slow down the web server to unuseable levels, including hitting soft WDT resets. Hence, why I reverted back to doing one single string comparison to find out whether we are dealing with an event callback or not.

In fact, I ended almost not using server.on() at all, saving lots of memory. If you have hundreds of IOT objects, each of which can be queried by "/rest/item/" , this issue of excessive server.on() calls/string comparisons/memory use only gets worse.

If the ESP Arduino library would handle URL patterns, or match the beginning of the string e.g. server.onStartsWith() that would be a whole different case of course. I actually made a PR to implement such a function, plus the possibility to unregister, made modifications to the String class to avoid excessive copying etc but before I knew, it had become a massive PR which was difficult to explain, unless I reverted back to posting 20 or so individual issues, each with their own PR and code change. Rightfully so, the maintainers keep a close eye on compatibility and memory footprint, so it's not obvious to defend the addition of new functions.

Hopefully, this gives some insights in why things were done differently, even though you have a very valid point that there are simpler and "less-lines-of-code" ways to make the example work.
But I am also a believer in posting examples that trade off some simplicity for real world deployment experience, scalability and performance, specifically in the embedded IOT world.

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR also pulled in a bunch of old submodules (i.e. a git commit -a without a prior git submodule update and so it's busted.

Can you please either remove the erroneous submodules included in the PR, or close this one and make a new one with only your single example file?

@earlephilhower earlephilhower added the merge-conflict PR has a merge conflict that needs manual correction label Feb 22, 2020
@earlephilhower earlephilhower self-requested a review May 15, 2020 23:33
@earlephilhower earlephilhower removed the merge-conflict PR has a merge conflict that needs manual correction label May 15, 2020
@earlephilhower earlephilhower added this to the 2.7.2 milestone May 15, 2020
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Comments are good and the code style looks fine. Seems like something useful in the examples.

Address points of @devyte's code review:
* Use IPAddress vs. uint32_t
* Refactor the URL parsing logic to use strlen vs. sizeof, since there
  was some confusion in the original (correct) version
* Minimize copies of WiFiClients while in use
* Use byref access for sensor updates

Fix multi-sensor updates
* Create an update Ticker for each sensor, because the original code
  only had one whose callback was overridden by sensorB, meaning sensorA
  never changed
Avoid duplicating WiFiClient by using the WiFiClient object embedded in
the subscriber[] array instead.
@earlephilhower earlephilhower merged commit 4519db8 into esp8266:master May 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement request: support for Server Sent Events aka EventSource
5 participants