Opened 4 years ago

Closed 4 years ago

Last modified 9 months ago

#1610 closed enhancement (fixed)

Alternative to Create a new wireless network.

Reported by: reuben Owned by: erikos
Priority: Normal Milestone:
Component: Sugar Version: 0.84.x
Severity: Unspecified Keywords: r+
Cc: reuben, tomeu, sascha_silbe Distribution/OS: Unspecified
Bug Status: Assigned

Description

Restore "Mesh Network" Icons in Neighborhood view to ease creation of ad-hoc networks.

See conversation thread here: http://lists.laptop.org/pipermail/devel/2009-December/026831.html

Attachments (12)

adhoc_default_networks_1406.patch (32.3 KB) - added by erikos 4 years ago.
Patch with autoconnection if there is no known network
ad_hoc_artwork.patch (7.4 KB) - added by erikos 4 years ago.
The icons, the channels are represented using the maya numerals.
adhoc_default_networks_2306.patch (37.5 KB) - added by erikos 4 years ago.
New patch that fixes the adhoc icons on the XO1.0, we use the maya numeral icons here too
adhoc_default_networks_2306-2.patch (37.7 KB) - added by erikos 4 years ago.
Fixed another little bug
0001-adhoc-master.patch (37.3 KB) - added by erikos 4 years ago.
Patch against master
0002-adhoc-master.patch (37.4 KB) - added by erikos 4 years ago.
Moved the autoconnect to a method, it is called when the networks are up
0001-Add-default-Ad-hoc-networks-1610.patch (38.9 KB) - added by erikos 4 years ago.
New 'git fomat-patch'-ed version with a fix for the "Error getting the access point properties: org.freedesktop.DBus.Error.UnknownMethod: Method "GetAll" with signature "s" on interface "org.freedesktop.DBus.Properties" doesn't exist" error and another fix for a typo that caused weired behavior
0002-Removed-another-blocking-call.-And-set-the-IP4Config.patch (2.8 KB) - added by erikos 4 years ago.
Removed another blocking call. Set IP4Config mode to 'link-local', 'shared' slipped in from testing.
0001-Add-default-Ad-hoc-networks-1610.2.patch (39.4 KB) - added by erikos 4 years ago.
Merged two patches and fixed a typo
0001-Add-default-Ad-hoc-networks-1610.3.patch (43.0 KB) - added by erikos 4 years ago.
New patch which addresses the review comments and uses a gconf option to determine whether to show the sugar adhoc networks instead of checking for the mesh device
0001-Add-default-Ad-hoc-networks-1610-4.patch (44.9 KB) - added by erikos 4 years ago.
Reworked patch based on the review comments
0001-Add-default-Ad-hoc-networks-1610-5.patch (44.9 KB) - added by erikos 4 years ago.
fixed a typo

Download all attachments as: .zip

Change History (43)

comment:1 Changed 4 years ago by erikos

  • Owner changed from dsd to erikos
  • Status changed from new to assigned

Changed 4 years ago by erikos

Patch with autoconnection if there is no known network

comment:2 Changed 4 years ago by erikos

  • Bug Status changed from Unconfirmed to Assigned
  • Keywords r? added

The above patch does the following:

  • when there is no mesh hardware we add three icons (channel 1,6,11) to the mesh view
  • after sugar is started up and after an timeout (30 secs) we see no "known" network we do autoconnect to an Ad-hoc network
  • first we try if there is an Ad-hoc network that is used by other learners in the area, if not we default to channel 1
  • the adhoc icons only have the stroke color by default, the fill color indicates that there are other learners using the adhoc network (the icon does change accordingly)

comment:3 Changed 4 years ago by erikos

  • The patch is against 0.84. Of course we should land it in master, too. It should be applyable with small modifications to master. Once we are happy with the general approach I will do the master branch patch so that we are able to land it there, too.
  • There is artwork needed for this patch. We have agreed on some new icons to represent the Ad-hoc network icons (the channels are represented using the maya numerals).

Changed 4 years ago by erikos

The icons, the channels are represented using the maya numerals.

Changed 4 years ago by erikos

New patch that fixes the adhoc icons on the XO1.0, we use the maya numeral icons here too

Changed 4 years ago by erikos

Fixed another little bug

comment:4 follow-up: Changed 4 years ago by sayamindu

How will this behave in machines which have both mesh support and can do ad-hoc ? (sorry if I've missed any prior discussion on this)

comment:5 Changed 4 years ago by tomeu

Looks great to me, will review in detail when a patch for master is proposed.

Something that has attracted my attention is that the code talks about automatically connecting to an ad-hoc connection in some circumstances, but we only autoconnect to a network that has been named as Sugar does, may be good to make this clearer.

Also, have we considered translating the SSID of the network?

Changed 4 years ago by erikos

Patch against master

comment:6 Changed 4 years ago by tomeu

  • Cc tomeu added

comment:7 in reply to: ↑ 4 ; follow-up: Changed 4 years ago by erikos

Replying to sayamindu:

How will this behave in machines which have both mesh support and can do ad-hoc ? (sorry if I've missed any prior discussion on this)

So, on machines like the XO-1.0 we will use the mesh as the hardware allows to. On the XO-1.5 we will present the user with the three adhoc networks, as there is no mesh hardware available. The XO-1.5 and XO-1.0 will see adhoc networks in his neighborhood view, so it the XO-1.0 can connect to an adhoc network that has been created by a learner on the XO-1.5. What we removed is the option in the device palette to create adhoc networks, as we have now standard ways of creating networks without infrastructure.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by sascha_silbe

  • Cc sascha_silbe added

Replying to erikos:

So, on machines like the XO-1.0 we will use the mesh as the hardware allows to. On the XO-1.5 we will present the user with the three adhoc networks, as there is no mesh hardware available. The XO-1.5 and XO-1.0 will see adhoc networks in his neighborhood view, so it the XO-1.0 can connect to an adhoc network that has been created by a learner on the XO-1.5. What we removed is the option in the device palette to create adhoc networks, as we have now standard ways of creating networks without infrastructure.

Do I understand correctly that in order to let XO-1s collaborate with XO-1.5s now an XO-1.5 user needs to initiate the connection as there's no way for the XO-1 user to create an ad-hoc network? If so, that seems cumbersome and confusing to me.

comment:9 Changed 4 years ago by erikos

The testing plans are updated with pictures, to help illustrate the new behavior: http://wiki.sugarlabs.org/go/0.90/Testing#Testing_plans

comment:10 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by erikos

Replying to sascha_silbe:

Replying to erikos:

So, on machines like the XO-1.0 we will use the mesh as the hardware allows to. On the XO-1.5 we will present the user with the three adhoc networks, as there is no mesh hardware available. The XO-1.5 and XO-1.0 will see adhoc networks in his neighborhood view, so it the XO-1.0 can connect to an adhoc network that has been created by a learner on the XO-1.5. What we removed is the option in the device palette to create adhoc networks, as we have now standard ways of creating networks without infrastructure.

Do I understand correctly that in order to let XO-1s collaborate with XO-1.5s now an XO-1.5 user needs to initiate the connection as there's no way for the XO-1 user to create an ad-hoc network? If so, that seems cumbersome and confusing to me.

Yeah true, I have overseen that point. Thanks for the comment. We concluded to remove the option from the palette as it might be confusing to the learner to have so many options. And in my opinion, once you have the three default adhoc networks there is not really a need to be able to create your 'own' new adhoc network (machines without mesh hardware). And we do not want to populate the neighborhood view on the XO-1.0 with three mesh icons and three default adhoc icons. Not sure yet, how/if we can do better here, suggestions welcome.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 4 years ago by sascha_silbe

Replying to erikos:

And we do not want to populate the neighborhood view on the XO-1.0 with three mesh icons and three default adhoc icons. Not sure yet, how/if we can do better here, suggestions welcome.

Then let's add a gconf switch to choose between Mesh/Ad-hoc/Both with the default set to Ad-hoc. AIUI the XO-1 mesh mode is not interoperable with any other device because it's based on an obsolete draft of IEEE 802.11s, so let's make it a conscious choice of deployers (or users) to enable mesh support. If IEEE 802.11s ever becomes a standard (it's still in the draft phase) and devices supporting the standard become widespread we can reconsider the default.

comment:12 Changed 4 years ago by erikos

  • Milestone changed from 0.84 to 0.90

comment:13 Changed 4 years ago by tomeu

  • Keywords r! added; r? removed

Not working here because when instantiating AdHocManager() in meshbox.py:1366, AdHocManager._autoconnect_adhoc() is executed when self._networks is still empty and thus raises an exception.

I have run into this code path because AdHocManager._have_configured_connections() returned False.

Changed 4 years ago by erikos

Moved the autoconnect to a method, it is called when the networks are up

comment:14 Changed 4 years ago by tomeu

  • Keywords r? added; r! removed

comment:15 Changed 4 years ago by tomeu

  • Keywords r! added; r? removed

Two issues after quick testing:

  • When I start sugar in sugar-emulator after applying the patches, it makes NM reconnect to the AP, interrupting any open connections. This is in NM's log:

Jul 14 11:07:40 tresne NetworkManager[1330]: <info> (wlan0): device state change: 8 -> 3 (reason 0)
Jul 14 11:07:40 tresne NetworkManager[1330]: <info> (wlan0): deactivating device (reason: 0).

  • Shell startup is blocked for several seconds and this is written in the logs:

1279098480.666589 ERROR root: Error getting the access point properties: org.freedesktop.DBus.Error.UnknownMethod: Method "GetAll" with signature "s" on interface "org.freedesktop.DBus.Properties" doesn't exist

Apart from the actual error, this makes me think that all/most D-Bus calls should be made asynchronously to void locking up the whole UI.

Would be good if you could get someone else to test this patch before submitting for review again, because this patch requires quite a bit of integration work.

Also, attach patches created with git format-patch.

comment:16 Changed 4 years ago by erikos

Tomeu, thanks for your comments.

I think the first issue you are describing has nothing to do with the patch itself. When you use sugar-emulator and have nm-applet running at the same time they will interfere with each other. For example: You are connected to the wireless network X with nm-applet and try to connect to the wireless network Y in the sugar emulator: nm-applet will disconnect from it's previous connection.

If now (with the patches applied), you have no wireless connection set in sugar (.sugar/X/nm/connections.cfg) after a timeout sugar will try to connect to an adhoc network, hence interrupt the open connection you had with nm-applet. As sugar and nm-applet are not synced for the connections this is what you will see.

About the blocking of the UI: the dbus call you are mentioning that give out the error message is asynchronous actually, there are handlers specified. I will have a look why the error message gets called in the first place, not sure yet.

Changed 4 years ago by erikos

New 'git fomat-patch'-ed version with a fix for the "Error getting the access point properties: org.freedesktop.DBus.Error.UnknownMethod: Method "GetAll" with signature "s" on interface "org.freedesktop.DBus.Properties" doesn't exist" error and another fix for a typo that caused weired behavior

comment:17 Changed 4 years ago by erikos

  • Keywords r? added; r! removed

The patch above does not solve the issue for sugar-emulator users. This is a general issue not introduced by the patch though. I will see if I can find more people to try it out and I will do tests on several machines myself.

Changed 4 years ago by erikos

Removed another blocking call. Set IP4Config mode to 'link-local', 'shared' slipped in from testing.

comment:18 Changed 4 years ago by erikos

Here are two RPMs against 0.84 for testing. Myself, I tested on OLPC build 850 with two XO-1.5 so far.

http://dev.laptop.org/~erikos/sugar-0.84.22-2.fc11.i586.rpm

http://dev.laptop.org/~erikos/sugar-artwork-0.84.2-2.fc11.i586.rpm

comment:19 in reply to: ↑ 11 Changed 4 years ago by garycmartin

Replying to sascha_silbe:

Replying to erikos:

And we do not want to populate the neighborhood view on the XO-1.0 with three mesh icons and three default adhoc icons. Not sure yet, how/if we can do better here, suggestions welcome.

Then let's add a gconf switch to choose between Mesh/Ad-hoc/Both with the default set to Ad-hoc. AIUI the XO-1 mesh mode is not interoperable with any other device because it's based on an obsolete draft of IEEE 802.11s, so let's make it a conscious choice of deployers (or users) to enable mesh support. If IEEE 802.11s ever becomes a standard (it's still in the draft phase) and devices supporting the standard become widespread we can reconsider the default.

