After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 707848 - Implement EnergyManagement service
Implement EnergyManagement service
Status: RESOLVED FIXED
Product: rygel
Classification: Applications
Component: librygel-core
git master
Other Linux
: Normal normal
: ---
Assigned To: rygel-maint
rygel-maint
Depends on:
Blocks:
 
 
Reported: 2013-09-10 11:33 UTC by Jussi Kukkonen
Modified: 2014-11-08 14:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
core: Add initial EnergyManagement service implementation (22.33 KB, patch)
2013-09-10 11:34 UTC, Jussi Kukkonen
none Details | Review

Description Jussi Kukkonen 2013-09-10 11:33:35 UTC
I've implemented the EnergyManagement:1 service spec that was recently published. The service is basically a way to say "this device may suspend, but you can wake it up by sending this magic packet over this transport". 

The implementation is quite minimal: it relies on UPower (the dbus service) to know when suspend/resume happens, and requires configuration to know the "suspend mode". The only hard requirement is linux.vapi (for some ioctl processing to figure out mac address and interface type): AFAICT it's not possible to do this in a cross-platform way. I'll take suggestions on how to handle this...

The configuration looks like this:

    [Playbin]
    enabled=true
    energy-management=true

    [EnergyManagement-eth0]
    mode-on-suspend=IP-down-WakeOn

In the example the UPnP device for Playbin plugin will include a EnergyManagement service. The suspended NetworkInterfaceMode of the service on interface eth0  is "IP-down-WakeOn".

It integrates to the rygel plugins exactly like BasicManagement in bug 707831.

Code is in wip/energy-management, but I'll attach a patch as well.
Comment 1 Jussi Kukkonen 2013-09-10 11:34:45 UTC
Created attachment 254584 [details] [review]
core: Add initial EnergyManagement service implementation
Comment 2 Jens Georg 2013-09-10 12:00:39 UTC
Review of attachment 254584 [details] [review]:

What exactly is listening to that wakeup stuff?

::: src/librygel-core/rygel-energy-management.vala
@@ +117,3 @@
+    }
+
+    private bool get_mac_and_network_type (string iface,

brrrrrrrrrrrrrrrr, can we hide this in a C file where we can do proper conditional compilation?

Also, wouldn't be the correct way to do this add this information to the GUPnP context?
Comment 3 Jussi Kukkonen 2013-09-10 12:45:50 UTC
(In reply to comment #2)
> Review of attachment 254584 [details] [review]:
> 
> What exactly is listening to that wakeup stuff?

As in what would subscribe to changes to NetworkInterfaceInfo variable? A control point interested in being able to wake up devices like this. A robust control point implementation could keep track of NetworkInterfaceInfos it has ever seen (in the same network) so it could try waking devices up even if it had not actually seen the device go to suspend.

To make this a bit more complicated, also other EnergyManagement implementations will subscribe to NetworkInterfaceInfo changes: the idea is that they will then proxy the info, so that if even only one device stays awake in the network, the NetworkInterfaceInfos of all the sleeping devices are still available in the network. This proxying is optional and not implemented in my patch.

> ::: src/librygel-core/rygel-energy-management.vala
> @@ +117,3 @@
> +    }
> +
> +    private bool get_mac_and_network_type (string iface,
> 
> brrrrrrrrrrrrrrrr, can we hide this in a C file where we can do proper
> conditional compilation?

Sure, that's actually even easier than ioctls in Vala (that just felt dirty). I'll do that.

> Also, wouldn't be the correct way to do this add this information to 
> the GUPnP context?

Mac address, absolutely. The reason I didn't go with that approach was the network type identification. The spec says this:
    <InterfaceType>
    Required. xsd:NMTOKEN. Exactly one element. Type of this
    physical interface as an enumerated string. Values are:
    * Ethernet
    * Wi-Fi
    * HPNAoverCoax
    * HomePlug
    * MoCA
    * 1905
    * Other

Currently the code just checks if the interface supports linux WirelessExtensions: if yes, it's "Wi-Fi", if not it's "Ethernet". Putting that in the EnergyManagement implementation felt unoptimal but acceptable. Putting something like that in gupnp API sounds worse... if you have great ideas how to implement that, I'm all ears.
Comment 4 Jens Georg 2013-09-13 09:08:02 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > Review of attachment 254584 [details] [review] [details]:
> > 
> > What exactly is listening to that wakeup stuff?
> 
> As in what would subscribe to changes to NetworkInterfaceInfo variable? A
> control point interested in being able to wake up devices like this. A robust
> control point implementation could keep track of NetworkInterfaceInfos it has
> ever seen (in the same network) so it could try waking devices up even if it
> had not actually seen the device go to suspend.

No I meant the physical wakeup pattern? Is that "standard" WOL stuff? I'm kind-of confused by the actual implementation, also by the usage of UPower's susped/resume signals, isn't that system-wide?


> To make this a bit more complicated, also other EnergyManagement
> implementations will subscribe to NetworkInterfaceInfo changes: the idea is
> that they will then proxy the info, so that if even only one device stays awake
> in the network, the NetworkInterfaceInfos of all the sleeping devices are still
> available in the network. This proxying is optional and not implemented in my
> patch.
> 
> > ::: src/librygel-core/rygel-energy-management.vala
> > @@ +117,3 @@
> > +    }
> > +
> > +    private bool get_mac_and_network_type (string iface,
> > 
> > brrrrrrrrrrrrrrrr, can we hide this in a C file where we can do proper
> > conditional compilation?
> 
> Sure, that's actually even easier than ioctls in Vala (that just felt dirty).
> I'll do that.

Exactly. Didn't feel very nice :)

> 
> > Also, wouldn't be the correct way to do this add this information to 
> > the GUPnP context?
> 
> Mac address, absolutely. The reason I didn't go with that approach was the
> network type identification. The spec says this:
>     <InterfaceType>
>     Required. xsd:NMTOKEN. Exactly one element. Type of this
>     physical interface as an enumerated string. Values are:
>     * Ethernet
>     * Wi-Fi
>     * HPNAoverCoax
>     * HomePlug
>     * MoCA
>     * 1905
>     * Other
> 
> Currently the code just checks if the interface supports linux
> WirelessExtensions: if yes, it's "Wi-Fi", if not it's "Ethernet". Putting that
> in the EnergyManagement implementation felt unoptimal but acceptable. Putting
> something like that in gupnp API sounds worse... if you have great ideas how to
> implement that, I'm all ears.

MAC address detection is actually a bit of a nasty portability issue, I looked into that for bug 695509.

At least the "intelligent" cm's such as Linux, NetworkManager and Connman should know at some point whether a device is WiFi or not and could do similar decisions to the current code.
Comment 5 Jens Georg 2014-02-01 14:05:39 UTC
ping?
Comment 6 Jens Georg 2014-10-31 22:10:26 UTC
Review of attachment 254584 [details] [review]:

::: src/librygel-core/rygel-energy-management.vala
@@ +140,3 @@
+            warning ("Failed to get mac address: %s\n",
+                     strerror (errno));
+            mac = "00:00:00:00:00:00";

While trying to port this to C... Why is type not set to "Other" here as above?
Comment 7 Jussi Kukkonen 2014-11-03 08:52:56 UTC
O-oh, I had forgotten this patch exists. Sorry.

(In reply to comment #6)
> Review of attachment 254584 [details] [review]:
> 
> ::: src/librygel-core/rygel-energy-management.vala
> @@ +140,3 @@
> +            warning ("Failed to get mac address: %s\n",
> +                     strerror (errno));
> +            mac = "00:00:00:00:00:00";
> 
> While trying to port this to C... Why is type not set to "Other" here as above?

The second ioctl will decide if it'll be set to "Ethernet" or "Wi-Fi". The values won't be very useful without a mac address (and the whole interface will be in "Unimplemented" mode) but I think it's still a valid choice.
Comment 8 Jussi Kukkonen 2014-11-03 09:05:56 UTC
And as I didn't seem to answer this back in the day:

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Review of attachment 254584 [details] [review] [details] [details]:
> > > 
> > > What exactly is listening to that wakeup stuff?
> > 
> > As in what would subscribe to changes to NetworkInterfaceInfo variable? A
> > control point interested in being able to wake up devices like this. A robust
> > control point implementation could keep track of NetworkInterfaceInfos it has
> > ever seen (in the same network) so it could try waking devices up even if it
> > had not actually seen the device go to suspend.
> 
> No I meant the physical wakeup pattern? Is that "standard" WOL stuff? I'm
> kind-of confused by the actual implementation, also by the usage of UPower's
> susped/resume signals, isn't that system-wide?

Yeah, supposedly it's 'standard' WOL (but I'm not guaranteeing there aren't other WOL patterns out there): so there should be hardware capable of responding to that WOL signal. 

The assumption is that whoever switches this on in rygel configuration also makes sure that the hardware is capable of resuming on WOL: that's not a user-friendly assumption but the whole feature is mostly interesting to someone building a HW+SW package that includes rygel.
Comment 9 Jens Georg 2014-11-03 11:41:22 UTC
Thanks. I have an updated branch with my suggestion to move the ugly code to C at home. And while doing that I think I understood the purpose and implementation of this service. I'll push to a new branch for checking later today
Comment 10 Jens Georg 2014-11-03 11:43:22 UTC
(In reply to comment #7)
> O-oh, I had forgotten this patch exists. Sorry.
> 
> (In reply to comment #6)
> > Review of attachment 254584 [details] [review] [details]:
> > 
> > ::: src/librygel-core/rygel-energy-management.vala
> > @@ +140,3 @@
> > +            warning ("Failed to get mac address: %s\n",
> > +                     strerror (errno));
> > +            mac = "00:00:00:00:00:00";
> > 
> > While trying to port this to C... Why is type not set to "Other" here as above?
> 
> The second ioctl will decide if it'll be set to "Ethernet" or "Wi-Fi". The
> values won't be very useful without a mac address (and the whole interface will
> be in "Unimplemented" mode) but I think it's still a valid choice.

Ok, I think I was mislead by the success = false line to take it as an early exit and not doing the WiFi ioctl afterwards. Makes sense.