fsm: add osmo_fsm_inst_state_chg_keep_or_start_timer()

During FSM design for osmo-msc, I noticed that the current behavior that
keep_timer=true doesn't guarantee a running timer can make FSM design a bit
complex, especially when using osmo_tdef for timeout definitions.

A desirable keep_timer=true behavior is one that keeps the previous timer
running, but starts a timer if no timer is running yet.

The simplest example is: a given state repeatedly transitions back to itself,
but wants to set a timeout only on first entering, avoiding to restart the
timeout on re-entering.

Another example is a repeated transition between two or more states, where the
first time we enter this group a timeout should start, but it should not
restart from scratch on every transition.

When using osmo_tdef timeout definitions for this, so far separate meaningless
states have to be introduced that merely set a fixed timeout.

To simplify, add osmo_fsm_inst_state_chg_keep_or_start_timer(), and use this in
osmo_tdef_fsm_inst_state_chg() when both keep_timer == true *and* T != 0.

In tdef_test.ok, the changes show that on first entering state L, the previous
T=1 is now kept with a large remaining timeout. When entering state L from O,
where no timer was running, this time L's T123 is started.

Change-Id: Id647511a4b18e0c4de0e66fb1f35dc9adb9177db

5 files changed, 76 insertions, 17 deletions

diff --git a/include/osmocom/core/fsm.h b/include/osmocom/core/fsm.h
index 13bfb331..c40d7f3c 100644
--- a/include/osmocom/core/fsm.h
+++ b/include/osmocom/core/fsm.h
@@ -254,6 +254,21 @@ int _osmo_fsm_inst_state_chg(struct osmo_fsm_inst *fi, uint32_t new_state,
 int _osmo_fsm_inst_state_chg_keep_timer(struct osmo_fsm_inst *fi, uint32_t new_state,
 					const char *file, int line);
 
+/*! perform a state change while keeping the current timer if running, or starting a timer otherwise.
+ *
+ *  This is useful to keep a timeout across several states, but to make sure that some timeout is actually running.
+ *
+ *  This is a macro that calls _osmo_fsm_inst_state_chg_keep_or_start_timer() with the given
+ *  parameters as well as the caller's source file and line number for logging
+ *  purposes. See there for documentation.
+ */
+#define osmo_fsm_inst_state_chg_keep_or_start_timer(fi, new_state, timeout_secs, T) \
+	_osmo_fsm_inst_state_chg_keep_or_start_timer(fi, new_state, timeout_secs, T, \
+						     __FILE__, __LINE__)
+int _osmo_fsm_inst_state_chg_keep_or_start_timer(struct osmo_fsm_inst *fi, uint32_t new_state,
+						 unsigned long timeout_secs, int T,
+						 const char *file, int line);
+
 /*! dispatch an event to an osmocom finite state machine instance
  *
  *  This is a macro that calls _osmo_fsm_inst_dispatch() with the given
diff --git a/src/fsm.c b/src/fsm.c
index 4876c04f..5723bab4 100644
--- a/src/fsm.c
+++ b/src/fsm.c
@@ -481,11 +481,20 @@ static int state_chg(struct osmo_fsm_inst *fi, uint32_t new_state,
 		st->onleave(fi, new_state);
 
 	if (fsm_log_timeouts) {
-		if (keep_timer && fi->timer.active && (osmo_timer_remaining(&fi->timer, NULL, &remaining) == 0))
-			LOGPFSMSRC(fi, file, line, "State change to %s (keeping " OSMO_T_FMT ", %ld.%03lds remaining)\n",
-				   osmo_fsm_state_name(fsm, new_state),
-				   OSMO_T_FMT_ARGS(fi->T), remaining.tv_sec, remaining.tv_usec / 1000);
-		else if (timeout_secs && !keep_timer)
+		if (keep_timer && fi->timer.active) {
+			/* This should always give us a timeout, but just in case the return value indicates error, omit
+			 * logging the remaining time. */
+			if (osmo_timer_remaining(&fi->timer, NULL, &remaining))
+				LOGPFSMSRC(fi, file, line,
+					   "State change to %s (keeping " OSMO_T_FMT ")\n",
+					   osmo_fsm_state_name(fsm, new_state),
+					   OSMO_T_FMT_ARGS(fi->T));
+			else
+				LOGPFSMSRC(fi, file, line,
+					   "State change to %s (keeping " OSMO_T_FMT ", %ld.%03lds remaining)\n",
+					   osmo_fsm_state_name(fsm, new_state),
+					   OSMO_T_FMT_ARGS(fi->T), remaining.tv_sec, remaining.tv_usec / 1000);
+		} else if (timeout_secs)
 			LOGPFSMSRC(fi, file, line, "State change to %s (" OSMO_T_FMT ", %lus)\n",
 				   osmo_fsm_state_name(fsm, new_state),
 				   OSMO_T_FMT_ARGS(T), timeout_secs);
