expose ping function to lua

[lepass7]

Missing feature

ping function

Justification

ping function will help the developer to know if there is connectivity to web and search for other devices in local network

Workarounds

I am currently using net module and doing post requests

[NicolSpies]

This would be helpful, I currently use net.dns.resolve() with a return IP address as a pseudo ping to check web connectivity. I use the MQTT server URL as the "pinged" address to in effect not only confirm general web connectivity but connectivity to the IP in play.

With the above said, to use your suggested ping function, I would have to do a dns resolve to get the IP first but I will only do it to confirm that the IP has not changed. I also assume the response size and speed from a ping will be smaller and faster when compared to a dns resolve.

[marcelstoer]

Given that you didn't ask for an ICMP ping implementation I'd vote to close this.

know if there is connectivity to web

There are several alternatives available in the firmware (plain socket, net.dns.resolve, http.request(url, "HEAD"...).

search for other devices in local network

In order to properly search you'd need more than just a ping anyway.

[NicolSpies]

I agree, we need protection against a "Ping of Death".

[wolfgangr]

May I propose to reopen the issue, please?

I hope that ping from Lua Land may be one of the first windfall results from my dealing with ethernet driver implementation (issue #1725).

• there is a app/lwip/app/ping.c and a app/include/lwip/app/ping.h in nodemcu-firmware
• there is a tutorial (by 'smcl' - knwon here?) http://blog.mclemon.io/esp8266-contributing-to-the-nodemcu-ecosystem with accompanying code https://github.com/smcl/nodemcu-firmware/ with an code example for surfacing ping to lua.

The promise is

> test.ping("google.com", 3, ping_recv)
    > 32 bytes from 216.58.214.206: icmp_seq = 1, ttl=53, time=14 m

and although I agree that there are other ways of network diagnose, people (including me) will like it.

However, smcl patches ping.c and ping.h to get the ttl and time fields. I consider those as essential parts of ping and am in favour of that. However, patching data structures breaks compatibility, which in turn provided me already with a great debugging learning example :~~| .

So we might

• keep a second interface like ping2, increasing maintenance
• don't bother breaking the interface if nobody uses ist
• change the interface and its users if we know about them
• leave out the ttl and the time fields in the output

I would ask the guardians of the code for a proposal hwo to proceed.
Without further guidance, I would go for the safe way and keep a parallel ping.

[wolfgangr]

ping pings :-)

=test.ping("www.google.de",2)
=test.ping("www.google.de",2)
> 32 bytes from 172.217.20.195, icmp_seq=25 ttl=53 time=37ms
32 bytes from 172.217.20.195, icmp_seq=26 ttl=53 time=38ms
ping 2, timeout 0, total payload 64 bytes, 1946 ms
=test.ping("192.168.0.50",2)
=test.ping("192.168.0.50",2)
> 32 bytes from 192.168.0.50, icmp_seq=27 ttl=62 time=5ms
32 bytes from 192.168.0.50, icmp_seq=28 ttl=62 time=3ms
ping 2, timeout 0, total payload 64 bytes, 1976 ms

I haven't yet decided on a place in namespace.
So i'd wait with a pull request until my other network management stuff settles.
Maybe I'd include it there.

[wolfgangr]

Following are the changes in the ping.h

...:~/test/esp-SDK/nodemcu-firmware/app/include/lwip/app$ diff ping.h ping2.h
77a78
>         uint8  ttl;

and ping.c files:

...:~/test/esp-SDK/nodemcu-firmware/app/lwip/app$ diff ping.c ping2.c
59c59
< #include "lwip/app/ping.h"
---
> #include "lwip/app/ping2.h"
167c167,170
<                                 pingmsg->ping_opt->recv_function(pingmsg->ping_opt,(void*) &pingresp);
---
>                                 pingresp.ttl = iphdr->_ttl;
>                                 pingmsg->ping_opt->recv_function(pingmsg,(void*) &pingresp);
> //                              pingmsg->ping_opt->recv_function(pingmsg->ping_opt, (void*) &pingresp);
> 

For reference, these are the complete definitions fof the relevant structs in ping2.h:

struct ping_option{
        uint32 count;
        uint32 ip;
        uint32 coarse_time;
        ping_recv_function recv_function;
        ping_sent_function sent_function;
        void* reverse;
};

struct ping_msg{
        struct ping_option *ping_opt;
        struct raw_pcb *ping_pcb;
        uint32 ping_start;
        uint32 ping_sent;
        uint32 timeout_count;
        uint32 max_count;
        uint32 sent_count;
        uint32 coarse_time;
};

struct ping_resp{
        uint32 total_count;
        uint32 resp_time;
        uint32 seqno;
        uint32 timeout_count;
        uint32 bytes;
        uint32 total_bytes;
        uint32 total_time;
        uint8  ttl;
        sint8  ping_err;
};

From my (still limited) understanding of C, the addition of the ttl-Field to ping_resp would not hurt anybody, as long as they rely on the symbolic refernces, right?

However, the change from pingmsg->ping_opt to pingmsg as pointer to handed over to the registered function upon ping received severly brakes the interface.

Would it be a solution to include a field like parent or msg to ping_opt where we store a pointer to the msg? While this still may look awkward, it would not brake interfaces. An appliation expecting the old behaviour would still receive the ping_opt it expects. An application aware of the changes could load the msg by this pointer and thus retrieve the ping time value.

[wolfgangr]

ok, just hacked this backreference, looks like it works.
Still some causes for reboot i have to catch

[wolfgangr]

OK, I catched the reboot issue on ping when WIFI down.

There are still some quirks with converting illegal IP numbers, but this is done by lwIP itself, and I do not dare to fiddle with this.

If somebody wants to try before the namespace question is settled:
https://github.com/wolfgangr/nodemcu-firmware/tree/wolfgangr-ping

file added:

/app/modules/test.c

files changed:

/app/include/lwip/app/ping.h
/app/include/user_modules.h
/app/lwip/app/ping.c

Presumably it even could handle callbacks on pings received, but I haven't tested.
Find some docu here (bottom qarter of the tutorial):
http://blog.mclemon.io/esp8266-contributing-to-the-nodemcu-ecosystem

[devyte]

I think this is a great addition, good job! I suggest making it net.ping().

[marcelstoer]

Good stuff.

So i'd wait with a pull request until my other network management stuff settles.

We prefer small isolate PRs for isolated features rather than one big blob.

I suggest making it net.ping().

👍

[wolfgangr]

suggest making it net.ping()

OK, sounds good.

We prefer small isolate PRs for isolated features

So would I. But I'd like to keep the goal in mind, which was an ethernet driver.
So, can I later have net.netif() or net.if() in a different file?

Do file names and module names have any other connection rather than by convention?
How is the source tree to be organized? So where are files supposed to live?

I simply could keep the file in place and rename it to nodemcu-firmware/app/modules/ping.c .
But I'd consider to open a network related subfolder?
Should I? And if so, where?

After obvious cleanup of tutorial scrap, this is what the function map definition might look like

// Module function map
static const LUA_REG_TYPE test_map[] = {
  { LSTRKEY( "ping" ),             LFUNCVAL( test_ping ) },
  { LSTRKEY( "__metatable" ),      LROVAL( test_map ) },
  { LNILKEY, LNILVAL }
};

// Define an empty test function 
int luaopen_test( lua_State *L ) {
  return 0;
}

NODEMCU_MODULE(TEST, "test", test_map, luaopen_test);

OK, change all test to net is the second obvious thing to do.
Ehmm - nearly all.
Should I have a luaopen_test function? Right now, I only know that omitting it breaks build...
Or is it just fair to leave room for others doing future improvements ;-)

