{"action":"created","issue":{"url":"https://api.github.com/repos/nodemcu/nodemcu-firmware/issues/1757","repository_url":"https://api.github.com/repos/nodemcu/nodemcu-firmware","labels_url":"https://api.github.com/repos/nodemcu/nodemcu-firmware/issues/1757/labels{/name}","comments_url":"https://api.github.com/repos/nodemcu/nodemcu-firmware/issues/1757/comments","events_url":"https://api.github.com/repos/nodemcu/nodemcu-firmware/issues/1757/events","html_url":"https://github.com/nodemcu/nodemcu-firmware/pull/1757","id":202472495,"number":1757,"title":"Try merge ping to upstream","user":{"login":"wolfgangr","id":3456715,"avatar_url":"https://avatars.githubusercontent.com/u/3456715?v=3","gravatar_id":"","url":"https://api.github.com/users/wolfgangr","html_url":"https://github.com/wolfgangr","followers_url":"https://api.github.com/users/wolfgangr/followers","following_url":"https://api.github.com/users/wolfgangr/following{/other_user}","gists_url":"https://api.github.com/users/wolfgangr/gists{/gist_id}","starred_url":"https://api.github.com/users/wolfgangr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wolfgangr/subscriptions","organizations_url":"https://api.github.com/users/wolfgangr/orgs","repos_url":"https://api.github.com/users/wolfgangr/repos","events_url":"https://api.github.com/users/wolfgangr/events{/privacy}","received_events_url":"https://api.github.com/users/wolfgangr/received_events","type":"User","site_admin":false},"labels":[{"id":417538195,"url":"https://api.github.com/repos/nodemcu/nodemcu-firmware/labels/new%20module","name":"new module","color":"5319e7","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":24,"created_at":"2017-01-23T08:33:41Z","updated_at":"2017-01-29T10:48:21Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/nodemcu/nodemcu-firmware/pulls/1757","html_url":"https://github.com/nodemcu/nodemcu-firmware/pull/1757","diff_url":"https://github.com/nodemcu/nodemcu-firmware/pull/1757.diff","patch_url":"https://github.com/nodemcu/nodemcu-firmware/pull/1757.patch"},"body":"Fixes #1555\r\n\r\n- [x] This PR is for the `dev` branch rather than for `master`.\r\n- [x] This PR is compliant with the [other contributing guidelines](https://github.com/nodemcu/nodemcu-firmware/blob/dev/CONTRIBUTING.md) as well (if not, please describe why).\r\n- [x] I have thoroughly tested my contribution. (see below)\r\n- [x] The code changes are reflected in the documentation at `docs/en/*`.\r\n\r\nExposing ping functionality for network diagnosis to lua land.\r\n\r\nuntested feature - I'v added a note inthe docs:\r\ncallback variant - allows you to register a lua function to be called upon each incoming ping, instead print on the serial console. \r\nSee issue #1555 for reason\r\n\r\n---\r\n\r\n**API design**\r\n- [x] Agreement on printing from C layer vs. callback -> no console output (except for debugging info)\r\n- [ ] Update of `net_info.ping()` API accordingly?\r\n\r\n**Documentation**\r\n- [ ] Reflect updated API spec in doc file\r\n- [ ] Align formatting of .md to existing files\r\n- [ ] Other proposed corrections\r\n\r\n**Implementation**\r\n- [ ] Fix callbacks from lwip context. They shall not call out to Lua but need to post tasks.\r\n- [ ] Remove caching of caller's lua state\r\n- [ ] Fix memory leak of unbalanced zallocs\r\n- [ ] Fix memory leak of Lua callback reference not unref'ed automatically\r\n- [ ] Adapt parameter parsing to API doc\r\n- [ ] Clarify uninitialized `ping_dns_lookup`\r\n"},"comment":{"url":"https://api.github.com/repos/nodemcu/nodemcu-firmware/issues/comments/275906286","html_url":"https://github.com/nodemcu/nodemcu-firmware/pull/1757#issuecomment-275906286","issue_url":"https://api.github.com/repos/nodemcu/nodemcu-firmware/issues/1757","id":275906286,"user":{"login":"devsaurus","id":10791520,"avatar_url":"https://avatars.githubusercontent.com/u/10791520?v=3","gravatar_id":"","url":"https://api.github.com/users/devsaurus","html_url":"https://github.com/devsaurus","followers_url":"https://api.github.com/users/devsaurus/followers","following_url":"https://api.github.com/users/devsaurus/following{/other_user}","gists_url":"https://api.github.com/users/devsaurus/gists{/gist_id}","starred_url":"https://api.github.com/users/devsaurus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/devsaurus/subscriptions","organizations_url":"https://api.github.com/users/devsaurus/orgs","repos_url":"https://api.github.com/users/devsaurus/repos","events_url":"https://api.github.com/users/devsaurus/events{/privacy}","received_events_url":"https://api.github.com/users/devsaurus/received_events","type":"User","site_admin":false},"created_at":"2017-01-29T10:48:21Z","updated_at":"2017-01-29T10:48:21Z","body":"> My proposal were to put ping_opt as a static.\r\n\r\nYes, that would solve the memory issue for sure.\r\n\r\n> I know this breaks reentrancy,\r\n\r\nMy feeling is that the whole ping stack is not reentrant even in the current state.\r\nAt the moment you can issue multiple pings, but they'll interfere with each other since they share global variable for function callback reference. A static ping_opt will \"just\" be another shared resource.\r\n\r\n> After that I think would be the right place to free ping_option struct.\r\n> But we don' get a callback after that any more.\r\n\r\nWhat about the call to `ping_opt->sent_function()` at the end of `ping_coarse_tmr`? It looks like a \"Last order, please\" to me. Could net_if hook in there and free dynamic memory together with unref'ing Lua callbacks? lwip/ping is quite new to me, never looked into that. So this proposal might be nonsense.\r\n"}}
{"actor":{"display_login":"devsaurus"}}