Were any gconf switches added? I have two XO-1s here with OLPC build 850, is there a way for me to use them to test the new adhoc behaviour?

comment:20 Changed 4 years ago by erikos

Gary there is no gconf switch, yet. The code does the following, it checks whether a mesh device is available, if not create the Ad-hoc networks. If a XO-1.0 deployment wants to use the Ad-hoc networks they can disable the device (init script) for example.

But yes, maybe a gconf option might be even better. Would make the code simpler as we do not have to deal with timeouts for checking of the existence of the device.

For now for testing, just set line 1142 in meshbox.py to 'False' (with the rpms I posted above) in order to test it on XO-1.0 the Ad-hoc networks (you should see the Ad-hoc and mesh icons then).

Changed 4 years ago by erikos

Merged two patches and fixed a typo

comment:21 Changed 4 years ago by tomeu

  • Keywords r! added; r? removed
+        if self._mode == network.NM_802_11_MODE_ADHOC and \
+                self._name.startswith('Ad-hoc Network'):

We are doing this check in lots of places, and that string is a bit easy to misstype because of the hyphen, caps, etc. Could we have a constant instead or maybe better, a global function?

+    def __init__(self, manager, channel):

If manager is a singleton, why pass it around and have references to it? Just have a get_instance() function in the module and call it whenever it's needed.

+        self._palette = self._create_palette()
+        self.set_palette(self._palette)

Would be great if we could create the palette only when it's needed.

+        _palette = palette.Palette(_("Ad-hoc Network %d") % self._channel,
+                                   icon=self._palette_icon)

PEP-08 says to avoid name clashes by appending an underscore, not by prepending it.

+                try:
+                    frequency = properties['Frequency']
+                    channel = network.frequency_to_channel(frequency)
+                    if self._channel == channel:
+                        self._active = True
+                    else:
+                        self._active = False
+                except KeyError:
+                    logging.debug("Error getting the Frequency.")

This looks dangerous to me, in which cases do you anticipate getting a KeyError?

+        """If a new adhoc network is announcd that is named like an Ad-hoc
+        network we take this as an indication that other learners are
+        using the network an indicate this.
+
+        """

Docstrings should be answers to the question "what does this module/class/method does?". Same for other docstrings in the patch.

+    def indicate_population(self, state):

indicate_population is a bit disconcerting?

+        if state == True:

"if state:" is enough, but then seems like we need a better name than "state".

+    def _add_adhoc_networks(self, device):

Should be named as a callback?

+            self._check_mesh_source = gobject.timeout_add( \

Better use gobject.timeout_add_seconds to group timeouts and reduce wakeups.

+        if self._adhoc_manager != None and \

"is not None"

+            if old_hash is None: # new Ad-hoc network finished initializing

This looks like a bit too magic to me.

+                self._adhoc_manager.add_access_point(ap)

The view is updating the model? The expected is that the model emits updates and the view reacts to those updates.

 class MeshBox(gtk.VBox):

This may be a good opportunity to remove one more bogus reference to "Mesh".

+    def __init__(self, icon, access_point):
+        self.icon = icon
+        self.access_point = access_point

The model shouldn't reference the view, only the other way around.

+    timeout = 30

Should be in caps because it's a constant and would be good if the name implied what kind of timeout it is (_AUTOCONNECT_TIMEOUT?).

+        self.channels = [1, 6, 11]

Another constant?

+        logging.debug('Failed to create Ad-hoc network: %s', err)

logging.error?

+        if ' 1' == access_point.name[-2:]:

Considered using str.endswith()?

+            try:
+                self.channel = frequency_to_channel(properties['Frequency'])
+            except KeyError:
+                self.channel = 1

What do you expect could emit KeyError here?

Changed 4 years ago by erikos

New patch which addresses the review comments and uses a gconf option to determine whether to show the sugar adhoc networks instead of checking for the mesh device

comment:22 follow-up: Changed 4 years ago by tomeu

        <long>>If TRUE, Sugar will show default Ad-hoc networks for channel 1,6 and 11</long> 

One > too much, maybe we should mention we also create automatically an ad-hoc network, not just display them.

+            try:
+                channel = network.frequency_to_channel(self._frequency)
+            except KeyError:
+                channel = 1

If you really want to use an exception here, then create a new one because KeyError can be raised because of several reasons. But probably frequency_to_channel() should just print a warning and return 1 if the frequency is not known.

+                if connection:

is not None

+                        connection.set_connected()

connection.connect() may be more in line with the existing code.

+        self._bus = dbus.SystemBus()
+        self._manager = adhoc.get_manager()

No big deal, but if these aren't part of the sate of the class, then it would be better to just get references to them whenever we need it instead of caching the references.

+        'ap-added': (gobject.SIGNAL_RUN_FIRST, gobject.TYPE_NONE,

"ap" may not be obvious to all the future readers of this code.

+    _SHOW_ADHOC = '/desktop/sugar/network/adhoc'

s/_SHOW_ADHOC/_SHOW_ADHOC_GCONF_KEY?

+            self._devices[device_o].connect('ap_added', self.__ap_added_cb)
+            self._devices[device_o].connect('ap_removed', self.__ap_removed_cb)

Better use - instead of underscores.

+            if properties['WirelessHardwareEnabled'] == True:

Why not else or at least elif? No need to compare to True/False (see PEP-08 for reasoning)

+            if self._adhoc_manager.remove_access_point(ap_o) == True:

Same here, but it's a bit weird to have the remove() func return a boolean. Why not having a separate is_ad_hoc() method or similar?

+            self._adhoc_manager.listen(device)

By the name of the method, I would expect it can be called several times. May be better to change it to start_listening() and to raise an exception if it's called more than once.

+_adhoc_manager = None
+
+
+def get_manager():
+    global _adhoc_manager
+
+    if not _adhoc_manager:
+        _adhoc_manager = AdHocManager()
+    return _adhoc_manager

May be good to keep it consistent with the other singletons. Don't assume that None evaluates to False.

+            try:
+                self._current_channel = network.frequency_to_channel(frequency)
+            except KeyError:
+                logging.debug("Error getting the Frequency.")

Same as above.

+        if ' 1' == access_point.name[-2:]:

Why not endswith()? (Please reply to my comments if you don't agree so I don't have to repeat myself).

comment:23 in reply to: ↑ 22 ; follow-up: Changed 4 years ago by erikos

Thanks for the review!

Replying to tomeu:

        <long>>If TRUE, Sugar will show default Ad-hoc networks for channel 1,6 and 11</long> 

One > too much, maybe we should mention we also create automatically an ad-hoc network, not just display them.

I added the 'automatically connect part' to the description.

+            try:
+                channel = network.frequency_to_channel(self._frequency)
+            except KeyError:
+                channel = 1

If you really want to use an exception here, then create a new one because KeyError can be raised because of several reasons. But probably frequency_to_channel() should just print a warning and return 1 if the frequency is not known.

Yes. moved the error handling part into the function and log a descriptive warning. Returning 1 might hide the error too much, though. Maybe 0 or -1?

+                if connection:

is not None

Done.

+                        connection.set_connected()

connection.connect() may be more in line with the existing code.

set_connected is actually a method of NMSettingsConnection (model/network.py). Do you mean to change that one?

+        self._bus = dbus.SystemBus()
+        self._manager = adhoc.get_manager()

No big deal, but if these aren't part of the sate of the class, then it would be better to just get references to them whenever we need it instead of caching the references.

Done.

+        'ap-added': (gobject.SIGNAL_RUN_FIRST, gobject.TYPE_NONE,

"ap" may not be obvious to all the future readers of this code.

Changed it to 'access-point-added'. Is quite long, that is why I did not do so in the first place, but yeah more descriptive.

+    _SHOW_ADHOC = '/desktop/sugar/network/adhoc'

s/_SHOW_ADHOC/_SHOW_ADHOC_GCONF_KEY?

Done.

+            self._devices[device_o].connect('ap_added', self.__ap_added_cb)
+            self._devices[device_o].connect('ap_removed', self.__ap_removed_cb)

Better use - instead of underscores.

Done.

+            if properties['WirelessHardwareEnabled'] == True:

Why not else or at least elif? No need to compare to True/False (see PEP-08 for reasoning)

Both done. In the spec (http://www.python.org/dev/peps/pep-0008/) they only say, that it is not the correct form but not the reasoning :) Under 'Resolved Issues' one can find more: http://www.python.org/dev/peps/pep-0285/

+            if self._adhoc_manager.remove_access_point(ap_o) == True:

Same here, but it's a bit weird to have the remove() func return a boolean. Why not having a separate is_ad_hoc() method or similar?

Yes, I merged the checking and removing in one method. A made it two methods now.

if self._adhoc_manager is not None:

if self._adhoc_manager.is_sugar_adhoc_access_point(ap_o):

self._adhoc_manager.remove_access_point(ap_o)
return


+            self._adhoc_manager.listen(device)

By the name of the method, I would expect it can be called several times. May be better to change it to start_listening() and to raise an exception if it's called more than once.

Done.

+_adhoc_manager = None
+
+
+def get_manager():
+    global _adhoc_manager
+
+    if not _adhoc_manager:
+        _adhoc_manager = AdHocManager()
+    return _adhoc_manager

May be good to keep it consistent with the other singletons. Don't assume that None evaluates to False.

_adhoc_manager_instance = None
def get_adhoc_manager_instance():

global _adhoc_manager_instance
if _adhoc_manager_instance is None:

_adhoc_manager_instance = AdHocManager()

return _adhoc_manager_instance

get_adhoc_manager_instance is long but get_manager_instance would not be descriptive enough imho.

+            try:
+                self._current_channel = network.frequency_to_channel(frequency)
+            except KeyError:
+                logging.debug("Error getting the Frequency.")

Same as above.

Ok.

+        if ' 1' == access_point.name[-2:]:

Why not endswith()? (Please reply to my comments if you don't agree so I don't have to repeat myself).

Sorry, forgot to reply. I did not want to use endswith i the first place, as I was checking for one character too '6' and I wanted to be consistent. I do check for ' 6' now, so it works with endswith.

New patch will follow, will do more testing after these changes first.

Changed 4 years ago by erikos

Reworked patch based on the review comments

comment:24 Changed 4 years ago by erikos

  • Keywords r? added; r! removed

Changed 4 years ago by erikos

fixed a typo

comment:25 in reply to: ↑ 23 Changed 4 years ago by tomeu

Replying to erikos:

+                        connection.set_connected()

connection.connect() may be more in line with the existing code.

set_connected is actually a method of NMSettingsConnection (model/network.py). Do you mean to change that one?

Ah, as you prefer.

_adhoc_manager_instance = None
def get_adhoc_manager_instance():

global _adhoc_manager_instance
if _adhoc_manager_instance is None:

_adhoc_manager_instance = AdHocManager()

return _adhoc_manager_instance

get_adhoc_manager_instance is long but get_manager_instance would not be descriptive enough imho.

Some modules have only get_instance() because assume that they will be called as members of the module, thus having more context.

comment:26 Changed 4 years ago by tomeu

  • Keywords r+ added; r? removed

comment:27 Changed 4 years ago by erikos

Thanks again for the preview. I left the name get_adhoc_manager_instance as is and left NMSettingsConnection untouched as it is easier to backport to 0.84 then.

http://git.sugarlabs.org/projects/sugar/repos/mainline/commits/5ff30eadf670f5225ae305f10e248c7d97a88fb4

comment:28 Changed 4 years ago by erikos

  • Resolution set to fixed
  • Status changed from assigned to closed

Let's close this ticket. I have fixed a few first time bugs already and the latest tarball did work ok for me.

For the 0.84 patch I will use: http://dev.laptop.org/ticket/9845

comment:29 Changed 4 years ago by bernie

It was reported on the Dextrose list that, with this patch applied, the device icon for a newly ad-hoc network does not show up.

Are we perhaps missing network-wireless-connected.svg?

comment:30 Changed 4 years ago by bernie

Never mind, I had not applied ad_hoc_artwork.patch.

comment:31 Changed 9 months ago by dnarvaez

  • Milestone 0.90 deleted

Milestone 0.90 deleted

Note: See TracTickets for help on using tickets.