Merge lp://staging/~phablet-team/network-manager/lp1480877-wifi-rm-dup-scan-signals into lp://staging/~network-manager/network-manager/ubuntu

Proposed by Tony Espy
Status: Merged
Approved by: Mathieu Trudel-Lapierre
Approved revision: 997
Merged at revision: 998
Proposed branch: lp://staging/~phablet-team/network-manager/lp1480877-wifi-rm-dup-scan-signals
Merge into: lp://staging/~network-manager/network-manager/ubuntu
Diff against target: 202 lines (+121/-33)
2 files modified
debian/changelog (+11/-0)
debian/patches/0002-wifi-cull-the-scan-list-before-signalling-ScanDone-b.patch (+110/-33)
To merge this branch: bzr merge lp://staging/~phablet-team/network-manager/lp1480877-wifi-rm-dup-scan-signals
Reviewer Review Type Date Requested Status
Mathieu Trudel-Lapierre Needs Information
Manuel de la Peña (community) Approve
Review via email: mp+271575@code.staging.launchpad.net

Description of the change

This change fixes a problem where duplicate (2-3x) AccessPoint 'LastSeen' PropertiesChanged signals are generated each time a WiFi scan completes. This can be seen by running:

dbus-monitor --system --profile "type='signal',sender='org.freedesktop.NetworkManager'"

...and waiting for a scan to occur. You'll see 2-3 PropsChanged for each AccessPoint after a scan. From then on, you'll only see changes for AccessPoint/0 until the next scan happens.

The change makes cull_scan_list the function responsible for updating an AP's last-seen property, and ensures that scan_done_cb is the only function that calls cull_scan_list. It also removes the bss_updated_cb function, as its only purpose was to update the last-seen property and schedule yet another scanlist cull.

To post a comment you must log in.
Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

Have you discussed this with the upstream developers?

The suggested changes look reasonable, but there may be unforseen consequences to changing the code this way. Have you made sure that scan were still running in a reasonable fashion, even if a scan request failed, for example? It seems to me (from memory) that this would be one case where we might expect beacons to possibly schedule a new scan.

I much less intrusive change would be to simply remove the last-seen update on bss-updated -- it still makes sense to update last-seen for every scan results, otherwise it defeats its purpose... Still, there can be cases where scans fail, or when that value won't be up to date enough.

review: Needs Information
Revision history for this message
Tony Espy (awe) wrote :

My changes have no effect on whether or not scans are scheduled.

For the gory details see:

https://bugs.launchpad.net/ubuntu-rtm/+source/network-manager/+bug/1480877/comments/41

I checked, and the change that introduced DUP signals in first place was a location services fix ( see: 0002-wifi-cull-the-scan-list-before-signalling-ScanDone-b.patch ), which hasn't been up-streamed, so my changes aren't really directly applicable.

I've test three different phones, and a laptop running wily, and the only time bss_updated_cb is ever fired is after a scan completes. As scan_done_cb always calls cull_scan_list directly, which in turn updates all of the last-seen properties of the know AccessPoints, the removal of bss_updated_cb is basically a no-op.

The removal of the schedule_scan_list call from new_bss_cb again is also predicated on the fact that new_bss_cb only ever gets triggered when a scan happens. As it was the last function that used schedule_scanlist_cull, I removed it and the associated functions/priv member.

Revision history for this message
Manuel de la Peña (mandel) wrote :

This should not affect the location service and fixes the redundant signals.

review: Approve
Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

Have you made sure that this did not break the signal level change notifications, and that we can still see signal level changes in the indicator on desktop?

review: Needs Information
Revision history for this message
Tony Espy (awe) wrote :

If you mean, signal changes for the currently connected AP ( AccessPoint/0 ), then yes, I've verified that this doesn't break signal strength changes for the current AP.

These signal strength updates are handled by periodic_update_cb, which fires every six seconds when connected to an AP. This callback in turn calls a function periodic_update which reads the signal strength of the connected AP via NMPlaform and then sets the AP property, which fires a DBus signal.

That said, I've only verified this behavior directly on a phone (krillin stable #25) running the 0.9.10 version of this patch. If you want me to verify it on a wily desktop before we build the silo, I will do so...

Revision history for this message
Tony Espy (awe) wrote :

Just tested on my Thinkpad T410s running wily. It has an Intel WiFi adapter and is running iwlwifi.

Confirmed that DBus signal strength PropertiesChanged signals for AccessPoint/0 are still fired periodically when associated with an access point. Also confirmed that the indicator sees these signals and responds by changing the WiFi icon according to signal strength.

Testing with version 1.0.4-0ubuntu4~awe3 from my PPA:

https://launchpad.net/~awe/+archive/ubuntu/ppa/+packages

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches