net_info module - ping function

[vsky279]

Fixes #1555.

• This PR is for the dev branch rather than for master.
• This PR is compliant with the other contributing guidelines as well (if not, please describe why).
• I have thoroughly tested my contribution.
• The code changes are reflected in the documentation at docs/*.

This PR is created based on initial module by @wolfgangr with only few fixes. It seems to be pity to drop the work that has been done by @wolfgangr.
This version is working reliably in one of my applications so I think it is eligible to be merged into dev version.
Once reviewed I will squash these commits into one.
Actually can I do it just for the PR and have them separated in my branch?

[marcelstoer]

Regardless of whether this will ever be merged...the CI build failed. Not sure if you have access to this https://travis-ci.org/nodemcu/nodemcu-firmware/jobs/563711700

net_info.c:14:22: fatal error: c_stdlib.h: No such file or directory
 #include "c_stdlib.h"
                      ^
compilation terminated.
../../Makefile:417: recipe for target '.output/eagle/debug/obj/net_info.o' failed
make[2]: *** [.output/eagle/debug/obj/net_info.o] Error 1
make[2]: Leaving directory '/home/travis/build/nodemcu/nodemcu-firmware/app/modules'
../Makefile:380: recipe for target '.subdirs' failed
make[1]: *** [.subdirs] Error 2
make[1]: Leaving directory '/home/travis/build/nodemcu/nodemcu-firmware/app'
Makefile:380: recipe for target '.subdirs' failed
make: *** [.subdirs] Error 2
make: Leaving directory '/home/travis/build/nodemcu/nodemcu-firmware'
The command "if [ "$OS" = "linux" ]; then bash "$TRAVIS_BUILD_DIR"/tools/travis/ci-build-linux.sh; fi" exited with 1.

That is due to #2838.

[vsky279]

I will fix it when #2853 is solved. I can't compile now :-(

[nwf]

Thank you for the contribution. Before this can be merged, I would like to see several changes to the module, not the least of which is that I would like to be able to have multiple ping requests going at once (as LwIP supports) and see proper C object life-cycle management.

[vsky279]

Regardless of whether this will ever be merged...the CI build failed. Not sure if you have access to this https://travis-ci.org/nodemcu/nodemcu-firmware/jobs/563711700

Ok, so at least it compiles now.

Nathan @nwf thanks for all suggestions. Progressively I will try to implement them. It will be pending for some while as I'm on another project now.

Stay tuned.

[nwf]

Thank you for your continued work on this module. 👍

[TerryE]

would be great of that experience eventually led to an updated (or "finally released" actually) version of this page: https://github.com/nodemcu/nodemcu-firmware/wiki/%5BDRAFT%5D-How-to-write-a-C-module

@marcelstoer my thoughts entirely.

[vsky279]

Ok, so now we have very clean implementation of the net_info.ping() function.
It was redesigned with great help of @TerryE Terry (in fact Terry re-wrote it 😄).
That was great experience that improved substantially my understanding of C Lua programming. Thank you.

[TerryE]

(in fact Terry re-wrote it smile).

That's too much attribution, IMO. We wouldn't be here without Lukáš doing the heavy lifting. Doing this exercise has really help me clarify some of my own thoughts. Thanks for your work.

[TerryE]

@nwf I think that this code in good to go now. My only Q is whether this one function merits its own module or should be just move it into net possibly with a NET_INFO_ENABLE option in user_config.h just in case we don't always want to compile in the ping code. What is your view?

[nwf]

I'd like it to be merged with net, available as net.ping(), possibly gated with NET_PING_ENABLE.

[marcelstoer]

My only Q is whether this one function merits its own module or should be just move it into net

That has come up here and in the original issue #1555 a couple of times. I personally favor the latter with no extra option. I don't see why this should be gated and other stuff not.
The current net module is quite a large beast (server sockets, client sockets, TCP, UDP, DNS) that adding another function won't make much difference. If at some point someone feels motivated to split it into more manageable chunks then yes, go for it.

P.S. any extra y/n option will make a function unavailable to those who don't compile stuff themselves

[nwf]

@marcelstoer Well, merging with net and gating behind a default-on #define would be fine for web builders and leave the option for people trying to reduce firmware size. Still, I think you're right that just landing it in net is fine.

[TerryE]

@vsky279 can you drop the net_info module, ditto the separate doc file and modules define. Add your code and doc to net and the enabled NET_INFO_ENABLE option in user_config.h.

I'd like to merge this before I do my next tranche of fixes. Thanks.

[vsky279]

ping is a part of net module now. By default it is not included. NET_INFO_ENABLE needs to be defined in user_config.h.
I preserved ping implementation in separate net_info.c and net_info.h files. It seems cleaner to me then to have it directly in net.c. But let me know if you prefer latter option.

I was thinking about implementing a callback to the sent event as @TerryE suggested once. A second callback function as the fourth parameter of net.ping seems to me appropriate. What do you think?

[nwf]

I dislike the use of the name info for ping; can it be net_ping.c and NET_PING_ENABLE? Sorry for the long tail of cosmetic changes.

ETA: Another callback at the end seems like a perfectly reasonable addition, yes.

[vsky279]

All implemented and tested. Should be ready to be merged.