[Wifi] Enable BSSID for WPS PIN

RESOLVED FIXED

Status

Firefox OS
General
--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Masashi Honma, Assigned: Masashi Honma)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Currently BSSID for WPS PIN is fixed to 'any'.

In the ordinal environment, it's no problem.
But if there are some WPS PIN available APs, our phone try to connect them until correct PIN found. It could waste some times to connect.

This patch enables BSSID to specify appropriate AP.

This patch should be landed after Bug 790231.
(Assignee)

Comment 1

6 years ago
Created attachment 661544 [details] [diff] [review]
Patch file for this bug
(Assignee)

Comment 2

6 years ago
Should I update UUID of nsIWifi.idl even if I only update it's comment ?
(Assignee)

Comment 3

6 years ago
This is the Gecko side patch of the Gaia side Pull Request #4771.
(Assignee)

Comment 4

6 years ago
Is there any comment on this ?

If you could not understand why this patch is needed, see pull request #5066 for Gaia.

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Masashi Honma from comment #2)
> Should I update UUID of nsIWifi.idl even if I only update it's comment ?

Nope no need. The uuid only has to change if the vtable of the interface changes (or a function signature changes).
Comment on attachment 661544 [details] [diff] [review]
Patch file for this bug

Do we want to default the bssid to any if the frontend doesn't pass one through?

I'm assuming that I'm only reviewing the parts of this patch that aren't covered by bug 790231 (see my comment there as well).

I'm sorry for the delay in seeing this! Please remember to CC me on bugs you file on wifi stuff!
Attachment #661544 - Flags: review+
(Assignee)

Comment 7

6 years ago
> Do we want to default the bssid to any if the frontend doesn't pass one through?

Yes.

> I'm sorry for the delay in seeing this!
> Please remember to CC me on bugs you file on wifi stuff!

OK!
Severity: normal → enhancement
(In reply to Masashi Honma from comment #7)
> > Do we want to default the bssid to any if the frontend doesn't pass one through?
> 
> Yes.

I don't see that in your patch, though?
(Assignee)

Comment 9

6 years ago
Created attachment 666280 [details] [diff] [review]
Patch file after review
(Assignee)

Comment 10

6 years ago
> I don't see that in your patch, though?

I attached new patch includes that.
And this patch includes patch for Bug 790231.
(Assignee)

Comment 11

6 years ago
The Gaia side patch is PR #5528.
Comment on attachment 666280 [details] [diff] [review]
Patch file after review

Thanks! I'm curious though -- do you think it might make more sense to pass WPS via another property on ScanResult objects? That way consumers can treat "capabilities" as "security capabilities" and won't have to filter out WPS? Looking at your gaia pull request, I'm not sure that the WPS-only case is handled correctly. Let me know what you think.
Attachment #666280 - Flags: review+
Masashi, please read https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_reviews for the next patch you attach here. It's hard to keep track of all of the bugs & patches, so not following the procedures might mean that your patches wait longer than necessary before getting committed :( It's a pain to get used to, but it makes life a lot easier for me and other reviewers.
Assignee: nobody → masashi.honma
(Assignee)

Comment 14

6 years ago
> That way consumers can treat "capabilities" as "security capabilities" and won't have to filter out WPS?

Yes. Current Gaia code use "capabilities" as "encryptions" property. So I will separate current "capabilities" property to "encryptions" property and use "capabilities" property as "WPS" flag and other flags container.

> I'm not sure that the WPS-only case is handled correctly.

You are right. When opposite AP is open encryption and WPS available, the Gaia UI will show "securedBy WPS".

In practice, there is not so much such AP (open and WPS available). So I will separate the property after this patch landed. Is this approach OK ?

> please read https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_reviews

Thank you for good information !
(In reply to Masashi Honma from comment #14)
> In practice, there is not so much such AP (open and WPS available). So I
> will separate the property after this patch landed. Is this approach OK ?

I actually saw one the other day! Sure, that's fine. I'll check this patch in today in that case. You can attach the new patch to bug 790231.
(Assignee)

Comment 16

6 years ago
> You can attach the new patch to bug 790231.

I attached new patch on bug 790231.
(Assignee)

Comment 17

6 years ago
> I'll check this patch in today in that case.

I have built an "open and WPS available AP" and test the patch (Attachment #666280 [details] [diff]) with the AP. There was only display issue. There was no connectivity issue. I could connect to the AP and browse internet.

Updated

6 years ago
Attachment #661544 - Attachment is obsolete: true
On second thought. I'd rather check this in at the same time as bug 790231 to avoid introducing a regression in order to fix it later.
Hello Masashi-san,

I'm very sorry, but I had to back out this patch :(.  The gaia pull request was not merged in time, and without this patch breaks wifi.

We need to work with kaze to get the pull request merged.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c924b386e2ea
(Assignee)

Comment 21

6 years ago
OK. I will re-write the patch for master branch, and re-send it in few hours.
(Assignee)

Comment 22

6 years ago
I re-wrote. See new PR #5697.
(Assignee)

Comment 23

5 years ago
> We need to work with kaze to get the pull request merged.

Was this patch merged ?
If no, I can take a time to write new patch.
If I wrote new patch, who can review and merge it ?

Comment 24

5 years ago
> If I wrote new patch, who can review and merge it ?
Vicent, can you please help this review?
Flags: needinfo?(vchang)
(In reply to Ken Chang from comment #24)
> > If I wrote new patch, who can review and merge it ?
> Vicent, can you please help this review?
Sure, I can help to review the new patch.
Flags: needinfo?(vchang)
(Assignee)

Comment 26

5 years ago
Vincent Chang, OK, I will re-write my patch after current work.

I have a question about Gaia/Gecko tree.
I'm building Gaia/Gecko tree by below commands.

$ git clone git://github.com/mozilla-b2g/B2G.git
$ cd B2G/
$ ./config.sh nexus-s
$ git pull
$ ./repo sync
$ ./build.sh -j8

But Gaia tree which is build by these commans is older than git master.
I can replace the older gaia tree by hand.
But I think mismatch between Gaia and Gecko interface causes some troubles.
Is there any way to get newest Gaia and appropriate Gecko tree ?
(Assignee)

Comment 27

5 years ago
> I have a question about Gaia/Gecko tree.

I will ask this to ML.
(Assignee)

Comment 28

5 years ago
Created attachment 766235 [details]
Gaia patch
(Assignee)

Updated

5 years ago
Attachment #666280 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #766235 - Attachment mime type: text/plain → text/html
Attachment #766235 - Flags: review?(vchang)
(Assignee)

Comment 29

5 years ago
Created attachment 766236 [details] [diff] [review]
Gecko Patch
Attachment #766236 - Flags: review?(vchang)
(Assignee)

Comment 30

5 years ago
I have attached two patches.

The Gaia side pull request has backward compatibility.
In other words, the Gaia code works on old and new Gecko code.
So, if you land Gaia first, you can land Gecko sefely.
After both patches are landed, I will remove the backward compatibility code from Gaia.
(Assignee)

Comment 31

5 years ago
Vincent Chang,
Is there any comment ?
Sorry for my late response, I was in a business trip to China last week.
I'll make the review done within this week. 
For the gaia part, kaze should be the right person to review it.
(Assignee)

Updated

5 years ago
Attachment #766235 - Flags: review?(vchang) → review?(kaze)
Duplicate of this bug: 790231
Hello Masashi, I’ve just closed bug 790231 because I assumed it was obsolete with your latest patches; is that OK?
The code looks good to me for the Gaia part, but I have two (possibly silly) semantic questions.

1. what you mean by “opposite” in these two messages?

>  wpsPinInput=Input {{pin}} to an opposite device
>  wpsPinAps=Select an opposite device

Does it mean “access point”, or is there a difference that I missed? I don’t want to block on UX but I’m confused by these messages…

2. why did you change the property names? (= “security” vs “capabilities” on the Gecko side, “Security” vs “Encryptions” on the Gaia side) Is that purely semantic, or is there a technical reason to rename them?

I have the impression that forward compatibility would require less code if we kept the same property names, but I might have missed the point.
(Assignee)

Comment 36

5 years ago
Fabien Cazenave,
Thanks for your review.

> I’ve just closed bug 790231

You can close it after this bug was fixed.
I will also close other related bugs and PRs after fix.

> 1. what you mean by “opposite” in these two messages?

The "opposite device" means "access point" right now.
But the "opposite device" could be "Group Owner" on the Wi-Fi Direct enabled device.
Currently B2G does not have Wi-Fi Direct functionality.
But we can use the word "opposite device" after B2G will implement the functionality.

> 2. why did you change the property names?

My purpose is to get "WPS" flag from scan result.
My first idea is putting "WPS" flag to existing "capabilities" property.
But Blake Kaplan proposed that "WPS" flag could be passed by another property.
(See https://bugzilla.mozilla.org/show_bug.cgi?id=791506#c12)
So I agreed and created a new property.

The new property name "security" was proposed by Blake Kaplan
(See https://bugzilla.mozilla.org/show_bug.cgi?id=790231#c9)
and agreed by you.
(See https://bugzilla.mozilla.org/show_bug.cgi?id=790231#c10)
So I'm using it.
Comment on attachment 766236 [details] [diff] [review]
Gecko Patch

Review of attachment 766236 [details] [diff] [review]:
-----------------------------------------------------------------

I am fine with the patch. 

But We need to be more careful before landing the patch 
because it changes the capabilities of network object.
There might be many gaia branches, not sure if we need to 
compatible with them as well.
Attachment #766236 - Flags: review?(vchang) → review+
Comment on attachment 766235 [details]
Gaia patch

(In reply to Masashi Honma from comment #36)
> The "opposite device" means "access point" right now.
> But the "opposite device" could be "Group Owner" on the Wi-Fi Direct enabled
> device.

Hmm, it still confuses me but I’ll pass the ball to the UX team and see if they want to open a follow-up.

> The new property name "security" was proposed by Blake Kaplan
> and agreed by you.

I completely forgot about that. Thanks for the clarification!
Attachment #766235 - Flags: review?(kaze) → review+
Masashi, would you please squash your commits so we can merge your patch on Gaia?
(Assignee)

Comment 40

5 years ago
Fabien Cazenave,

> Masashi, would you please squash your commits so we can merge your patch on Gaia?

Sorry for my lackness of github knowledge.

I have heared that "git push -f" command squashes commit.
But it resulted in current state.

How can I squash my patch ?
I’ll suppose “upstream” refers to mozilla-b2g/gaia, and “origin” to masap/gaia. Here’s a way to squash your commits:

> git fetch upstream
> git checkout 20130622
> git merge upstream/master
> git reset --soft upstream/master
> git commit
> git push origin 20130622 -f

Instead of doing a merge & soft reset, another way would be to rebase your patch on upstream/master.
(Assignee)

Comment 42

5 years ago
Fabien Cazenave,
Thanks. I squashed the patch.
The Gaia part is merged on master:
https://github.com/mozilla-b2g/gaia/commit/fc469532b36759037881de2bd4ecc9a23ec12756

Now the Gecko part can be applied safely.
(Assignee)

Comment 44

5 years ago
Vincent Chang,

Could you land Gecko part ?
Because I don't have land authority.
Here you are. 
https://hg.mozilla.org/projects/birch/rev/43d14cca5953

You can also enter the keyword "checkin-needed" next time. :-)
(Assignee)

Comment 46

5 years ago
> You can also enter the keyword "checkin-needed" next time. :-)

Oh, thanks.
https://hg.mozilla.org/mozilla-central/rev/43d14cca5953
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.