e-mission / e-mission-data-collection

Repository for our own data collection
BSD 3-Clause "New" or "Revised" License
4 stars 19 forks source link

Installing a new version or changing the config restarts tracking #115

Closed shankari closed 8 years ago

shankari commented 8 years ago

I recently upgraded from v0.0.8 to v0.0.9 on a phone that had tracking stopped, and it started tracking again. I think that the duty cycling was turned off (it went directly to ongoing_trip), so the problem may be with the init transition not being handled properly in TripDiaryStateMachineServiceOngoing.

Note that one even more egregious case of this was fixed in 993a630695945fac5501ceb062f030e58785cd0d but as we move to more automatic updates, we want to ensure that we don't secretly start tracking again.

shankari commented 8 years ago

Logs:

06-08 12:56:53.554  29800-29800/edu.berkeley.eecs.emission I/System.out﹕ logger == null, lazily creating new logger
06-08 12:56:53.564  29800-29800/edu.berkeley.eecs.emission D/DataCollectionPlugin﹕ google play services available, initializing state machine
06-08 12:56:53.567  29800-29800/edu.berkeley.eecs.emission I/System.out﹕ mAccount = Account {name=dummy_account, type=eecs.berkeley.edu}
06-08 12:56:53.569  29800-29848/edu.berkeley.eecs.emission I/System.out﹕ All preferences are {setup_complete=82, bgsync_launch_next_online=false, TripDiaryCurrState=local.state.tracking_stopped}
06-08 12:56:53.577  29800-29848/edu.berkeley.eecs.emission D/TripDiaryStateMachineReceiver﹕ Setup not complete, sending initialize
06-08 12:56:53.643  29800-29800/edu.berkeley.eecs.emission I/System.out﹕ During plugin initialize, created usercacheedu.berkeley.eecs.emission.cordova.usercache.BuiltinUserCache@49fca7a

06-08 12:56:54.443  29800-29800/edu.berkeley.eecs.emission I/TripDiaryStateMachineReceiver﹕ noarg constructor called
06-08 12:56:54.458  29800-29800/edu.berkeley.eecs.emission I/TripDiaryStateMachineReceiver﹕ TripDiaryStateMachineReciever onReceive(android.app.ReceiverRestrictedContext@ffc5c18, Intent { act=local.transition.initialize flg=0x10 cmp=edu.berkeley.eecs.emission/.cordova.tracker.location.TripDiaryStateMachineReceiver }) called
...

06-08 12:56:54.602  29800-29800/edu.berkeley.eecs.emission D/TripDiaryStateMachineService﹕ service started with flags = 0 startId = 1 action = local.transition.initialize
06-08 12:56:54.655  29800-29800/edu.berkeley.eecs.emission D/TripDiaryStateMachineService﹕ after reading from the prefs, the current state is local.state.tracking_stopped
06-08 12:56:54.668  29800-29800/edu.berkeley.eecs.emission D/BuiltinUserCache﹕ Added value for key statemachine/transition at time 1.465415814657E9
...
06-08 12:56:54.818  29800-29800/edu.berkeley.eecs.emission D/TripDiaryStateMachineService﹕ TripDiaryStateMachineReceiver handleTrackingStopped(local.transition.initialize) called
06-08 12:56:54.825  29800-29800/edu.berkeley.eecs.emission D/TripDiaryStateMachineService﹕ newState after handling action is local.state.ongoing_trip
06-08 12:56:54.836  29800-29800/edu.berkeley.eecs.emission D/TripDiaryStateMachineService﹕ newState saved in prefManager is local.state.ongoing_trip
06-08 12:56:54.848  29800-29800/edu.berkeley.eecs.emission D/NotificationHelper﹕ Generating notify with id 78283 and message Success moving to local.state.ongoing_trip
shankari commented 8 years ago

ok so the problem is overloading initialize to mean both initialize and start tracking. Should really fix that.

shankari commented 8 years ago

So although I thought that this was because we were reusing initialize, that is not the problem.

The problem is that we handle initOnUpgrade by calling initialize, and on initialize, we ALWAYS restart tracking. Clearly we should check the current state, either in initOnUpgrade or in the FSM. Probably the FSM. From stopped_tracking, the only state that should leave should be start_tracking.

And for reconfigure, we actually stop and start tracking. We should have a different transition for that. That will also ensure that we can remove this super hacky code, and potentially, with correct versioning, can reconfigure even if the data is pushed from the server.

    public static void restartCollection(Context ctxt) {
        /*
         Super hacky solution, but works for now. Steps are:
         * Stop tracking
         * Poll for state change
         * Start tracking
         * Ugh! My eyeballs hurt to even read that?!
         */
        ctxt.sendBroadcast(new Intent(ctxt.getString(R.string.transition_stop_tracking)));
        final Context fCtxt = ctxt;
        new Thread(new Runnable() {
            @Override
            public void run() {
        boolean stateChanged = false;
        while(!stateChanged) {
            try {
                Thread.sleep(1000); // Wait for one second in the loop
                        stateChanged = (TripDiaryStateMachineService.getState(fCtxt).equals(
                                fCtxt.getString(R.string.state_tracking_stopped)));
            } catch (InterruptedException e) {
                e.printStackTrace();
                // Sleep has been interrupted, might as well exit the while loop
                stateChanged = true;
            }
        }
                fCtxt.sendBroadcast(new Intent(fCtxt.getString(R.string.transition_start_tracking)));
            }
        }).start();
    }
shankari commented 8 years ago

The challenge is to then deal with yet another transition in an already complicated state diagram. And if the user is reconfiguring, they can always stop tracking anyway. Will fix the first part for now. Will fix the second part later.

shankari commented 8 years ago

We should have a different transition for that.

We cannot easily do that. I spent a bunch of time doing it, and then it turned out that it wouldn't work because we use different services for duty cycled and ongoing, and the service is chosen by the receiver. So for this to work, we need a call to the receiver between stop and start at least when we toggle duty cycling, which means that we might as well do it for everything.

This is not a huge issue on iOS anyway, since stopping does not happen in the background.

So we are going to handle reconfigure by stop + start in the foreseeable future.

shankari commented 8 years ago

So we are going to handle reconfigure by stop + start in the foreseeable future.

I will add a quick check for the current state to ensure that we don't inadvertently start tracking on reconfigure.