Feature request: support for moving the FSM

[enbyted]

First of all - thanks for this project and your work here!

For my use case I would need to have the FSM instance be movable and have its state list also movable (i.e. replacing with a different address that contains the same information). The reasons are quite complex, but boiling down to me trying to (ab)use the framework in ways that were probably not quite intended - the state list is dynamically build in runtime.

Currently fsm is not movable, I suppose because it could allow for dangerous behavior when people move the FSM and the location of the state array. Though that can probably be acceptable with proper documentation, or by making a non-standard copy constructor that takes in something like i_know_this_is_dangerours_t as a token.

Moving of state list is more difficult, but could be achieved if during the transition we can still access the old list (as as in dependent-class move constructor). For that usecase you could provide a move_states function that will:

• Assert that the number of states did not change
• Check & save the current state id
• call set_states
• Set p_state to new address of saved state id

Maybe it could even be part of the new copy constructor? I know this sounds convoluted, there probably is a better way of doing it - I'm open for ideas.

I can implement this, once we reach an understanding of what would be acceptable way for you of achieving the goal.

[jwellbelove]

the state list is dynamically build in runtime.

Can we not just call set_states() with the new state list, adding code to set_states() for adjustment of the pointers if a state list already exists?

[enbyted]

@jwellbelove I suppose we can do that, given that we have a move constructor.

Actually, it would be enough to support the usecase to have a direct way of setting a state (example -> make start() take the initial state as an optional argument).

With that primitive, to move the FSM I could:

1. initialize a fresh FSM
2. check current state id
3. call set_states()
4. call start with saved state id and skipping the entry callback

// I can see the horror on your face! I promise it's not that bad... While I can't go into details, imagine making a tool to generate a custom interpreter, effectively building a state machine. Multiply that that with limited memory in deeply embedded device.

[jwellbelove]

Do you actually need a 'fresh' FSM if you are just moving it anyway? A move implies that the old FSM is not going to be used any more (I may be missing a fundamental aspect of what you want to do). Is there a reason you can't just replace the state list in the current FSM + adjustments or does the old FSM continue to be actively in use? I'm trying to get a complete picture of your use case so that we can work out the simplest possible solution that has little to no impact on current users.

It's getting close to 9pm here, so I'm off to eat dinner and watch the telly. Speak to you tomorrow.

[enbyted]

I'm trying to do a simple move, exactly as you describred.

Do you actually need a 'fresh' FSM if you are just moving it anyway?

Well, move construction kind of implies that you have a fresh FSM that you move old state into

A move implies that the old FSM is not going to be used any more

Yes, correct

Is there a reason you can't just replace the state list in the current FSM + adjustments

Currently - there is no way for me to directly set the previous state after replacing the state list

Note, that I am trying to both move the FSM and the state list.

Think of an object like that:

class werid : public etl::fsm {
  etl::vector<etl::ifsm_state *, 8192U> m_states;

public:
  weird() : etl::fsm{0U}, m_states{} {
    build_state_vector();
    set_states(m_states.begin(), m_states.size());
    start(true);
  }

  weird(const weird&) = delete;
  weird& operator=(const weird&) = delete;

  weird(weird&& other) : etl::fsm{0U}, m_states{etl::move(other.m_states)} {
    // TODO: What goes here ?
  }
  weird& operator=(weird&& other) {
    m_states = etl::move(other.m_states);
    // TODO: What goes here ?
    return *this;
  }
};

[jwellbelove]

I just want to make sure I understand correctly. Do you just want to be able to move the FSM along with the pointer to the state list and the current state, leaving the old FSM in a 'not started' 'no state list' condition?

weird fsm1;
weird.set_states(<some state list>);
weird.start();

weird fsm2(etl::move(fsm1));

// fsm1 has no state list defined.
// fsm1 is not started.

// fsm2 is using fsm1's state list.
// fsm2 is started and is in the state that fsm1 was in.

This change may also be applicable to the _ext containers (such as etl::vector_ext), though I'd have to think about any hidden 'gotchas'.

[enbyted]

Ah, I see I forgotten an important detail in my example - the addresses of the states change. I'm sorry for that, I must have been distracted.

class werid : public etl::fsm {
  etl::vector<ThingsThatInheritFrom_etl_ifsm_state, 8192U> m_state_values;
  etl::vector<etl::ifsm_state *, 8192U> m_states;

public:
weird(weird&& other) : etl::fsm{0U}, m_state_values{etl::move(other.m_state_values)}, m_states{} {
    rebuild_states_vector();
    // TODO: What goes here ?
  }
  weird& operator=(weird&& other) {
    m_state_values = etl::move(other.m_state_values);
    m_states.clear();
    rebuild_states_vector();
    // TODO: What goes here ?
    return *this;
  }
// Rest can be the same/similar I guess
private:
  void rebuild_states_vector() {
    for (etl::ifsm_state& state : m_state_values) {
      m_states.push_back(&state);
    }
  }
};

The addresses of the state values are changing, however their effective value is staying constant.

Coming to your snippet:

weird fsm1;
weird.set_states(<some state list>);
weird.start();

weird fsm2(etl::move(fsm1));

/// I don't care about the state of fsm1 after the move as it is UB to use it anyway based on move-semantics
/// Also, the destructor of fsm doesn't do anything special.
// fsm1 has no state list defined.
// fsm1 is not started.

/// That would be sensible state after the move
// fsm2 is using fsm1's state list.
// fsm2 is started and is in the state that fsm1 was in.

/// Now the tricky part is, changing the state list to account for the fact that the addresses of the concrete states have changed

As I said, being able to specify the start state would be enough I think, given this example it would properly move the fsm1 to fsm2, right?

weird(weird&& other) : etl::fsm{0U}, m_state_values{}, m_states{} {
    const auto state_id = other.get_state_id();
    // Note moving after calling the state ID, because get_state_id uses the old pointer to one of the states, which may be UB after moving it
    m_state_values = etl::move(other.m_state_values);
    rebuild_states_vector();
    start(false, state_id);
    // now we are in the same state as the other
    // the other is considered moved-out-of so using it is UB, although in this scenario we have effectively made a copy of it
    // (if moving m_state_values is equivalent to copying it)
  }

It might not be enough for hfsm, I'm not sure - I'm not using that here.

// Moving the storage of an ext container would be quite problematic, unless the types are trivially copiable/movable. Also, I don't think it's quite as useful to move the storage data of an _ext container as if you wanted that capability you'd just use the non-ext version. Ext to me is more about having a static storage (like really static, or done in one heap allocation), so it will not move anyway.

[jwellbelove]

Moving the storage of a _ext would be similar to what the STL does for a container move. Both would just copy the pointer to the storage to the new container and invalidate the old container's pointer. In both cases the original container would be in a valid and destructible state, and the new valid container would point to the old container's storage.

I'm out today (looking for a new(er) car), but I'll have a look at the other points when I get back later.

[enbyted]

Moving the storage of a _ext would be similar to what the STL does for a container move. Both would just copy the pointer to the storage to the new container and invalidate the old container's pointer.

What you're describing is moving the _ext container itself, not the storage. Moving the storage would be more like what std::vector does when it reallocates:

• Allocate new storage (on our ETL - just use the new pointer)
• Move all of the contents from old storage to new one (i.e. loop through and call move-constructors)
• De-allocate old storage (no-op for ETL)

I'm out today (looking for a new(er) car)

Good luck with that! And thank you for your amazing responsiveness here.

[jwellbelove]

What you're describing is moving the _ext container itself, not the storage. Moving the storage would be more like what std::vector does when it reallocates:

Yes, I was talking about 'move' as in rvalue reference 'move' as opposed to a 'copy-like move'.

std::vector<int> v1;
std::vector<int> v2(std::move(v1)); // v2 has v1's buffer

int buffer[10];
etl::vector_ext<int> v1(buffer, 10);
etl::vector_ext<int> v2(etl::move(v1)); // v2 has a pointer to 'buffer', v1 points to nullptr

[enbyted]

@jwellbelove are you awaiting some feedback from me here?

I think we're currently waiting on you, but I might be wrong.

but I'll have a look at the other points when I get back later.

[jwellbelove]

Sorry, I got side tracked into looking at move constructors for other containers.

You said Now the tricky part is, changing the state list to account for the fact that the addresses of the concrete states have changed

The state list for etl::fsm is passed as a pointer to an array of etl::ifsm_state pointers (etl::ifsm_state** state_list), so the concrete states would be at the same address that they were before. Or are your concrete states members of your class and will also be 'moved' to the new etl::fsm based class?

[enbyted]

Sorry, got sidetracked as well.

Or are your concrete states members of your class and will also be 'moved' to the new etl::fsm based class?

Yes, the concrete states are also moving. Coming back to my example, the class that is getting moved is:

class werid : public etl::fsm {
  etl::vector<ThingsThatInheritFrom_etl_ifsm_state, 8192U> m_state_values;
  etl::vector<etl::ifsm_state *, 8192U> m_states;
/* ... */
};

[enbyted]

Hi @jwellbelove, did you get sidetracked again? :smile:

As I said originally, I'm fine with doing the actual implementation, but let's agree first on what that implementation should entail.

[jwellbelove]

I didn't get sidetracked this time, well I did, sort of. I've been on holiday in Cumbria and Scotland for the last 10 days.