@@ -500,7 +509,8 @@ static int state_chg(struct osmo_fsm_inst *fi, uint32_t new_state,
 	fi->state = new_state;
 	st = &fsm->states[new_state];
 
-	if (!keep_timer) {
+	if (!keep_timer
+	    || (keep_timer && !osmo_timer_pending(&fi->timer))) {
 		fi->T = T;
 		if (timeout_secs)
 			osmo_timer_schedule(&fi->timer, timeout_secs, 0);
@@ -592,6 +602,35 @@ int _osmo_fsm_inst_state_chg_keep_timer(struct osmo_fsm_inst *fi, uint32_t new_s
 	return state_chg(fi, new_state, true, 0, 0, file, line);
 }
 
+/*! perform a state change while keeping the current timer if running, or starting a timer otherwise.
+ *
+ *  This is useful to keep a timeout across several states, but to make sure that some timeout is actually running.
+ *
+ *  Best invoke via the osmo_fsm_inst_state_chg_keep_or_start_timer() macro which logs the source file where the state
+ *  change was effected. Alternatively, you may pass file as NULL to use the normal file/line indication instead.
+ *
+ *  All changes to the FSM instance state must be made via an osmo_fsm_inst_state_chg_*
+ *  function.  It verifies that the existing state actually permits a
+ *  transition to new_state.
+ *
+ *  \param[in] fi FSM instance whose state is to change
+ *  \param[in] new_state The new state into which we should change
+ *  \param[in] timeout_secs If no timer is running yet, set this timeout in seconds (if !=0), maximum-clamped to
+ *                          2147483647 seconds.
+ *  \param[in] T Timer number, where positive numbers are considered to be 3GPP spec compliant timer numbers and are
+ *               logged as "T1234", while negative numbers are considered Osmocom specific timer numbers logged as
+ *               "X1234".
+ *  \param[in] file Calling source file (from osmo_fsm_inst_state_chg macro)
+ *  \param[in] line Calling source line (from osmo_fsm_inst_state_chg macro)
+ *  \returns 0 on success; negative on error
+ */
+int _osmo_fsm_inst_state_chg_keep_or_start_timer(struct osmo_fsm_inst *fi, uint32_t new_state,
+						 unsigned long timeout_secs, int T,
+						 const char *file, int line)
+{
+	return state_chg(fi, new_state, true, timeout_secs, T, file, line);
+}
+
 /*! dispatch an event to an osmocom finite state machine instance
  *
  *  Best invoke via the osmo_fsm_inst_dispatch() macro which logs the source
diff --git a/src/tdef.c b/src/tdef.c
index 7e79d680..692e2cf0 100644
--- a/src/tdef.c
+++ b/src/tdef.c
@@ -220,7 +220,7 @@ struct osmo_tdef *osmo_tdef_get_entry(struct osmo_tdef *tdefs, int T)
  *
  * 	struct osmo_tdef_state_timeout my_fsm_timeouts[32] = {
  * 		[MY_FSM_STATE_3] = { .T = 423 }, // look up timeout configured for T423
- * 		[MY_FSM_STATE_7] = { .T = 235 },
+ * 		[MY_FSM_STATE_7] = { .keep_timer = true, .T = 235 }, // keep previous timer if running, or start T235
  * 		[MY_FSM_STATE_8] = { .keep_timer = true }, // keep previous state's T number, continue timeout.
  * 		// any state that is omitted will remain zero == no timeout
  *	};
@@ -254,20 +254,25 @@ int _osmo_tdef_fsm_inst_state_chg(struct osmo_fsm_inst *fi, uint32_t state,
 				  const char *file, int line)
 {
 	const struct osmo_tdef_state_timeout *t = osmo_tdef_get_state_timeout(state, timeouts_array);
-	unsigned long val;
+	unsigned long val = 0;
 
 	/* No timeout defined for this state? */
 	if (!t)
 		return _osmo_fsm_inst_state_chg(fi, state, 0, 0, file, line);
 
+	if (t->T)
+		val = osmo_tdef_get(tdefs, t->T, OSMO_TDEF_S, default_timeout);
+
 	if (t->keep_timer) {
-		int rc = _osmo_fsm_inst_state_chg_keep_timer(fi, state, file, line);
-		if (t->T && !rc)
-			fi->T = t->T;
-		return rc;
+		if (t->T)
+			return _osmo_fsm_inst_state_chg_keep_or_start_timer(fi, state, val, t->T, file, line);
+		else
+			return _osmo_fsm_inst_state_chg_keep_timer(fi, state, file, line);
 	}
 
-	val = osmo_tdef_get(tdefs, t->T, OSMO_TDEF_S, default_timeout);
+	/* val is always initialized here, because if t->keep_timer is false, t->T must be != 0.
+	 * Otherwise osmo_tdef_get_state_timeout() would have returned NULL. */
+	OSMO_ASSERT(t->T);
 	return _osmo_fsm_inst_state_chg(fi, state, val, t->T, file, line);
 }
 
diff --git a/tests/tdef/tdef_test.ok b/tests/tdef/tdef_test.ok
index 135951e5..d9ef99b2 100644
--- a/tests/tdef/tdef_test.ok
+++ b/tests/tdef/tdef_test.ok
@@ -130,9 +130,9 @@ state=A T=1, 76.954322 s remaining
  --> A (configured as T1 100 s) rc=0;	state=A T=1, 100.000000 s remaining
 Time passes: 23.045678 s
 state=A T=1, 76.954322 s remaining
- --> L (configured as T123(keep_timer) 1 s) rc=0;	state=L T=123, 76.954322 s remaining
+ --> L (configured as T123(keep_timer) 1 s) rc=0;	state=L T=1, 76.954322 s remaining
  --> O (no timer configured for this state) rc=0;	state=O T=0, no timeout
- --> L (configured as T123(keep_timer) 1 s) rc=0;	state=L T=123, no timeout
+ --> L (configured as T123(keep_timer) 1 s) rc=0;	state=L T=123, 1.000000 s remaining
 - test T=0:
  --> O (no timer configured for this state) rc=0;	state=O T=0, no timeout
 - test no timer:
diff --git a/tests/tdef/tdef_test_range_64bit.ok b/tests/tdef/tdef_test_range_64bit.ok
index eed58e65..7ec295db 100644
--- a/tests/tdef/tdef_test_range_64bit.ok
+++ b/tests/tdef/tdef_test_range_64bit.ok
@@ -158,9 +158,9 @@ state=A T=1, 76.954322 s remaining
  --> A (configured as T1 100 s) rc=0;	state=A T=1, 100.000000 s remaining
 Time passes: 23.045678 s
 state=A T=1, 76.954322 s remaining
- --> L (configured as T123(keep_timer) 1 s) rc=0;	state=L T=123, 76.954322 s remaining
+ --> L (configured as T123(keep_timer) 1 s) rc=0;	state=L T=1, 76.954322 s remaining
  --> O (no timer configured for this state) rc=0;	state=O T=0, no timeout
- --> L (configured as T123(keep_timer) 1 s) rc=0;	state=L T=123, no timeout
+ --> L (configured as T123(keep_timer) 1 s) rc=0;	state=L T=123, 1.000000 s remaining
 - test T=0:
  --> O (no timer configured for this state) rc=0;	state=O T=0, no timeout
 - test no timer: