Networking rampage and accumulated fixes #3060
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
ETA, again: found and fixed a memory leak in the new |
|
Cool! I like every PR that removes more code than it adds. Can we land this on |
|
I'm happy to hit merge and deal with any fallout, but I suspect it would be better if someone else gave things a quick run-through first. ;) |
This is the easiest part of nodemcu#3004 . It removes a bunch of functions that were never called in our tree.
Further work on nodemcu#3004 While here, remove `mqtt`'s charming DNS-retry logic (which is neither shared with nor duplicated in other modules) and update its :connect() return value behavior and documentation.
A write-only global! How about that.
All the TLS stuff moved over there a long time ago, and net_createUDPSocket should just do what it says on the tin.
We can barely function as a TLS client; being a TLS server seems like a real stretch. This code was never called from Lua anyway.
There is nothing "ssl_packet" about this structure. Get rid of the terrifying "pbuffer" pointer. Squash two structure types together and eliminate an unused field.
Split out espconn_mbedtls_parse, which we can use as part of our effort towards addressing nodemcu#3032
The new feature part of nodemcu#3032 Subsequent work will remove the old mechanism.
nwf
force-pushed
the
for-upstream
branch
2 times, most recently
from
e38ce43
to
5bf1665
Compare
|
I think it would be very valuable (and thus welcome by the community) if we merged dev back to master in the coming weeks. It's been almost six months but there are a few things pending: https://github.com/nodemcu/nodemcu-firmware/milestone/14. So, I wouldn't mind landing this on dev soonish as it looks quite mature. |
Everywhere we have the mqtt_state_t we also have the lmqtt_userdata.
Instead, just place the espconn structure itself at the top of the user data. This enlarges the structure somewhat but removes one more layer of dynamic heap usage and NULL checks. While here, simplify the code a bit.
|
Argh, I didn't really mean to squash all those together. I'm feeling jittery about more of them, now, too, but whatever, I think I can live with some egg on my face and follow-up commits to fix bugs as we find them. |
marcelstoer
pushed a commit
that referenced
this pull request
Jun 9, 2020
* espconn: remove unused espconn code, take 1 This is the easiest part of #3004 . It removes a bunch of functions that were never called in our tree. * espconn: De-orbit espconn_gethostbyname Further work on #3004 While here, remove `mqtt`'s charming DNS-retry logic (which is neither shared with nor duplicated in other modules) and update its :connect() return value behavior and documentation. * espconn: remove scary global pktinfo A write-only global! How about that. * net: remove deprecated methods All the TLS stuff moved over there a long time ago, and net_createUDPSocket should just do what it says on the tin. * espconn_secure: remove ESPCONN_SERVER support We can barely function as a TLS client; being a TLS server seems like a real stretch. This code was never called from Lua anyway. * espconn_secure: more code removal * espconn_secure: simplify ssl options structure There is nothing "ssl_packet" about this structure. Get rid of the terrifying "pbuffer" pointer. Squash two structure types together and eliminate an unused field. * espconn_secure: refactor mbedtls_msg_info_load Split out espconn_mbedtls_parse, which we can use as part of our effort towards addressing #3032 * espconn_secure: introduce TLS cert/key callbacks The new feature part of #3032 Subsequent work will remove the old mechanism. * tls: add deprecation warnings * luacheck: net.ifinfo is a thing now * tls: remove use of espconn->reverse * mqtt: stop using espconn->reverse Instead, just place the espconn structure itself at the top of the user data. This enlarges the structure somewhat but removes one more layer of dynamic heap usage and NULL checks. While here, simplify the code a bit. * mqtt: remove redundant pointer to connect_info Everywhere we have the mqtt_state_t we also have the lmqtt_userdata. * mqtt: doc fixes * mqtt: note bug * tls: allow :on(...,nil) to unregister a callback
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
devbranch rather than formaster.docs/*.This represents the work done thus far on a few issues; #3032 and easy aspects of #3004 (xref #3006) as well as refactoring and simplification around the network-facing things I use regularly. I've been carrying these patches locally for a while and would like to push them upstream, at least to
dev, for others to beat about a bit and so that other things stop feeling quite so blocked on these. Much of this has been subject to testing via the test harness in my https://github.com/nwf/nodemcu-firmware/tree/dev-active branch (recall #2983), so I feel fairly confident in having not broken anything obvious.The
mqttandtls"stop using espconn->reverse" refactoring could be done for other network-facing modules (http,coap, andwebsocket) as well and would probably be similarly beneficial. Then we can remove the->reversefield fromespconnand make a little more progress on #3006. I don't feel confident in changing code in the remaining ones, absent test support.ETA: removed
espconn->reversefromtls, too, and fixed a stack imbalance issue in the new TLS cert/key callback code.