Tim Guan-tin Chien [:timdream] (please needinfo) Reporter
	Description • 12 years ago

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.

	Vincent Chang[:vchang][changyihsin] 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

	Tim Guan-tin Chien [:timdream] (please needinfo) 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.

	Dietrich Ayala (:dietrich)
	Comment 3 • 12 years ago

Need requirements analysis to see if this is blocking.

	Chris Lee [:clee]
	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?

	Vincent Chang[:vchang][changyihsin] 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.

	Vincent Chang[:vchang][changyihsin] 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.

	Shian-Yow Wu [:swu]
	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?

	Tim Guan-tin Chien [:timdream] (please needinfo) 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.

	Tim Guan-tin Chien [:timdream] (please needinfo) Reporter
	Comment 9 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] 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.

	Vincent Chang[:vchang][changyihsin] 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

	Marshall Culpepper [:marshall_law]
	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..

	Blake Kaplan (:mrbkap) (inactive)
	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 = ...

?

	Vincent Chang[:vchang][changyihsin] 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.

	Blake Kaplan (:mrbkap) (inactive)
	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.

	Vincent Chang[:vchang][changyihsin] 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

	Alex Keybl [:akeybl]
	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.

	Blake Kaplan (:mrbkap) (inactive)
	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.

	Vincent Chang[:vchang][changyihsin] 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 ?

	Blake Kaplan (:mrbkap) (inactive)
	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?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 21 • 12 years ago

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

Looks good, thanks.

	Michael Wu [:mwu]
	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.

	Blake Kaplan (:mrbkap) (inactive)
	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?

	Michael Wu [:mwu]
	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?

	Vincent Chang[:vchang][changyihsin] 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.

	Vincent Chang[:vchang][changyihsin] 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.

	Blake Kaplan (:mrbkap) (inactive)
	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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 28 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] 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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 30 • 12 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	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.

	Tim Guan-tin Chien [:timdream] (please needinfo) 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?

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 33 • 11 years ago

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

	Vincent Chang[:vchang][changyihsin] 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.

	Vincent Chang[:vchang][changyihsin] 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.

	Blake Kaplan (:mrbkap) (inactive)
	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.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 37 • 11 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 38 • 11 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	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:.

	Preeti Raghunath(:Preeti)
	Comment 40 • 11 years ago

Ken

Please check if this blocks 1.4

	Ken Chang[:kenkai]
	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.

	Jason Smith [:jsmith]
	Comment 42 • 11 years ago

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

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 43 • 11 years ago

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

	Ken Chang[:kenkai]
	Comment 44 • 11 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 45 • 11 years ago

Rebase and push to Try 

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 46 • 11 years ago

Rebase the patches.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 47 • 11 years ago

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

	Vincent Chang[:vchang][changyihsin] 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.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 49 • 11 years ago

Fix the try server errors.

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

	howie [:howie]
	Comment 50 • 10 years ago

Hi Blake, could you proceed the review? Thanks.

	Blake Kaplan (:mrbkap) (inactive)
	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.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 52 • 10 years ago

Rebase the patch.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 53 • 10 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 54 • 10 years ago

Addressed the review comments.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 55 • 10 years ago

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

	Ken Chang[:kenkai]
	Comment 56 • 10 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	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')...

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 58 • 10 years ago

Address the review comments.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 59 • 10 years ago

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

	Ed Morley [:emorley]
	Comment 60 • 10 years ago

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