Closed
Bug 774582
Opened 12 years ago
Closed 10 years ago
Unable to know if there is a user connected to Wifi tethering network
Categories
(Firefox OS Graveyard :: Wifi, defect)
Tracking
(blocking-basecamp:-, tracking-b2g:backlog)
RESOLVED
FIXED
2.0 S3 (6june)
People
(Reporter: timdream, Assigned: vchang)
References
Details
(Whiteboard: [LOE:M])
Attachments
(2 files, 12 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
https://github.com/mozilla-b2g/gaia/issues/2339 So on page 6 of the status bar spec, we would need to dim the icon if there is no user connected to the tethering network (and highlight the icon if there is). So we wound need that information exposed from API. I am told by Vincent Chang that @philikon suggested for v1 we can just get the value from a "read-only" mozSettings value.
Assignee | ||
Comment 1•12 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #0) > https://github.com/mozilla-b2g/gaia/issues/2339 > > So on page 6 of the status bar spec, we would need to dim the icon if there > is no user connected to the tethering network (and highlight the icon if > there is). So we wound need that information exposed from API. > > I am told by Vincent Chang that @philikon suggested for v1 we can just get > the value from a "read-only" mozSettings value. Updated the settings path to indicate connected users tetherhing.wifi.connectedClients => 0, 1, 2.... tethering.usb.connectedClients => 0 or 1
No longer blocks: 735172
Reporter | ||
Comment 2•12 years ago
|
||
(In reply to Vincent Chang from comment #1) > > Updated the settings path to indicate connected users > tetherhing.wifi.connectedClients => 0, 1, 2.... > tethering.usb.connectedClients => 0 or 1 Yeah that works. Thanks, I will update the Gaia code.
Reporter | ||
Updated•12 years ago
|
blocking-basecamp: --- → ?
Updated•12 years ago
|
blocking-basecamp: ? → +
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → vchang
Comment 4•12 years ago
|
||
Besides bug 735547, is there additional work here that is necessary to surface if a user is connected to the phone via tethering?
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Chris Lee [:clee] from comment #4) > Besides bug 735547, is there additional work here that is necessary to > surface if a user is connected to the phone via tethering? We may need a timer to update the number of connected users in gecko part and set it using settings API. This information can be obtained from dhcp server or wifi driver. But still need to check how to get this information in Android.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [LOE:S]
Assignee | ||
Comment 6•12 years ago
|
||
After testing netd command "softap clients", I found that netd doesn't support this command when we enable HAVE_HOSTAPD compiler flag. However, we have to enable HAVE_HOSTAPD flag to support wifi hotspot. So, we may need to obtain this information from wifi drvier or hostapd directly. Not sure if android has library to deal with the control command to hostapd. Change the [LOE] from S to M.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [LOE:S] → [LOE:M]
Updated•12 years ago
|
Whiteboard: [LOE:M] → [LOE:M] [WebAPI:P0]
Comment 7•12 years ago
|
||
The number of connected users will be reset after device reboot. It seems better to store this information in Web API instead of settings. As tethering already landed, would we reconsider to put the value in Web API?
Reporter | ||
Comment 8•12 years ago
|
||
(In reply to Shian-Yow Wu from comment #7) > The number of connected users will be reset after device reboot. It seems > better to store this information in Web API instead of settings. As > tethering already landed, would we reconsider to put the value in Web API? It's fine. Web content can just always assume these values are invalid if the tethering itself is not enabled. If tethering itself is re-enabled after reboot, it should reset these numbers.
Assignee | ||
Updated•12 years ago
|
Summary: Unable to know if there is a user connected to the tethering network → Unable to know if there is a user connected to Wifi tethering network
Assignee | ||
Comment 10•12 years ago
|
||
Hi Marshall, Since Philikon is on vacation right now, may I ask you to help to review my patch ? This patch is used to create a timer and poll the connected wifi client numbers when we turn on wifi tethering.
Attachment #667302 -
Flags: review?(marshall)
Assignee | ||
Comment 11•12 years ago
|
||
We need this library to be pushed to otoro device's /system/lib. adb remount /system/lib adb push libhostapd.so /system/lib
Comment 12•12 years ago
|
||
Comment on attachment 667302 [details] [diff] [review] Patch, polling the connected client Blake, would you mind taking the review on this? I'm not familiar with wifi code..
Attachment #667302 -
Flags: review?(marshall) → review?(mrbkap)
Updated•12 years ago
|
Attachment #667305 -
Attachment mime type: text/plain → application/octet-stream
Comment 13•12 years ago
|
||
Comment on attachment 667302 [details] [diff] [review] Patch, polling the connected client Review of attachment 667302 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/NetworkManager.js @@ +152,5 @@ > settingsLock.get(SETTINGS_USB_DHCPSERVER_STARTIP, this); > settingsLock.get(SETTINGS_USB_DHCPSERVER_ENDIP, this); > > + // Reset to zero. > + settingsLock.set(SETTINGS_WIFI_CONNECTEDCLIENTS, 0, null); It seems a little odd to me that this is a setting, but I don't know of a better way of exposing it and I'm not fully familiar with the network manager DOM interface (philikon should probably comment on this when he gets back). @@ +635,5 @@ > }, > > + _wifiClientTimerEnabled: false, > + > + _wifiClientTimer: null, Is there a reason not to use the timer to subsume the boolean? @@ +640,5 @@ > + > + checkConnectedWifiClients: function checkConnectedWifiClients() { > + let params = {}; > + params.cmd = "getSoftAPConnectedClients"; > + params.isAsync = false; let params = { cmd: "getSoftAPConnectedClients", isAsync: false }; @@ +647,5 @@ > + this.tetheringSettings[SETTINGS_WIFI_CONNECTEDCLIENTS] = data.ret; > + // Application updates status by listening to settings change. > + settingsLock.set(SETTINGS_WIFI_CONNECTEDCLIENTS, data.ret, null); > + if (this._wifiClientTimerEnabled) { > + if (this._wifiClientTimer == null) { if (!this._wifiClientTimer) ::: dom/system/gonk/net_worker.js @@ +366,5 @@ > > +function getSoftAPConnectedClients(params) { > + // Connect to hostapd. > + let connection = libhostapd.connectToHostapd(); > + if (connection < 0) { Why not make this: if (libhostapd.connectToHostapd() < 0) { ... } let connection = ... ?
Attachment #667302 -
Flags: review?(mrbkap)
Assignee | ||
Comment 14•12 years ago
|
||
Hi Blake, Thanks for your help to review this patch. NetworkManager don't have dom interface to expose information to gaia right now. Instead of creating a new API, Philikon suggested us using settings API to expose just connected user information to gaia for the time being. Not sure if we need to create a new API at this moment.
Comment 15•12 years ago
|
||
(In reply to Vincent Chang from comment #14) > NetworkManager don't have dom interface to expose information to gaia right > now. Instead of creating a new API, Philikon suggested us using settings API > to expose just connected user information to gaia for the time being. Not > sure if we need to create a new API at this moment. Okay, that makes sense. I'm fine with leaving that as is.
Assignee | ||
Comment 16•12 years ago
|
||
Hi Blake, I updated the patch to 1. Address the comment 13. 2. Check if we load the libhostapd.so successfully or not. 3. Update settings when the number of connected client is different. The initial version of libhostapd is located in https://github.com/changyihsin/libhostapd May I know how to ask for review and push new native library to mozilla-b2g in github ? Regards Vincent
Attachment #667302 -
Attachment is obsolete: true
Attachment #671353 -
Flags: review?(mrbkap)
Comment 17•12 years ago
|
||
Upon further investigation, this no longer blocks basecamp. If you'd like to still land for v1, please provide an assessment of risk (of regressions) when nominating for uplift.
blocking-basecamp: + → -
Whiteboard: [LOE:M] [WebAPI:P0] → [LOE:M]
Comment 18•12 years ago
|
||
Comment on attachment 671353 [details] [diff] [review] Updated patch, polling the connected client, v1 Review of attachment 671353 [details] [diff] [review]: ----------------------------------------------------------------- One more patch needed, I think. ::: dom/system/gonk/NetworkManager.js @@ +665,5 @@ > + > + checkConnectedWifiClients: function checkConnectedWifiClients() { > + let params = {}; > + params.cmd = "getSoftAPConnectedClients"; > + params.isAsync = false; Please initialize the parameters in the object literal as I mentioned in my last comment! @@ +673,5 @@ > + this.tetheringSettings[SETTINGS_WIFI_CONNECTEDCLIENTS] = data.ret; > + // Application updates status by listening to settings change. > + settingsLock.set(SETTINGS_WIFI_CONNECTEDCLIENTS, data.ret, null); > + } > + if (!this._wifiClientTimer) { I think this could be: if (!this._wifiClientTimer) { this._wifiClientTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); this._wifiClientTimer.initWithCallback(this, WIFI_CLIENT_CHECK_INTERVAL, Ci.nsITimer.TYPE_REPEATING_SLACK); } to avoid reinitializing the timer every time.
Attachment #671353 -
Flags: review?(mrbkap)
Assignee | ||
Comment 19•12 years ago
|
||
This patch addresses the comment 18. mrbkap, could you please also help to review the initial version of libhostapd in https://github.com/changyihsin/libhostapd I am not sure how to create native library to mozilla-b2g in github ? Can you guide me how ?
Attachment #671353 -
Attachment is obsolete: true
Attachment #676920 -
Flags: review?(mrbkap)
Assignee | ||
Updated•12 years ago
|
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Comment 20•12 years ago
|
||
(In reply to Vincent Chang[:vchang] from comment #19) > I am not sure how to create native library to mozilla-b2g in github ? Can > you guide me how ? I'm not sure either. mwu, do you know?
Flags: needinfo?(mwu)
Comment 21•12 years ago
|
||
Comment on attachment 676920 [details] [diff] [review] Updated patch, polling the connected client, v1.1 Looks good, thanks.
Attachment #676920 -
Flags: review?(mrbkap) → review+
Comment 22•12 years ago
|
||
mrbkap will need to review libhostapd. I'm not sure if we're allowed to add new modules at this point. The library needs to be added to gonk-misc/b2g.mk's product packages to build, but we might not be allowed to do that.
Flags: needinfo?(mwu)
Comment 23•12 years ago
|
||
r=me on libhostapd. Who do we have to talk to in order to figure out if we can get the library landed?
Comment 24•12 years ago
|
||
(In reply to Blake Kaplan (:mrbkap) from comment #23) > r=me on libhostapd. Who do we have to talk to in order to figure out if we > can get the library landed? gal or cjones might know. I think it's too late to add any libraries at this point, but they would know more. Why do we need to add a new library? Is this for consistency with how we talk to wifi in general?
Assignee | ||
Comment 25•12 years ago
|
||
(In reply to Michael Wu [:mwu] from comment #24) > (In reply to Blake Kaplan (:mrbkap) from comment #23) > > r=me on libhostapd. Who do we have to talk to in order to figure out if we > > can get the library landed? > > gal or cjones might know. I think it's too late to add any libraries at this > point, but they would know more. > > Why do we need to add a new library? Is this for consistency with how we > talk to wifi in general? Yes, that's right, and Android doesn't provide native library which can be used to talk to hostapd.
Assignee | ||
Comment 26•12 years ago
|
||
Hi Chris, Can we add a new native library to gonk at this point ? The library is used as a interface to talk to hostapd. We need to talk to hostapd and get information about number of connected wifi-clients.
Flags: needinfo?(jones.chris.g)
Comment 27•12 years ago
|
||
One thing to note is that if it is too late to add the library, we can also just expose this as an XPCOM component instead of ctypes.
(In reply to Vincent Chang[:vchang] from comment #26) > Hi Chris, > > Can we add a new native library to gonk at this point ? The library is used > as a interface to talk to hostapd. We need to talk to hostapd and get > information about number of connected wifi-clients. That would be somewhat painful. Why can't we add this code to gecko directly?
Flags: needinfo?(jones.chris.g)
Assignee | ||
Comment 29•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #28) > (In reply to Vincent Chang[:vchang] from comment #26) > > Hi Chris, > > > > Can we add a new native library to gonk at this point ? The library is used > > as a interface to talk to hostapd. We need to talk to hostapd and get > > information about number of connected wifi-clients. > > That would be somewhat painful. > > Why can't we add this code to gecko directly? This code uses libwpa_client library in wpa_supplicant, and the implementation of libwpa_client seems using blocking I/O(send/recv/select). Although we use the library in net_worker thread, it should be better to use non-blocking I/O just like what we are doing for ril and netd, right ? If the answer is yes, should we also modify the IPC mechanism in wifi worker ? It also uses libwpa_client to do IPC with wpa_supplicant.
I don't quite understand how this new library would be used then. Are you modifying wpa_supplicant as well?
Comment 31•12 years ago
|
||
Hey Vincent, I don't understand what your concern is. It should be fine to use blocking IO so long as it's off the main thread (background threads can block for as long as they'd like). There should be no problem with adding a Gecko component, connected via the SystemWorkerManager (almost exactly like what we do for wifi) that exposes the same thing as your ctypes library.
Assignee | ||
Updated•12 years ago
|
Assignee: vchang → dlee
Reporter | ||
Comment 32•11 years ago
|
||
Hi all, This issue is somehow being forgotten for almost a year. Could we make this work in the next release?
Flags: needinfo?(vchang)
Flags: needinfo?(dlee)
Reporter | ||
Updated•11 years ago
|
blocking-b2g: --- → 1.4?
Assignee | ||
Comment 33•11 years ago
|
||
Dimi is working on the NFC bugs. I'll continue to fix this one.
Assignee: dlee → vchang
Flags: needinfo?(vchang)
Updated•11 years ago
|
Flags: needinfo?(dlee)
Assignee | ||
Comment 34•11 years ago
|
||
1. Use libwpa_client to create ipc between hostapd and gecko. 2. create a timer to poll the connection of hotspot.
Attachment #667305 -
Attachment is obsolete: true
Attachment #676920 -
Attachment is obsolete: true
Attachment #8357567 -
Flags: feedback?(mrbkap)
Assignee | ||
Comment 35•11 years ago
|
||
This WIP patch works fine on unagi. However, instead of using hostapd daemon, buri uses wpa_supplicant when turn on hotspot in netd, buri also provides a netd command "AP_GET_STA_LIST" to query the number of connected stations.
Comment 36•11 years ago
|
||
Comment on attachment 8357567 [details] [diff] [review] WIP patch v1.0 Review of attachment 8357567 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good overall. I have some nits and comments below, but no red flags. ::: dom/wifi/WifiHotspotUtils.cpp @@ +50,5 @@ > +{ > + char *cfile; > + int32_t flen; > + > + if (ifname == nullptr) { Here and below, please prefer |if (!varname)| to explicit comparisons to nullptr. @@ +59,5 @@ > + cfile = (char*)malloc(flen); > + if (cfile == nullptr) { > + return nullptr; > + } > + snprintf(cfile, flen, "%s/%s", ctrl_iface_dir, ifname); Why not simply use nsPrintfCString here instead of rolling your own? If you must do this, I'd prefer using |new char[flen]| and an nsAutoArrayPtr<char>. @@ +69,5 @@ > +} > + > +int32_t > +WifiHotspotUtils::sendCommand(struct wpa_ctrl *ctrl, const char *cmd, > + char *reply, size_t *reply_len) Nit: Please indent the second line so that the 'c' lines up with the first 's' in "struct". ::: dom/wifi/WifiHotspotUtils.h @@ +38,5 @@ > +// Set up a dlsymed function ready to use. > +#define USE_DLFUNC(name) \ > + FUNC##name name = (FUNC##name) dlsym(GetSharedLibrary(), #name); \ > + if (!name) { \ > + MOZ_ASSUME_UNREACHABLE("Symbol not found in shared library : " #name); \ Uber nit: the \ at the end of this line needs to line up with the other \s. ::: dom/wifi/WifiWorker.js @@ +931,5 @@ > + if (wifiHotspotStatusTimer) { > + wifiHotspotStatusTimer.cancel(); > + wifiHotspotStatusTimer = null; > + } > + }; Nit (here and below): no need for the semicolons after function declarations. @@ +964,5 @@ > manager.tetheringState = "UNINITIALIZED"; > } else { > manager.tetheringState = "COMPLETED"; > + wifiCommand.connectToHostapd(function(result) { > + if (result) return; Nit: return on its own line.
Attachment #8357567 -
Flags: feedback?(mrbkap) → feedback+
Assignee | ||
Comment 37•11 years ago
|
||
1. Address the review comments. 2. Add DOM API to update connected station number.
Attachment #8357567 -
Attachment is obsolete: true
Attachment #8363516 -
Flags: review?(mrbkap)
Comment 39•11 years ago
|
||
Comment on attachment 8363516 [details] [diff] [review] Patch v1.1 Review of attachment 8363516 [details] [diff] [review]: ----------------------------------------------------------------- Two more small comments... r=me with them addressed. ::: dom/wifi/WifiHotspotUtils.cpp @@ +164,5 @@ > + > + if (sendCommand(ctrl_conn, "STA-FIRST", addr, &addrLen)) { > + return 0; > + } > + stations++; Might as well declare stations here, initialized to 1. ::: dom/wifi/WifiHotspotUtils.h @@ +23,5 @@ > + int32_t do_wifi_hostapd_command(const char *command, > + char *reply, > + size_t *reply_len); > + int32_t do_wifi_hostapd_get_stations(); > +private: Uber-nit: blank line before private:.
Attachment #8363516 -
Flags: review?(mrbkap) → review+
Comment 41•11 years ago
|
||
(In reply to Preeti Raghunath(:Preeti) from comment #40) > Ken > > Please check if this blocks 1.4 Yes, it is.
Updated•11 years ago
|
Whiteboard: [LOE:M] → [LOE:M][FT:RIL]
Comment 42•11 years ago
|
||
This doesn't fall under the QC blocking feature list & DSDS feature list. Renoming.
blocking-b2g: 1.4+ → 1.4?
Updated•11 years ago
|
blocking-b2g: 1.4? → backlog
Comment 44•11 years ago
|
||
Madai needs this feature. We have to keep this moving.
Target Milestone: --- → 1.5 S1 (9may)
Updated•11 years ago
|
Whiteboard: [LOE:M][FT:RIL] → [LOE:M]
Assignee | ||
Comment 45•11 years ago
|
||
Rebase and push to Try https://tbpl.mozilla.org/?tree=Try&rev=eaed3834fc55
Assignee | ||
Comment 46•11 years ago
|
||
Rebase the patches.
Attachment #8363516 -
Attachment is obsolete: true
Attachment #8363517 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8417938 -
Flags: review+
Assignee | ||
Comment 47•11 years ago
|
||
1. Add nsIDOMMozWifiStationInfoEvent.idl. 2. Add test case in test_all_synthetic_events.html and test_interfaces.html.
Attachment #8417938 -
Attachment is obsolete: true
Attachment #8418673 -
Flags: review?(mrbkap)
Assignee | ||
Comment 48•11 years ago
|
||
Comment on attachment 8418673 [details] [diff] [review] Patch v1.3 Review of attachment 8418673 [details] [diff] [review]: ----------------------------------------------------------------- I am fixing the trying server error. Cancel the review request for now.
Attachment #8418673 -
Flags: review?(mrbkap)
Assignee | ||
Comment 49•11 years ago
|
||
Fix the try server errors. https://tbpl.mozilla.org/?tree=Try&rev=5c45134d9ee9
Attachment #8419879 -
Flags: review?(mrbkap)
Comment 51•10 years ago
|
||
Comment on attachment 8419879 [details] [diff] [review] Patch v1.4 Review of attachment 8419879 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/WifiHotspotUtils.cpp @@ +102,5 @@ > + DIR *dir = opendir(ctrl_iface_dir); > + if (dir) { > + while ((dent = readdir(dir))) { > + if (strcmp(dent->d_name, ".") == 0 || > + strcmp(dent->d_name, "..") == 0) { Nit: the |strcmp|s should line up. @@ +105,5 @@ > + if (strcmp(dent->d_name, ".") == 0 || > + strcmp(dent->d_name, "..") == 0) { > + continue; > + } > + NS_WARNING(nsPrintfCString("Selected interface '%s'", dent->d_name).get()); This seems like it should be removed (it probably isn't worth warning for something that we expect to happen). ::: dom/wifi/WifiHotspotUtils.h @@ +23,5 @@ > + int32_t do_wifi_hostapd_command(const char *command, > + char *reply, > + size_t *reply_len); > + int32_t do_wifi_hostapd_get_stations(); > +private: Please add a blank line above this.
Attachment #8419879 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 52•10 years ago
|
||
Rebase the patch.
Attachment #8418673 -
Attachment is obsolete: true
Attachment #8419879 -
Attachment is obsolete: true
Assignee | ||
Comment 53•10 years ago
|
||
Try server result... https://tbpl.mozilla.org/?tree=Try&rev=f3e31ddcf21b
Assignee | ||
Comment 54•10 years ago
|
||
Addressed the review comments.
Attachment #8428565 -
Attachment is obsolete: true
Assignee | ||
Comment 55•10 years ago
|
||
We need this patch to prevent crash in Flame. I also verify this in Nexus5 and Unagi.
Attachment #8429791 -
Flags: review?(mrbkap)
Comment 56•10 years ago
|
||
Hope that we can finish it before sprint 3 of 2.0.
Target Milestone: 2.0 S1 (9may) → 2.0 S3 (6june)
Comment 57•10 years ago
|
||
Comment on attachment 8429791 [details] [diff] [review] Part2 - fix crash in Flame v1.0 Review of attachment 8429791 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/WifiHotspotUtils.cpp @@ +83,5 @@ > + if (strncmp(cmd, "STA-FIRST", 9) == 0 || > + strncmp(cmd, "STA-NEXT", 8) == 0) { > + char *pos = reply; > + > + if (memcmp(reply, "FAIL", 4) == 0) { This can't happen, right? If reply was "FAIL", then we already would have returned early above. @@ +87,5 @@ > + if (memcmp(reply, "FAIL", 4) == 0) { > + return -1; > + } > + > + while (*pos != '\0' && *pos != '\n') I'd probably write while (*pos && *pos != '\n')...
Attachment #8429791 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 58•10 years ago
|
||
Address the review comments.
Attachment #8429791 -
Attachment is obsolete: true
Assignee | ||
Comment 59•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/11696ba7ddcb https://hg.mozilla.org/integration/b2g-inbound/rev/9fc43c5c8ee1
Comment 60•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/11696ba7ddcb https://hg.mozilla.org/mozilla-central/rev/9fc43c5c8ee1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•