Re: [PATCH 3/6] radio settings: add FastDormancy property
Hi Mika, On 10/26/2010 10:31 AM, Mika Liljeberg wrote: > --- > include/radio-settings.h | 13 + > src/radio-settings.c | 114 +++-- > 2 files changed, 121 insertions(+), 6 deletions(-) > Patch has been applied, thanks. I did make a couple of minor tweaks afterwards. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 3/6] radio settings: add FastDormancy property
Hi Denis, > Patch has been applied, thanks. I did make a couple of minor tweaks > afterwards. Thanks. A question regarding this particular tweak: diff --git a/src/radio-settings.c b/src/radio-settings.c index cb0a598..eb2a42d 100644 --- a/src/radio-settings.c +++ b/src/radio-settings.c @@ -126,8 +126,7 @@ static void radio_set_fast_dormancy(struct ofono_radio_settings *rs, const char *path = __ofono_atom_get_path(rs->atom); dbus_bool_t value = enable; - if ((rs->flags & RADIO_SETTINGS_FLAG_CACHED) && - rs->fast_dormancy == enable) + if (rs->fast_dormancy == enable) return; ofono_dbus_signal_property_changed(conn, path, The fundamental problem here is that there is only a single CACHED flag for multiple properties, which may be modified individually. So, either you get extra signals or you get too few. I checked the CACHED flag here because otherwise the following might happen: 1. client tries to GetProperties() and gets the "Operation already in progress" error. 2. client waits for PropertyChanged signal to get the FastDormancy value 3. signal never comes because the default value happens to match the one returned by the driver and the signal is suppressed I do agree that sending extra signals is bad but I think that not sending a signal is even worse. If the client cannot rely on getting a PropertyChanged signal after a busy error, all it can do is resort to polling. I.e., every client has to implement a polling pattern for GetProperties: while (GetProperties() == BUSY) sleep(FOR_A_WHILE); Having a separate CACHED flag for each value would solve this optimally. Failing that, I don't think a few extra signals is so bad. Forcing clients to poll is just ugly. Am I missing something? MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH 3/6] radio settings: add FastDormancy property
Hi Mika, On 10/28/2010 08:32 AM, mika.liljeb...@nokia.com wrote: > Hi Denis, > >> Patch has been applied, thanks. I did make a couple of minor tweaks >> afterwards. > > Thanks. A question regarding this particular tweak: > > diff --git a/src/radio-settings.c b/src/radio-settings.c > index cb0a598..eb2a42d 100644 > --- a/src/radio-settings.c > +++ b/src/radio-settings.c > @@ -126,8 +126,7 @@ static void radio_set_fast_dormancy(struct > ofono_radio_settings *rs, > const char *path = __ofono_atom_get_path(rs->atom); > dbus_bool_t value = enable; > > - if ((rs->flags & RADIO_SETTINGS_FLAG_CACHED) && > - rs->fast_dormancy == enable) > + if (rs->fast_dormancy == enable) > return; > > ofono_dbus_signal_property_changed(conn, path, > > The fundamental problem here is that there is only a single CACHED flag for > multiple properties, which may be modified individually. So, either you get > extra signals or you get too few. I checked the CACHED flag here because > otherwise the following might happen: Yes, I know. But this problem is present in every single atom. oFono does not guarantee that every attribute is signaled when the atom is initialized. > > 1. client tries to GetProperties() and gets the "Operation already in > progress" error. > 2. client waits for PropertyChanged signal to get the FastDormancy value > 3. signal never comes because the default value happens to match the one > returned by the driver and the signal is suppressed > In general I think that for interfaces where this can happen, the likelihood is very low that it actually will in the real world. Do note that I have had the same argument with myself off and on for the past two years. So far this was never raised as an issue. If this ever becomes a problem, we can fix it properly using an appropriate idiom. > I do agree that sending extra signals is bad but I think that not sending a > signal is even worse. If the client cannot rely on getting a PropertyChanged > signal after a busy error, all it can do is resort to polling. I.e., every > client has to implement a polling pattern for GetProperties: > > while (GetProperties() == BUSY) > sleep(FOR_A_WHILE); > > Having a separate CACHED flag for each value would solve this optimally. > Failing that, I don't think a few extra signals is so bad. Forcing clients to > poll is just ugly. > Honestly, if you expect multiple applications to battle over the FastDormancy property, then it should be modeled differently. Perhaps with application registration and lifetime tracking over D-Bus, similar to how agents work. > Am I missing something? > > MikaL Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 3/6] radio settings: add FastDormancy property
Hi Denis, > > The fundamental problem here is that there is only a single > CACHED flag for multiple properties, which may be modified > individually. So, either you get extra signals or you get too > few. I checked the CACHED flag here because otherwise the > following might happen: > > Yes, I know. But this problem is present in every single atom. oFono > does not guarantee that every attribute is signaled when the atom is > initialized. So I gathered. To me this looks like a wider issue, though. InProgress errors are returned in many other contexts as well, and they are not that well documented in the API documentation. All this makes it a bit painful to write a robust oFono client. Probably this could be at least partially rectified by improving the documentation. > > 1. client tries to GetProperties() and gets the "Operation > already in progress" error. > > 2. client waits for PropertyChanged signal to get the > FastDormancy value > > 3. signal never comes because the default value happens to > match the one returned by the driver and the signal is suppressed > > > > In general I think that for interfaces where this can happen, the > likelihood is very low that it actually will in the real world. Perhaps so. The network and SIM interfaces, which are most likely to be bombared by multiple UI components, seem to be doing the right thing at least. > Do note that I have had the same argument with myself off and > on for the > past two years. So far this was never raised as an issue. > If this ever > becomes a problem, we can fix it properly using an appropriate idiom. If this becomes a problem, it won't necessarily be visible to upstream. More likely this will be noticed in product maturization phase, and the fixes made to a product specific stable branches might never trickle back to upstream. > > I do agree that sending extra signals is bad but I think > that not sending a signal is even worse. If the client cannot > rely on getting a PropertyChanged signal after a busy error, > all it can do is resort to polling. I.e., every client has to > implement a polling pattern for GetProperties: > > > > while (GetProperties() == BUSY) > > sleep(FOR_A_WHILE); > > > > Having a separate CACHED flag for each value would solve > this optimally. Failing that, I don't think a few extra > signals is so bad. Forcing clients to poll is just ugly. > > > > Honestly, if you expect multiple applications to battle over the > FastDormancy property, then it should be modeled differently. Perhaps > with application registration and lifetime tracking over > D-Bus, similar > to how agents work. Hardly that, FastDormancy is unlikely to be a problem. I was merely curious whether there is a general design rule underneath, or if these things are decided on a case by case basis. Based on your comments and looking at the code, I guess it's more case by case. I just feel uneasy about an API that returns transient errors by design, on the (even if informed) assumption that it will probably be ok. I also dislike the fact that a generic InProgress error pretty much forces a client to just retry each operation until it succeeds. If there are problems like this, they are most likely discovered only in the product maturization phase and then it's generally too late to fix the upstream. Too late for that particular product, that is. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH 3/6] radio settings: add FastDormancy property
Hi Mika, > > So I gathered. To me this looks like a wider issue, though. InProgress errors > are returned in many other contexts as well, and they are not that well > documented in the API documentation. All this makes it a bit painful to write > a robust oFono client. Probably this could be at least partially rectified by > improving the documentation. > Of course, and patches are always welcome. >> Do note that I have had the same argument with myself off and >> on for the >> past two years. So far this was never raised as an issue. >> If this ever >> becomes a problem, we can fix it properly using an appropriate idiom. > > If this becomes a problem, it won't necessarily be visible to upstream. More > likely this will be noticed in product maturization phase, and the fixes made > to a product specific stable branches might never trickle back to upstream. > I disagree. I think upstream will be notified and we will improve the situation as we progress. But I do agree it might be too late for that particular product. Of course that is the case with all software. >> Honestly, if you expect multiple applications to battle over the >> FastDormancy property, then it should be modeled differently. Perhaps >> with application registration and lifetime tracking over >> D-Bus, similar >> to how agents work. > > Hardly that, FastDormancy is unlikely to be a problem. I was merely curious > whether there is a general design rule underneath, or if these things are > decided on a case by case basis. Based on your comments and looking at the > code, I guess it's more case by case. I just feel uneasy about an API that > returns transient errors by design, on the (even if informed) assumption that > it will probably be ok. I also dislike the fact that a generic InProgress > error pretty much forces a client to just retry each operation until it > succeeds. If there are problems like this, they are most likely discovered > only in the product maturization phase and then it's generally too late to > fix the upstream. Too late for that particular product, that is. > One of the biggest principles in oFono is not to perform premature optimization. Sure there is a potential issue, but nobody knows whether it will actually manifest itself outside of malicious code (which we tell to bugger off) or what the most common manifestation pattern will be. If / once we know for sure this is a problem, then we can solve it properly. So far this approach has been working very nicely for us. There are countless occasions where taking the wait-and-see approach and gathering more information allowed us to devise a much better solution than we would have originally. So the general rule is: Do the simplest thing that is likely to work. If it doesn't work, improve it. > Br, > > MikaL Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 3/6] radio settings: add FastDormancy property
Hi Denis, > One of the biggest principles in oFono is not to perform premature > optimization. Sure there is a potential issue, but nobody > knows whether > it will actually manifest itself outside of malicious code (which we > tell to bugger off) or what the most common manifestation > pattern will be. > > If / once we know for sure this is a problem, then we can solve it > properly. So far this approach has been working very nicely for us. > There are countless occasions where taking the wait-and-see > approach and > gathering more information allowed us to devise a much better solution > than we would have originally. > > So the general rule is: Do the simplest thing that is likely to work. > If it doesn't work, improve it. Can't fault the approach, it's generally a good one. That said, I see this more as an API quality issue rather than an optimization issue. Anyway, I've raised the concern. Let's hope my worries are unfounded. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
