Closed Bug 774582 Opened 7 years ago Closed 6 years ago

Unable to know if there is a user connected to Wifi tethering network

Categories

(Firefox OS Graveyard :: Wifi, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:-, tracking-b2g:backlog)

RESOLVED FIXED
2.0 S3 (6june)
blocking-basecamp -
tracking-b2g backlog

People

(Reporter: timdream, Assigned: vchang)

References

Details

(Whiteboard: [LOE:M])

Attachments

(2 files, 12 obsolete files)

29.74 KB, patch
Details | Diff | Splinter Review
1.21 KB, 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.
Blocks: 735172
(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
(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.
Need requirements analysis to see if this is blocking.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Assignee: nobody → vchang
Depends on: 735547
Besides bug 735547, is there additional work here that is necessary to surface if a user is connected to the phone via tethering?
(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.
Blocks: 735172
Whiteboard: [LOE:S]
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.
Whiteboard: [LOE:S] → [LOE:M]
Whiteboard: [LOE:M] → [LOE:M] [WebAPI:P0]
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?
(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.
Vincent, Shian-Yow, has this been fixed yet?
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
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)
We need this library to be pushed to otoro device's /system/lib. 

adb remount /system/lib
adb push libhostapd.so /system/lib
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)
Attachment #667305 - Attachment mime type: text/plain → application/octet-stream
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)
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.
(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.
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)
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 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)
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)
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
(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 on attachment 676920 [details] [diff] [review]
Updated patch, polling the connected client, v1.1

Looks good, thanks.
Attachment #676920 - Flags: review?(mrbkap) → review+
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)
r=me on libhostapd. Who do we have to talk to in order to figure out if we can get the library landed?
(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?
(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.
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)
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)
(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?
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: vchang → dlee
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)
blocking-b2g: --- → 1.4?
Dimi is working on the NFC bugs. I'll continue to fix this one.
Assignee: dlee → vchang
Flags: needinfo?(vchang)
Flags: needinfo?(dlee)
Attached patch WIP patch v1.0 (obsolete) — Splinter Review
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)
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.
Blocks: 947174
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+
Depends on: 886110
Attached patch Patch v1.1 (obsolete) — Splinter Review
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)
This patch is based on bug 886110 which we should land first.
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+
Ken

Please check if this blocks 1.4
Flags: needinfo?(kchang)
(In reply to Preeti Raghunath(:Preeti) from comment #40)
> Ken
> 
> Please check if this blocks 1.4
Yes, it is.
Blocks: b2g-WLAN-2.0
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(kchang)
Whiteboard: [LOE:M] → [LOE:M][FT:RIL]
Blocks: 961995
This doesn't fall under the QC blocking feature list & DSDS feature list. Renoming.
blocking-b2g: 1.4+ → 1.4?
blocking-b2g: 1.4? → backlog
change Comp to WIFI. Let me know if it's wrong.
Component: General → Wifi
Madai needs this feature. We have to keep this moving.
Target Milestone: --- → 1.5 S1 (9may)
Whiteboard: [LOE:M][FT:RIL] → [LOE:M]
Attached patch Patch v1.2 r=mrbkap (obsolete) — Splinter Review
Rebase the patches.
Attachment #8363516 - Attachment is obsolete: true
Attachment #8363517 - Attachment is obsolete: true
Attachment #8417938 - Flags: review+
Attached patch Patch v1.3 (obsolete) — Splinter Review
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)
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)
Attached patch Patch v1.4 (obsolete) — Splinter Review
Fix the try server errors.

https://tbpl.mozilla.org/?tree=Try&rev=5c45134d9ee9
Attachment #8419879 - Flags: review?(mrbkap)
Hi Blake, could you proceed the review? Thanks.
Flags: needinfo?(mrbkap)
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+
Flags: needinfo?(mrbkap)
Attached patch Patch v1.5 r=mrbkap (obsolete) — Splinter Review
Rebase the patch.
Attachment #8418673 - Attachment is obsolete: true
Attachment #8419879 - Attachment is obsolete: true
Attached patch Patch v1.6Splinter Review
Addressed the review comments.
Attachment #8428565 - Attachment is obsolete: true
Attached patch Part2 - fix crash in Flame v1.0 (obsolete) — Splinter Review
We need this patch to prevent crash in Flame. 
I also verify this in Nexus5 and Unagi.
Attachment #8429791 - Flags: review?(mrbkap)
Blocks: 968659
Hope that we can finish it before sprint 3 of 2.0.
Target Milestone: 2.0 S1 (9may) → 2.0 S3 (6june)
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+
Address the review comments.
Attachment #8429791 - Attachment is obsolete: true
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.