No mesh support under 83.x (joyride 2631)

[garycmartin]

Testing an XO-1 under joyride 2631, the neighbourhood view and/or the device frame area show no UI related to the support mesh networking. I was vaguely expecting the 3 neighbourhood mesh icons to perhaps have been removed, and replaced with a single frame device mesh icon, with mesh controlling palette, not sure the status of this.

[garycmartin]

OK, after some more testing with a 2nd XO...

Joyride 2631 neighbourhood view shows no mesh UIs, until another machine 'creates' a mesh network. I have a 2nd machine running 767, if both are connected to an AP, no mesh UIs appear on Joyride 2631. If I then switch the 767 XO to one of the mesh channels, the Joyride 2631 XO displays 2 (??) orange fill with green stroke circles in the neighbourhood view. Both are called olpc-mesh (no indication of channel). Clicking either seems to have no (successful) effect, i.e no mesh device in the frame and no sign of the other buddy when clicking (and waiting) either. A click does make the circle fill pulse, and changes the palette to "olpc-mesh, Connecting..., Disconnect" but reverts to just "olpc-mesh, Connect" after about ~25sec.

[marcopg]

I fear this is not going to get done by 0.84 :( It would be good to raise the issue on the laptop.org mailing lists to see if someone is interested to contribute it.

[garycmartin]

I can't see mesh happening for 0.86, perhaps Link-Local/Ad-hoc networking will be enough so we at least have the infrastructure free 'kids under a tree' scenario still working for Sugar.

[tomeu]

Daniel may be interested on this.

[dsd]

NetworkManager-0.8 from git supports OLPC mesh, now we just need sugar support again. here's a patch against 0.84.5

 http://dev.laptop.org/~dsd/20090716/olpcmesh.patch

[tomeu]

Replying to dsd:

NetworkManager-0.8 from git supports OLPC mesh, now we just need sugar support again. here's a patch against 0.84.5
 http://dev.laptop.org/~dsd/20090716/olpcmesh.patch

Thanks for the patch, please see  http://wiki.sugarlabs.org/go/Development_Team/Code_Review for instructions.

[tomeu]

Looks like this won't make 0.86 :(

[dsd]

patch attached, please review

[dsd]

updated patch with improved device icon behaviour

[tomeu]

Excellent, follow some comments, mainly about style:

        self._palette.props.primary_text = _("Mesh Network") + " " + str(self._channel) 

Wonder if in RTL scripts the number would make more sense to be on the left.

            except dbus.exceptions.DBusException: 
 	605	                pass 

We really don't want to log anything?

    def __init__(self, device, tray, view_class=WirelessDeviceView): 
...
        self._device_view = view_class(self._device) 

Find this hard to read, cannot just pass the device type and instantiate one or the other with an if?

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

Would be better to create the palette when it's needed (grep the code for "def create_palette"), in order to save memory.

        p = palette.Palette(_("Mesh Network") + " " + str(self._channel)) 

We should try to avoid such identifiers, if each of us uses the smallest names that make sense to ourselves, others will end up having lots of difficulties.

    def _disconnect_activate_cb(self, item): 

Only one _? Guess you cannot remove this handler altogether?

        self._greyed_out = (query != '') 

Wonder if wouldn't make sense to accept the local translation of mesh for helping locating the mesh icons.

from jarabe.model.network import OlpcMesh as OlpcMeshSettings 

Wouldn't be better to just name jarabe.model.network.OlpcMesh as OlpcMeshSettings?

    def __init__(self, meshdev): 

Can we call it mesh_device or just device? Even more as it's a public member.

        self.ethdev = self._bus.get_object(_NM_SERVICE, ethdev_o) 
...
        self._bus.add_signal_receiver(self.__mshdev_state_changed_cb, 

Same as above.

        # this is a stack of connections that we'll iterate through until 
        # we find one that works 
        self._connection_queue = [] 

Is it a stack or a queue?

    # Activate a mesh connection on a user-specified channel 
    # Looks for XS first, then resorts to simple mesh 
    def user_activate_channel(self, channel): 

Can we make this and other comments apidocs? Also, would be good to remove as many of the inline comments as possible, possibly by splitting some of the methods and then giving apidocs to those if the method names are not enough.

[dsd]

most of your comments apply equally to existing code found in those files, but i've made the changes for most of them.

Replying to tomeu:

We really don't want to log anything?

No - the connections might have changed state during the loop

Would be better to create the palette when it's needed (grep the code for "def create_palette"), in order to save memory.

I had a quick look and couldn't figure this out. The other 3 classes in this file create the palette the same way.

Wonder if wouldn't make sense to accept the local translation of mesh for helping locating the mesh icons.

it would be nice but I haven't bothered to implement this this time around. The behaviour that I have implemented matches 0.82.

Wouldn't be better to just name jarabe.model.network.OlpcMesh as OlpcMeshSettings?

this would break the naming convention of classes in that file

Is it a stack or a queue?

its a connection queue implemented as a stack

I couldn't see any opportunities to divide functions up without impacting code readability.

sorry that I'm a bit strapped for time..here's an updated patch

[dsd]

actually its an incremental patch, hopefully makes review easier?

[tomeu]

actually its an incremental patch, hopefully makes review easier?

Better to always attach the whole patch, can you do so? I will finish it if you cannot devote more time on it.

[tomeu]

complete patch

[tomeu]

Pushed as  http://git.sugarlabs.org/projects/sugar/repos/mainline/commits/2351cf86

Thanks a lot for tackling this long-standing issue.