Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flight mode fixes #4

Merged
merged 5 commits into from Jul 3, 2014

Conversation

Projects
None yet
3 participants
@cyphermox
Copy link
Owner

commented Jun 13, 2014

No description provided.

@tonyespy

This comment has been minimized.

Copy link

commented Jun 13, 2014

My first comment would be that your commit messages are a bit sparse. It'd be helpful to use the normal convention of short (<50chars) summary line, followed by a few lines actually describing what was changed. As we're actually trying to fix one or more reported bugs, including the bug numbers in the commit message would be helpful too.

Also, have you actually reproduced any of these scenarios and/or do you have instructions on how to test these changes?

Finally see comments inline.

@tonyespy

This comment has been minimized.

Copy link

commented on src/urf-arbitrator.c in 3ab50a3 Jun 13, 2014

So if one device fails to block, we bail on the rest of the devices, and the flight-mode is not enabled ( and the global state is updated in persist either )?

What happens if the first device happens to be the modem, and then WiFi fails to block. Aren't we left in a state where the modem is off, but urfkill doesn't consider the device in FlightMode?

I would think we should be treating flight-mode as a transactional event, we either can turn off all the radios, or we can't, but in the latter case we shouldn't leave the device partially shutdown.

This comment has been minimized.

Copy link
Owner Author

replied Jun 16, 2014

We are, but it's better to keep the devices in a partially disabled state, that is visibly partial (flight mode isn't set in that case) than trying to revert changes and risk that failing too.

As much as I'd want to make flight mode purely atomic, there is no way to do this -- you'll always have to set a property for flight mode, and then iterate through devices.

@tonyespy

This comment has been minimized.

Copy link

commented on src/urf-arbitrator.c in 3ab50a3 Jun 13, 2014

This block of code just seems like duplicate code to me anyways... Isn't each device's persist state set by urf_arbitrator_set_block() already?

@tonyespy

This comment has been minimized.

Copy link

commented on ff31657 Jun 13, 2014

Why are you making this change?

Also, it appears to me that if urfkill fails to offline/online ofono's modem, the state of the modem is not consistent anymore as the block is set both in the device state and the persist state when the call to urfk_arbitrate_set_block() is called for the modem, however if a failure occurs in set_online_cb(), it's merely logged.

This comment has been minimized.

Copy link
Owner Author

replied Jun 16, 2014

That is irrelevant. This commit was to fix the bug Alfonso reported. I had already mentioned it was unlikely to be caused by the dbus calls timing out.

It really shouldn't be up to urfkill to retry operations for the modem. There is no reasonable way to do this from urfkill, we can only know bits and pieces as to why things fail, and I don't feel it's right to go try to do loops of retries. As a minimum, that would mean adding way more oFono-specific logic to urfkill that it really ought to contain.

There are really two way around this: either oFono needs to grow the retry operations on its own, since it's what really knows about the modem, the error states, and whatnot, and able to know what operations likely should need a retry, or fix oFono/RIL to not fail these operations in the first place.

Regardless, there will always be a possibility for failure. We need to be aware of that. Sometimes, things don't work, and it's just too bad.

@@ -191,6 +189,9 @@ urf_arbitrator_set_flight_mode (UrfArbitrator *arbitrator,

ret = urf_arbitrator_set_block (arbitrator, i, want_state);
}

if (!ret)
break;

This comment has been minimized.

Copy link
@alfonsosanchezbeato

alfonsosanchezbeato Jun 16, 2014

Agreed with Tony's comment, either we can shut down all radios or the operation should terminate with an error. Also, I do not understand why this test is not inside the "if (state != KILLSWITCH_STATE_NO_ADAPTER) {" statement.

Besides, it looks like urf_arbitrator_set_block() saves the state only when radio operation is successful. I do not actually understand what is saved-states file for. As discussed in launchpad, for me it makes sense only if we store there the desired state. We want to move the system to that state when we re-start urfkilld.

This comment has been minimized.

Copy link
@cyphermox

cyphermox Jun 16, 2014

Author Owner

This is exactly the point. Desired state is saved, and we try to set flight mode. If one of the devices fails to be disabled, then the others before may have succeeded, and that's fine -- they will be considered disabled, you just won't be in flight mode. You can still toggle flight mode again, or toggle each device independently.

@@ -338,6 +338,10 @@ proxy_ready_cb (GObject *source_object,
g_cancellable_reset (priv->cancellable);
g_signal_connect (priv->proxy, "g-signal",
G_CALLBACK (modem_signal_cb), modem);

/* Explicitly set default timeout for method calls to 30 seconds */
g_dbus_proxy_set_default_timeout (priv->proxy, 30000);

This comment has been minimized.

Copy link
@alfonsosanchezbeato

alfonsosanchezbeato Jun 16, 2014

The time out problem was in the end a race condition problem for MTK modems + dual SIM, happening when both radios where being onlined within a very short interval. Please take a look at

https://bugs.launchpad.net/barajas/+bug/1329707

So you can remove this, apologies for the confusion as for me the WWAN bug was triggered due to this problem. Anyway, this illustrates how failing to switch a radio state causes wrong values to be stored in saved-states.

cyphermox added some commits Jun 16, 2014

UrfConfig: always write persistence file on change
Always write the persistence file out to disk when the persist state
is changed for a device type. This avoids issues with inconsistency if
writing the file fails on shutdown.

Signed-off-by: Mathieu Trudel-Lapierre <mathieu.trudel-lapierre@canonical.com>
UrfDaemon: log on failure to set flight mode
There was not error logged if the device could not be set in or out of
flight mode; only a return code to the DBus caller. Now also log, which
will help debugging flight mode issues.

Signed-off-by: Mathieu Trudel-Lapierre <mathieu.trudel-lapierre@canonical.com>

cyphermox added a commit that referenced this pull request Jul 3, 2014

@cyphermox cyphermox merged commit 740b08d into master Jul 3, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.