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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 1

[Vincent Chang[:vchang][changyihsin]]

(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

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 3

[Dietrich Ayala (:dietrich)]

Need requirements analysis to see if this is blocking.

Comment 4

[Chris Lee [:clee]]

Besides bug 735547, is there additional work here that is necessary to surface if a user is connected to the phone via tethering?

Comment 5

[Vincent Chang[:vchang][changyihsin]]

(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.

Comment 6

[Vincent Chang[:vchang][changyihsin]]

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.

Comment 7

[Shian-Yow Wu [:swu]]

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?

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Vincent, Shian-Yow, has this been fixed yet?

Comment 10

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch, polling the connected client

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.

Comment 11

[Vincent Chang[:vchang][changyihsin]]

Attachment: For test: Native library used to communicate to hostapd.

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

[Marshall Culpepper [:marshall_law]]

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..

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

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 = ...

?

Comment 14

[Vincent Chang[:vchang][changyihsin]]

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

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 16

[Vincent Chang[:vchang][changyihsin]]

Attachment: Updated patch, polling the connected client, v1

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

Comment 17

[Alex Keybl [:akeybl]]

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.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 19

[Vincent Chang[:vchang][changyihsin]]

Attachment: Updated patch, polling the connected client, v1.1

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 ?

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(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?

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 676920 [details] [diff] [review]
Updated patch, polling the connected client, v1.1

Looks good, thanks.

Comment 22

[Michael Wu [:mwu]]

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.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

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

[Michael Wu [:mwu]]

(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?

Comment 25

[Vincent Chang[:vchang][changyihsin]]

(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.

Comment 26

[Vincent Chang[:vchang][changyihsin]]

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.

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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?

Comment 29

[Vincent Chang[:vchang][changyihsin]]

(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.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't quite understand how this new library would be used then.  Are you modifying wpa_supplicant as well?

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 32

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Hi all,

This issue is somehow being forgotten for almost a year. 
Could we make this work in the next release?

Comment 33

[Vincent Chang[:vchang][changyihsin]]

Dimi is working on the NFC bugs. I'll continue to fix this one.

Comment 34

[Vincent Chang[:vchang][changyihsin]]

Attachment: WIP patch v1.0

1. Use libwpa_client to create ipc between hostapd and gecko. 
2. create a timer to poll the connection of hotspot.

Comment 35

[Vincent Chang[:vchang][changyihsin]]

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

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 37

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.1

1. Address the review comments. 
2. Add DOM API to update connected station number.

Comment 38

[Vincent Chang[:vchang][changyihsin]]

Attachment: convert to webidl patch in bug 886110

This patch is based on bug 886110 which we should land first.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

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:.

Comment 40

[Preeti Raghunath(:Preeti)]

Ken

Please check if this blocks 1.4

Comment 41

[Ken Chang[:kenkai]]

(In reply to Preeti Raghunath(:Preeti) from comment #40)
> Ken
> 
> Please check if this blocks 1.4
Yes, it is.

Comment 42

[Jason Smith [:jsmith]]

This doesn't fall under the QC blocking feature list & DSDS feature list. Renoming.

Comment 43

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

change Comp to WIFI. Let me know if it's wrong.

Comment 44

[Ken Chang[:kenkai]]

Madai needs this feature. We have to keep this moving.

Comment 45

[Vincent Chang[:vchang][changyihsin]]

Rebase and push to Try 

https://tbpl.mozilla.org/?tree=Try&rev=eaed3834fc55

Comment 46

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.2 r=mrbkap

Rebase the patches.

Comment 47

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.3

1. Add nsIDOMMozWifiStationInfoEvent.idl. 
2. Add test case in test_all_synthetic_events.html and test_interfaces.html.

Comment 48

[Vincent Chang[:vchang][changyihsin]]

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.

Comment 49

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.4

Fix the try server errors.

https://tbpl.mozilla.org/?tree=Try&rev=5c45134d9ee9

Comment 50

[howie [:howie]]

Hi Blake, could you proceed the review? Thanks.

Comment 51

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 52

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.5 r=mrbkap

Rebase the patch.

Comment 53

[Vincent Chang[:vchang][changyihsin]]

Try server result...
https://tbpl.mozilla.org/?tree=Try&rev=f3e31ddcf21b

Comment 54

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.6

Addressed the review comments.

Comment 55

[Vincent Chang[:vchang][changyihsin]]

Attachment: Part2 - fix crash in Flame v1.0

We need this patch to prevent crash in Flame. 
I also verify this in Nexus5 and Unagi.

Comment 56

[Ken Chang[:kenkai]]

Hope that we can finish it before sprint 3 of 2.0.

Comment 57

[Blake Kaplan (:mrbkap) (inactive)]

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')...

Comment 58

[Vincent Chang[:vchang][changyihsin]]

Attachment: Part2 - fix crash in Flame v1.1

Address the review comments.

Comment 59

[Vincent Chang[:vchang][changyihsin]]

https://hg.mozilla.org/integration/b2g-inbound/rev/11696ba7ddcb
https://hg.mozilla.org/integration/b2g-inbound/rev/9fc43c5c8ee1

Comment 60

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/11696ba7ddcb
https://hg.mozilla.org/mozilla-central/rev/9fc43c5c8ee1