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

Thingspeak: async client fixes #1806

Merged
merged 7 commits into from
Jul 7, 2019
Merged

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Jun 30, 2019

  • remove memory management in async client callbacks
  • instead of global one, reference asyncclient from callback argument
  • transfer temporary data-buffer to the global scope, avoid unintentional String copy
  • implement strnstr to search onData payload

@JavierAder as discussed in #1752
Will try to leave it running with some sensor board.

There is another rough one, but greatly increasing firmware size:
dev...mcspr:tspk/client-fixes
+1280bytes vs +48416bytes here
(supporting newest Core versions requires some tweaks to the networking part. I hope that can be reused somewhere to justify code size increase)

- remove memory management in async client callbacks
- instead of global one, reference asyncclient from callback argument
- transfer temporary data-buffer to the global scope, avoid accidental String copy
- implement strnstr to search onData payload
@JavierAder
Copy link

I will try it; it seems a good solution. I would only add a handler for AsyncClient:onError, but I am not totally sure (is it necesary call to close in case of error? in case of dns error?).
Another thing, when you do '_tspk_data = "";' it releases the previously assigned memory? or it generates garbage? Probably it is a silly question, but I come from the Java world...

@mcspr
Copy link
Collaborator Author

mcspr commented Jul 2, 2019

Error is any lwip-level error, including dns:
https://github.com/me-no-dev/ESPAsyncTCP/blob/7e9ed22ed0078097f895dfee6ebcec90046b81b9/src/ESPAsyncTCP.cpp#L751-L788
https://github.com/me-no-dev/ESPAsyncTCP/blob/7e9ed22ed0078097f895dfee6ebcec90046b81b9/src/ESPAsyncTCP.cpp#L477
It would still call onDisconnect:
https://github.com/me-no-dev/ESPAsyncTCP/blob/7e9ed22ed0078097f895dfee6ebcec90046b81b9/src/ESPAsyncTCP.cpp#L356-L360

Regarding string, you can check out WString.cpp in the core. Internally, it allocates on heap. Nice implementation detail is that it copies right-hand argument, replacing data but not buffer itself ("" is \0) when it is less in size than the buffer. e.g. I added:

- DEBUG_MSG_P(PSTR("[THINGSPEAK] POST %s?%s\n"), THINGSPEAK_URL, _tspk_data.c_str());
+ DEBUG_MSG_P(PSTR("[THINGSPEAK] POST %s?%s [%p]\n"), THINGSPEAK_URL, _tspk_data.c_str(), _tspk_data.c_str());
[206136] [THINGSPEAK] Enqueuing field #1 with value 25.804
[206139] [THINGSPEAK] Enqueuing field #2 with value 985.4015
[206140] [THINGSPEAK] Enqueuing field #3 with value 51.57
[206303] [THINGSPEAK] Connected to api.thingspeak.com:80
[206304] [THINGSPEAK] POST /update?field1=25.804&field2=985.4015&field3=51.57&api_key=blah [3fff4f5c]
[206480] [THINGSPEAK] Response value: 39
[206482] [THINGSPEAK] Disconnected
[266190] [THINGSPEAK] Enqueuing field #1 with value 25.712
[266193] [THINGSPEAK] Enqueuing field #2 with value 985.3445
[266194] [THINGSPEAK] Enqueuing field #3 with value 51.83
[266499] [THINGSPEAK] Connected to api.thingspeak.com:80
[266500] [THINGSPEAK] POST /update?field1=25.712&field2=985.3445&field3=51.83&api_key=blah [3fff4f5c]
[266676] [THINGSPEAK] Response value: 0
[266677] [THINGSPEAK] Re-enqueuing 2 more time(s)
[266677] [THINGSPEAK] Disconnected
[281834] [THINGSPEAK] Connected to api.thingspeak.com:80
[281836] [THINGSPEAK] POST /update?field1=25.712&field2=985.3445&field3=51.83&api_key=blah [3fff4f5c]
[282040] [THINGSPEAK] Response value: 41
[282041] [THINGSPEAK] Disconnected

Sorry, I missed the timestamping earlier but I think it is working ok right now.

@JavierAder
Copy link

Ok, finally, starting testing under SonOff Pow R2 v2.0 and dev 1.3.16 (but from June 9... i din't see that there were new commits...), and using AsyncTCP 1.2.0 (https://github.com/me-no-dev/ESPAsyncTCP/compare/55cd520..7e9ed22 almost no difference with 1.1.3, only a potential null access when you close a connection already closed and other few fixes related to SSL).
For now (2 hours running), it seems all right; no crash, and no lost connection with thingspeak server.

@mcspr
Copy link
Collaborator Author

mcspr commented Jul 4, 2019

I have the same results with Sonoff Basic & temperature sensor.
I'll bump ESPAsyncTCP version anyways.

Ok, finally, starting testing under SonOff Pow R2 v2.0 and dev 1.3.16 (but from June 9... i din't see that there were new commits...), and using AsyncTCP 1.2.0 (https://github.com/me-no-dev/ESPAsyncTCP/compare/55cd520..7e9ed22 almost no difference with 1.1.3, only a potential null access when you close a connection already closed and other few fixes related to SSL).

Just curious, did you use my tree https://github.com/mcspr/espurna/tree/tspk/client-static or applied patches manually? (combined with asynctcp library version change).

@JavierAder
Copy link

2 days running, no crash, no lost connection; all right.

Just curious, did you use my tree https://github.com/mcspr/espurna/tree/tspk/client-static or applied patches manually? (combined with asynctcp library version change).

I applied patches manually (I don't grasp git very well....). I took your last thinkspeak.ino ("commit promptly run flush" ad0e34a, included ) and manually edit prototypes.h and utils.h based in the other commits referenced in this pull request (declaration of strnlen and strnstr in prototypes.h and definition of strnstr in utils.ino); and yes, in platformio.ini I edited the dependences. I currently use
https://github.com/me-no-dev/ESPAsyncTCP#7e9ed22

I don't know if it is necessary git your tree completely, at least, with respect to thingspeak; anyway, as soon as I can, I will.

mcspr added a commit that referenced this pull request Jul 7, 2019
Fix random resets when connection closes prematurely
@mcspr mcspr merged commit bd00758 into xoseperez:dev Jul 7, 2019
@mcspr mcspr deleted the tspk/client-static branch July 7, 2019 08:21
@mcspr
Copy link
Collaborator Author

mcspr commented Jul 7, 2019

You might want to check out:
https://hub.github.com/

Or more manual method:
https://help.github.com/en/articles/checking-out-pull-requests-locally

Ofc, even more manual approach works too:

$ git remote add mcspr https://github.com/mcspr/espurna
$ git fetch mcspr <branch>
$ git checkout -b <local branch> <branch> / FETCH_HEAD

I think we need to add this to readme or something, to reduce the friction of pr testing...
We should heavily imply that we want to test dev, not master + patch (sometimes happens too)

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.

None yet

2 participants