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

Fixed size json payload & other WS bugfixes #1843

Merged
merged 43 commits into from
Aug 12, 2019

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Aug 6, 2019

Resolve #1731 (?), #1823 (original question), #1824, #1831
cc @gernst48 @tonilopezmr @Interpyme @ruimarinho

I'll clean it up later from some experimental leftovers and fix some dumb behaviour (like, not immediately sending sensor readouts)
POW runs this reasonably well. Refresh does not crash 🤷‍♂
(but I could not make it crash even with old builds. must be something related to OS / browser / network conditions)

And probably try again to rethink how to push static data before everything else, to reduce total number of messages (although, it is smaller than before #1387 / 1.13.3)

Keeping ArduinoJson 5 for now.

@mcspr
Copy link
Collaborator Author

mcspr commented Aug 7, 2019

If anyone can test the tree itself (i.e. checkout the mcspr:web/fixed-size-json-v5), that would be greatly appreciated.

Some notes about the ws module, as the main changes are there... The goal is to chain send_callback_f funcs so that it firstly bulks up some known constants in a single object root and sends it (...Visible is an obvious one, since it only depends on the module being present). But, after that, callbacks need to be called separately again. Some state / stage var can control this, maybe... And the final one sending sensor data.

@tonilopezmr
Copy link
Contributor

tonilopezmr commented Aug 7, 2019

This PR fixes my problems!! well done @mcspr

@tonilopezmr
Copy link
Contributor

tonilopezmr commented Aug 7, 2019

I'm still facing this #1731 because I don't get Current, Apparent power and Reactive Power for HLW8012 in Shelly 1PM

@mcspr
Copy link
Collaborator Author

mcspr commented Aug 7, 2019

Great. Need to also check Windows / Chrome situation, if queuing does work properly with delayed acks (...and the main reason single big json was used in the first place)

OT: I think we determined in the #1822 that indexing in the hwl sensor needs to be customizable, nothing in the web specifically. That way, it will also not be there in mqtt reports, thingspeak etc. And I kind of missed whether you wanted to add that yourself or not 🤔

@tonilopezmr
Copy link
Contributor

I tested it using mac os and chrome

@mcspr
Copy link
Collaborator Author

mcspr commented Aug 10, 2019

Note-to-self:
std::queue<T>, where T was never seen before, adds ~500bytes to the total firmware size (...because there is also std::deque<T> inside of it?)
edit: And also wastes 512bytes of RAM per queue instance:
https://github.com/gcc-mirror/gcc/blob/gcc-9_1_0-release/libstdc++-v3/include/bits/stl_deque.h#L94-L95

One alternative is to use custom linked list class.
or LinkedList<T> from ESPAsyncWebserver, which is exported by default -> https://github.com/me-no-dev/ESPAsyncWebServer/blob/master/src/StringArray.h
not as big, surprisingly. API is almost the same: empty() -> isEmpty(), pop() -> remove(front()), push() -> add()

@mcspr
Copy link
Collaborator Author

mcspr commented Aug 12, 2019

Resolves #1824
Resolves #1831

@mcspr mcspr merged commit 2142343 into xoseperez:dev Aug 12, 2019
@mcspr mcspr deleted the web/fixed-size-json-v5 branch August 12, 2019 20:24
mcspr added a commit that referenced this pull request Aug 12, 2019
Amend #1843 , since we have updated ESPAsyncWebServer
Fixes (again) #1300

Gather WS debug messages in a buffer and flush every N times
info,keys,crash actually output data now
This was referenced Aug 15, 2019
@tonilopezmr
Copy link
Contributor

Congratulations @mcspr, awesome PR!!

@ruimarinho
Copy link
Contributor

@mcspr does this solve the double authentication request for credentials on Safari?

@mcspr
Copy link
Collaborator Author

mcspr commented Aug 21, 2019

@ruimarinho nothing about the auth was changed here. do you mean that it is showing auth pop-up twice?
/ is requesting digest auth and then script inside is calling /auth, also requiring digest auth. if yes, safari is an odd one here :/ every other browser caches credentials after the first request

@ruimarinho
Copy link
Contributor

@mcspr indeed, that. For some reason I made the connection between the WS connection and the double digest auth, but I haven't looked at the code yet. Maybe we can remove the script call to /auth and just depend on session cookie to pass on the auth header?

@mcspr
Copy link
Collaborator Author

mcspr commented Aug 21, 2019

Probably, yes. It is pretty basic right now, /auth whole purpose is to set / update {IP: timeout} in the internal "ticket" table. WS handler then checks that table, since Safari also does not support digest auth for Upgrade protocols (#507 (comment)).
Every request is filtered via webAuthenticate(request) and depends on the result of request->authenticate(WEB_USERNAME, password); where it checks if browser used digest auth headers or not. It is a likely place to move current auth mechanism "ticket" update, or update it with some per-client session instead

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.

Multiple web clients cause crash
3 participants