And I think I 'm supposed to supply Documentation - at least in some later PR to come.
Where? How? (mmh, I remember some contribution guide...should read that again this night...)

And how can I merge module maps across files?
Can I do so at all?
How is the module map thing processed?
Is there any documentation? Or at least a pointer to the relavant source?

I see there is a subfolder lua_modules/*.
I would even like to include lua code under the name space prefix as well.
Just think of net.traceroute() ....., or parsing the netif interface list in lua, keeping the C binding layer as thin as possible.

How is all this handled in user_modules.h?
Maybe this goes towards a module dependency tree, but I can memorize that there were a discussion somewhere on the internet on the question of flat vs. tree like module dependencies, right?

[devsaurus]

OK, change all test to net is the second obvious thing to do.

I'd even propose to integrate ping in the net.c file. I.e. add an entry for the ping function in net_map. This is how I interpret @devyte's proposal of net.ping().

And how can I merge module maps across files?

There are advanced concepts like inheritance or overloading, but they are most likely overkill.
If you intend to implement the ping functionality in a separate C file then just reference the (non-static) function for ping from there in net_map.

Should I have a luaopen_test function?

From what I see in the snippets above you don't need it. Replacing its reference with NULL should to the trick. There are several modules like this.

NODEMCU_MODULE(TEST, "test", test_map, NULL);

And I think I 'm supposed to supply Documentation - at least in some later PR to come.

Documentation for changed or new API functions should go into the corresponding location beneath doc/en/. And it should be part of the PR for the code - we want to keep the docs consistent with the code base. Also see CONTRIBUTING.md.

[marcelstoer]

I was earlier supportive of net.ping() and I wouldn't even mind for this code going into the existing net code directly. However, since you mentioned traceroute(), if(), etc. I feel that'd be a dead end.

Pretty much every NodeMCU firmware out there includes the net module. No surprise there, right? Therefore, keeping that module tight or even shrinking it (how about net-tcp, net-udp and net-dns?) can have a significant impact for a lot of people. While I think having ping() in Lua land would be really nice, I also think that most people won't actually need it. And they wouldn't need traceroute(), if(), et.al. either.

So, I propose to create a new module called "netinfo" or so. For now the only function it'd offer is ping().

[wolfgangr]

So what aobut calling the file app/modules/net_info.c and net_info.ping() be the call from lua land?

Poor man's name space structuring, so to say :-)
This keeps the technically flat namespace as it is at the moment, but helps the human mind structure it logically. And if at some time the net gear gets reorganized, as marcelstoer proposes, there would already be a natural place for it. Until that point, alphabetical sorting will keep stuff together.

The lua bindings for lwIP netif I would place in file app/modules/net_if.c then.

Can you agree with that?

[wolfgangr]

Still struggling with git :-(((

• renamed file and function to net_info....
• removed test stuff
• wrote a documentation
• got the markdown viewer - Dok looks fine (I think...)
• due credit to the original author
• pushed the current state to my personal git account
  https://github.com/wolfgangr/nodemcu-firmware/tree/net_info-ping

So far, so good.

• luckily made a tarball of my local repo.

Tried to squash commits, following
http://www.andrewconnell.com/blog/squash-multiple-git-commits-into-one
Apparently squashed what I wanted to keep and kept what I wanted to squash :-(
Forget about squashing for the moment....

Reverted to tarball and tried a merge
https://github.com/nodemcu/nodemcu-firmware/blob/master/CONTRIBUTING.md#keeping-your-fork-in-sync

git remote add upstream https://github.com/nodemcu/nodemcu-firmware.git
git fetch upstream
git checkout dev
git merge upstream/dev

Looks like all my recent commits are lost :-(

before the merge, it looked like this:

Obviously I should not have assigned the commit "ping test" to dev???
How can I come out of this?

[wolfgangr]

http://www-cs-students.stanford.edu/~blynn/gitmagic/

Git is a version control Swiss army knife. A reliable versatile multipurpose revision control tool whose extraordinary flexibility makes it tricky to learn, let alone master.

So it's not me who is the idiot?

[wolfgangr]

hopelesly lost in GIT :-(
tried squashing commits again using git rebase -i HEAD~10
did it in smaller chunks instead of one large - seems to work fine.

but when I try to do a git rebase dev as prposed in the contribution guide, I gets laods of merge conflicts on my own files - not conflicts with other peoples work.

Looks like moving files back and forth, partially reverting to old versions, different by some whitespace only, and merging those commits upsets the automagic?

I tried a push, but received
hint: Updates were rejected because the tip of your current branch is behind its remote counterpart.

I suggest giving up the squeezing of commits in order not to ruin the stage fo work reached now.
I issued a pull request from my working online repo for my tag net_info-ping.
I hope merging this into dev is just a few keystrokes for you.

If not, please give me detailled instructions.
Sorry....

[devsaurus]

Just branch off from a current dev and add your commits. We can squash later with github's support when the PR is about to be merged.

A hint for rebase/squash safety: if unsure, do a trial of these on a separate branch and prevent your WIP from being messed up.

# start your local feature branch from dev (-b creates the new branch)
git checkout -b my_new_development dev
# add commits as you progress
git commit
...

# when you feel it's time for rebase or squash then branch off for a trial
git checkout -b just_for_a_trial
# fetch latest dev (don't "pull" it)
git fetch upstream dev
git rebase -i upstream/dev
git rebase -i HEAD~3
... 
# your feature branch wasn't touched and you can always go back to it, discard the trial and start over again
git checkout my_new_development
git branch -D just_for_a_trial
git checkout -b just_for_a_trial
...

At some point you'll know what works and what not. Just repeat the cycle and learn from the results. Your feature branch remains a safe fallback.
This is how I dry run my contributions, but I don't consider myself as a git expert - there might be more elegant ways.

[wolfgangr]

I see, there is only chance to learn git the hard way. Powerful beast, but beast, nevertheless.

But your proposal made me confident to try it again.
I did the squashing ' rebase` in small steps rather than in one large.
... until the problem hit me again.

But now I think I know (roughly) the cause of the problem:

I had marked a commit as dev right in the middle of a file reorganizing orgy, when I realized that I was in a "detached HEAD" state. (Well, now I know that I should have chosen any other branch name, then...)
So this quirky stage became my local dev reference, not the real upstream/dev

I squashed this commit out, but by calling

git rebase dev

I tried to anchor my world at this quirky intermediary local dev state from the trash bin .

To fix it , I did instead

git rebase upstream/dev

And - alas - everything worked like a charm.
merge, push and pull request without any trouble
Now I even get some formalities to my pull request and a message that automatic merge is possible.

I hope this PR is digestible for you now.

[wolfgangr]

Dear @marcelstoer
sorry for the still buggy PR :-\

There is a an untested feature in the PR:
There is a callback variant in the original sources, which allows you to register a lua function to be called upon each ping response, instead of printing to the serial console.
This function is untested. This fact ist mentioned in the doc.

While this functionality is not necessary for network diagnosis from the console, there may be users who want to write diagnosis from lua clients, e.g. http, mqtt or so.
So i'd propose not to disable it.

[vsky279]

Dear all, I can see that PR #1757 has been closed and there was no follow-up. I think it's a pity as good work has been done by @wolfgangr.

I took his code and found that it fits my simple needs. The only issue I had a problem with was the memory leak. I did not find a proper solution how to free the memory properly. I've tried to count callbacks and free the memory on the last one but it led to restart in majority of cases.
So I choose rather a workaround solution - I allocate the memory only at the first call. I think these 40 bytes are not an issue. No more memory leaks.

Please check https://github.com/vsky279/nodemcu-firmware/tree/ping, especially this commit vsky279@79c8b36.

Is it ready for PR or are there any further substantial flaws?
cc @marcelstoer

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[cs8898]

So the net.ping() ICMP Command won't come?
currently really interested in coding with LUA, but it seems that i have to switch to the C version... And use some included Implementations

[vsky279]

Ok, I have created a PR #2854. Let's see reactions whether it is accepted to be merged into dev.
Still you can use this version, test and use it.