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

inconsistency of calling of exit and entry in state machine self transitions #766

Closed
vahdat-ab opened this Issue Mar 9, 2016 · 17 comments

Comments

Projects
None yet
4 participants
@vahdat-ab
Member

vahdat-ab commented Mar 9, 2016

In the generated code for Umple state machines, exit and entry parts of states are dealt in different ways. We know that cyclic transitions can be internal or external. As long as I know, Umple consider the cyclic transitions as an external one. Therefore, it must run entry and exit parts for every cyclic transiton. However, In the generated code, we can see the entry part is executed, but the exit part doesn't. Let's have a look at an example:

class A{
  boolean result = false;
  sm{
    created{
      exit /{execute_exit_code();}
      entry /{execute_entry_code();}
      init [b==false] -> created;
      init [b==true] -> initialized;
    }
    initialized{
      getback ->created;
    }
  } 
}

in the generated code, we have the following code for the event init:

  public boolean init()
  {
    boolean wasEventProcessed = false;

    Sm aSm = sm;
    switch (aSm)
    {
      case created:
        if (b.equals(false))
        {
          setSm(Sm.created);
          wasEventProcessed = true;
          break;
        }
        if (b.equals(true))
        {
          exitSm();
          setSm(Sm.initialized);
          wasEventProcessed = true;
          break;
        }
        break;
      default:
        // Other states do respond to this event
    }

    return wasEventProcessed;
  }

As we can see, the method exitSm() is not executed for the cyclic case. However, the method execute_entry_code() is executed always through the method setSm

  private void setSm(Sm aSm)
  {
    sm = aSm;

    // entry actions and do activities
    switch(sm)
    {
      case created:
        // line 7 "model.ump"
        execute_entry_code();
        break;
    }
  }

@TimLethbridge TimLethbridge added ucosp and removed JET labels Sep 20, 2016

@TimLethbridge TimLethbridge changed the title from ambiguity in the generated code of state machines to inconsistency of calling of exit and entry in state machine self transitions Sep 20, 2016

@tlaport4 tlaport4 self-assigned this Oct 11, 2016

@tlaport4

This comment has been minimized.

Contributor

tlaport4 commented Nov 8, 2016

There's logic to ignore the exit action when it's a self transition. When this logic is removed, many tests fail. Should I be updating the tests to pass in this situation?

Should both the entry and exit code be executed for self transitions? In this example, it would look like this:

  public boolean init()
  {
    boolean wasEventProcessed = false;

    Sm aSm = sm;
    switch (aSm)
    {
      case created:
        if (b.equals(false))
        {
          exitSm();
          setSm(Sm.created);
          wasEventProcessed = true;
          break;
        }
        if (b.equals(true))
        {
          exitSm();
          setSm(Sm.initialized);
          wasEventProcessed = true;
          break;
        }
        break;
      default:
        // Other states do respond to this event
    }

    return wasEventProcessed;
  }
@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Nov 8, 2016

@TimLethbridge I wonder what you think. What was the initial definition for transitions? Have you considered them internal or external? If it is external then those test cases must be updated?

@TimLethbridge

This comment has been minimized.

Member

TimLethbridge commented Nov 14, 2016

Above all we have to be consistent. I think that a self transition should always trigger an exit and entry. But analyse each test that is failing and see what is going on. There are many complex cases and each needs to be considered. But after consideration, if no good counterargument can be raised, then adjust the tests to ensure that all exits are called as well as all entries.

@tlaport4

This comment has been minimized.

Contributor

tlaport4 commented Nov 24, 2016

I've noticed that with this change, concurrent states no longer function the same way.

Take this example:

class Example
{
  status
  {
    Off
    {
      turnOn -> On;
    }
    On
    {
      turnOff -> Off;
      MotorIdle
      {
        flip -> MotorRunning;
        flup -> Off;
      }
      MotorRunning
      {
        flip -> MotorIdle;
      }
      
      ||
      
      FanIdle
      {
        flop -> FanRunning;  
      }
      
      FanRunning
      {
        flop -> FanIdle;
      }
      
      
    }
  
  }
}

Now, when flip is called, exitStatus is called.

    Example ex = new Example();
    ex.turnOn();
    ex.flip();

When this happens, it sets StatusMotorIdle and StatusFanIdle to Null because they're concurrent states. Then it sets StatusMotorIdle to MotorRunning as normal. However, StatusFanIdle remains to be Null. So then if you call, ex.flop();, it doesn't do anything because there is no case for when StatusFanIdle is Null (see below):

  public boolean flop()
  {
    boolean wasEventProcessed = false;
    
    StatusFanIdle aStatusFanIdle = statusFanIdle;
    switch (aStatusFanIdle)
    {
      case FanIdle:
        exitStatus();
        setStatusFanIdle(StatusFanIdle.FanRunning);
        wasEventProcessed = true;
        break;
      case FanRunning:
        exitStatus();
        setStatusFanIdle(StatusFanIdle.FanIdle);
        wasEventProcessed = true;
        break;
      default:
        // Other states do respond to this event
    }

    return wasEventProcessed;
  }

Should logic be added to handle this scenario?

@tlaport4

This comment has been minimized.

Contributor

tlaport4 commented Nov 24, 2016

Had a talk with Vahdat. There was an issue with my implementation for this change, and I better understand what I need to do. Ignore my last comment.

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Dec 16, 2016

Here is somehow a complete example shows how exit must be treated.
@TimLethbridge @opeyemiAdesina I wonder if you guys can check if the semantics related to execution order is correct or not?

class X{
  sm{
    on{
      exit /{exit_on_execute();}
      e1-> off;
      e2-> on;
      s1{
        exit /{exit_s1_execute();}
        e3-> s2;
        e4-> s1;
        e5-> on;
        e6-> off;
        m1{
          exit /{exit_m1_execute();}
          e7-> m2;
          e8-> m1;
          e9-> s1;
          e10->s2;
          e11->on;
          e12->off;
        } 
        m2{} 
      }
      s2{}
    }
    off{}
  }
  void exit_on_execute(){System.out.println("exited on");}
  void exit_s1_execute(){System.out.println("exited s1");}
  void exit_m1_execute(){System.out.println("exited m1");}
}

The result based on the Tyler's last PR
//e1-> off; : exit_on_execute(); //correct
//e2-> on; : exit_on_execute(); //correct
//e3-> s2; : exit_s1_execute(); //correct
//e4-> s1; : exit_s1_execute(); //correct
//e5-> on; : exit_s1_execute(); -> exit_on_execute(); //Not correct
//e6-> off; : exit_s1_execute(); -> exit_on_execute(); //Not correct
//e7-> m2; : exit_m1_execute(); //correct
//e8-> m1; : exit_m1_execute(); //correct
//e9-> s1; : exit_m1_execute(); ->exit_s1_execute(); //Not correct
//e10->s2; : exit_m1_execute(); ->exit_s1_execute(); //Not correct
//e11->on; : exit_m1_execute(); -> exit_s1_execute(); -> exit_on_execute(); //Not correct
//e12->off; : exit_m1_execute(); -> exit_s1_execute(); -> exit_on_execute(); //Not correct

The current result
//e1-> off; : exit_on_execute(); //correct
//e2-> on; : exit_on_execute(); //Not correct
//e3-> s2; : exit_s1_execute(); //correct
//e4-> s1; : exit_s1_execute(); //Not correct
//e5-> on; : exit_s1_execute(); -> exit_on_execute(); //Not correct
//e6-> off; : exit_s1_execute(); -> exit_on_execute(); //Not correct
//e7-> m2; : exit_m1_execute(); //correct
//e8-> m1; : exit_m1_execute(); //Not correct
//e9-> s1; : exit_m1_execute(); ->exit_s1_execute(); //Not correct
//e10->s2; : exit_m1_execute(); ->exit_s1_execute(); //Not correct
//e11->on; : exit_m1_execute(); -> exit_s1_execute(); -> exit_on_execute(); //Not correct
//e12->off; : exit_m1_execute(); -> exit_s1_execute(); -> exit_on_execute(); //Not correct

@tlaport4 Your PR resolves the issue related to a simple state. If it's a composite state, it's not working. I wonder if you can fix indicated issues in the execution orders?

@tlaport4

This comment has been minimized.

Contributor

tlaport4 commented Dec 16, 2016

What should the results be in your example?

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Dec 16, 2016

You need to make sure the result satisfies the execution order specified in my last comment. e.g. if the event e12 is called then exit_m1_execute(); -> exit_s1_execute(); -> exit_on_execute(); must be executed.

@tlaport4

This comment has been minimized.

Contributor

tlaport4 commented Dec 16, 2016

Oh, ok right. I thought you were saying those are the results that you got by running it (rather than the results that it should be).

@tlaport4

This comment has been minimized.

Contributor

tlaport4 commented Dec 16, 2016

I think this is an separate issue. If you look at e12, it doesn't involve self transitions at all. It should execute all three exit actions, but it only executes exit_m1_execute() (in both my PR and the current build).

Should this be a separate issue? Not all correct exit code is being called in nested state machines.

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Dec 16, 2016

Ok. I create a new issue and I'm going to merge your PR.
This issue is related to the issue #935.

@opeyemiAdesina

This comment has been minimized.

Contributor

opeyemiAdesina commented Dec 16, 2016

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Dec 16, 2016

@opeyemiAdesina please find my answer in the issue #935

@TimLethbridge

This comment has been minimized.

Member

TimLethbridge commented Dec 16, 2016

Opeyemi ... you exit one state (perhaps several, up the tree, executing the exit transitions one by one) ... you transition (executing any transition action) ... then you enter the destination states one by one going deeper into the hierarch (executing entry transitions one by one). So transitioning is done before entering.

@TimLethbridge

This comment has been minimized.

Member

TimLethbridge commented Dec 16, 2016

Here's my take on Vahdat's comments about 'Not correct'

//e5-> on; : exit_s1_execute(); -> exit_on_execute(); //Not correct
It needs to do exit_m1_execute if it is in m1. But if it is in m2 it doesn't
So yes, incorrect, depending on the starting state.

//e6-> off; : exit_s1_execute(); -> exit_on_execute(); //Not correct
Same comment as above,

//e9-> s1; : exit_m1_execute(); ->exit_s1_execute(); //Not correct
I think this is actually correct.

//e10->s2; : exit_m1_execute(); ->exit_s1_execute(); //Not correct
I think this is actually correct

//e11->on; : exit_m1_execute(); -> exit_s1_execute(); -> exit_on_execute(); //Not correct
Seems correct to me

//e12->off; : exit_m1_execute(); -> exit_s1_execute(); -> exit_on_execute(); //Not correct
Seems correct to me

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Dec 16, 2016

@TimLethbridge @opeyemiAdesina Please write your answers in this issue #935 .
I'll close this issue.

@opeyemiAdesina

This comment has been minimized.

Contributor

opeyemiAdesina commented Dec 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment