<p>Neels Hofmeyr <strong>merged</strong> this change.</p><p><a href="https://gerrit.osmocom.org/13143">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Jenkins Builder: Verified
Neels Hofmeyr: Looks good to me, approved
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">fsm: add osmo_fsm_inst_state_chg_keep_or_start_timer()<br><br>During FSM design for osmo-msc, I noticed that the current behavior that<br>keep_timer=true doesn't guarantee a running timer can make FSM design a bit<br>complex, especially when using osmo_tdef for timeout definitions.<br><br>A desirable keep_timer=true behavior is one that keeps the previous timer<br>running, but starts a timer if no timer is running yet.<br><br>The simplest example is: a given state repeatedly transitions back to itself,<br>but wants to set a timeout only on first entering, avoiding to restart the<br>timeout on re-entering.<br><br>Another example is a repeated transition between two or more states, where the<br>first time we enter this group a timeout should start, but it should not<br>restart from scratch on every transition.<br><br>When using osmo_tdef timeout definitions for this, so far separate meaningless<br>states have to be introduced that merely set a fixed timeout.<br><br>To simplify, add osmo_fsm_inst_state_chg_keep_or_start_timer(), and use this in<br>osmo_tdef_fsm_inst_state_chg() when both keep_timer == true *and* T != 0.<br><br>In tdef_test.ok, the changes show that on first entering state L, the previous<br>T=1 is now kept with a large remaining timeout. When entering state L from O,<br>where no timer was running, this time L's T123 is started.<br><br>Change-Id: Id647511a4b18e0c4de0e66fb1f35dc9adb9177db<br>---<br>M include/osmocom/core/fsm.h<br>M src/fsm.c<br>M src/tdef.c<br>M tests/tdef/tdef_test.ok<br>M tests/tdef/tdef_test_range_64bit.ok<br>5 files changed, 76 insertions(+), 17 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/include/osmocom/core/fsm.h b/include/osmocom/core/fsm.h</span><br><span>index 13bfb33..c40d7f3 100644</span><br><span>--- a/include/osmocom/core/fsm.h</span><br><span>+++ b/include/osmocom/core/fsm.h</span><br><span>@@ -254,6 +254,21 @@</span><br><span> int _osmo_fsm_inst_state_chg_keep_timer(struct osmo_fsm_inst *fi, uint32_t new_state,</span><br><span> const char *file, int line);</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+/*! perform a state change while keeping the current timer if running, or starting a timer otherwise.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * This is useful to keep a timeout across several states, but to make sure that some timeout is actually running.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * This is a macro that calls _osmo_fsm_inst_state_chg_keep_or_start_timer() with the given</span><br><span style="color: hsl(120, 100%, 40%);">+ * parameters as well as the caller's source file and line number for logging</span><br><span style="color: hsl(120, 100%, 40%);">+ * purposes. See there for documentation.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+#define osmo_fsm_inst_state_chg_keep_or_start_timer(fi, new_state, timeout_secs, T) \</span><br><span style="color: hsl(120, 100%, 40%);">+ _osmo_fsm_inst_state_chg_keep_or_start_timer(fi, new_state, timeout_secs, T, \</span><br><span style="color: hsl(120, 100%, 40%);">+ __FILE__, __LINE__)</span><br><span style="color: hsl(120, 100%, 40%);">+int _osmo_fsm_inst_state_chg_keep_or_start_timer(struct osmo_fsm_inst *fi, uint32_t new_state,</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned long timeout_secs, int T,</span><br><span style="color: hsl(120, 100%, 40%);">+ const char *file, int line);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /*! dispatch an event to an osmocom finite state machine instance</span><br><span> *</span><br><span> * This is a macro that calls _osmo_fsm_inst_dispatch() with the given</span><br><span>diff --git a/src/fsm.c b/src/fsm.c</span><br><span>index 4876c04..5723bab 100644</span><br><span>--- a/src/fsm.c</span><br><span>+++ b/src/fsm.c</span><br><span>@@ -481,11 +481,20 @@</span><br><span> st->onleave(fi, new_state);</span><br><span> </span><br><span> if (fsm_log_timeouts) {</span><br><span style="color: hsl(0, 100%, 40%);">- if (keep_timer && fi->timer.active && (osmo_timer_remaining(&fi->timer, NULL, &remaining) == 0))</span><br><span style="color: hsl(0, 100%, 40%);">- LOGPFSMSRC(fi, file, line, "State change to %s (keeping " OSMO_T_FMT ", %ld.%03lds remaining)\n",</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_fsm_state_name(fsm, new_state),</span><br><span style="color: hsl(0, 100%, 40%);">- OSMO_T_FMT_ARGS(fi->T), remaining.tv_sec, remaining.tv_usec / 1000);</span><br><span style="color: hsl(0, 100%, 40%);">- else if (timeout_secs && !keep_timer)</span><br><span style="color: hsl(120, 100%, 40%);">+ if (keep_timer && fi->timer.active) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* This should always give us a timeout, but just in case the return value indicates error, omit</span><br><span style="color: hsl(120, 100%, 40%);">+ * logging the remaining time. */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (osmo_timer_remaining(&fi->timer, NULL, &remaining))</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGPFSMSRC(fi, file, line,</span><br><span style="color: hsl(120, 100%, 40%);">+ "State change to %s (keeping " OSMO_T_FMT ")\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_fsm_state_name(fsm, new_state),</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_T_FMT_ARGS(fi->T));</span><br><span style="color: hsl(120, 100%, 40%);">+ else</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGPFSMSRC(fi, file, line,</span><br><span style="color: hsl(120, 100%, 40%);">+ "State change to %s (keeping " OSMO_T_FMT ", %ld.%03lds remaining)\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_fsm_state_name(fsm, new_state),</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_T_FMT_ARGS(fi->T), remaining.tv_sec, remaining.tv_usec / 1000);</span><br><span style="color: hsl(120, 100%, 40%);">+ } else if (timeout_secs)</span><br><span> LOGPFSMSRC(fi, file, line, "State change to %s (" OSMO_T_FMT ", %lus)\n",</span><br><span> osmo_fsm_state_name(fsm, new_state),</span><br><span> OSMO_T_FMT_ARGS(T), timeout_secs);</span><br><span>@@ -500,7 +509,8 @@</span><br><span> fi->state = new_state;</span><br><span> st = &fsm->states[new_state];</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!keep_timer) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!keep_timer</span><br><span style="color: hsl(120, 100%, 40%);">+ || (keep_timer && !osmo_timer_pending(&fi->timer))) {</span><br><span> fi->T = T;</span><br><span> if (timeout_secs)</span><br><span> osmo_timer_schedule(&fi->timer, timeout_secs, 0);</span><br><span>@@ -592,6 +602,35 @@</span><br><span> return state_chg(fi, new_state, true, 0, 0, file, line);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+/*! perform a state change while keeping the current timer if running, or starting a timer otherwise.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * This is useful to keep a timeout across several states, but to make sure that some timeout is actually running.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Best invoke via the osmo_fsm_inst_state_chg_keep_or_start_timer() macro which logs the source file where the state</span><br><span style="color: hsl(120, 100%, 40%);">+ * change was effected. Alternatively, you may pass file as NULL to use the normal file/line indication instead.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * All changes to the FSM instance state must be made via an osmo_fsm_inst_state_chg_*</span><br><span style="color: hsl(120, 100%, 40%);">+ * function. It verifies that the existing state actually permits a</span><br><span style="color: hsl(120, 100%, 40%);">+ * transition to new_state.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] fi FSM instance whose state is to change</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] new_state The new state into which we should change</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] timeout_secs If no timer is running yet, set this timeout in seconds (if !=0), maximum-clamped to</span><br><span style="color: hsl(120, 100%, 40%);">+ * 2147483647 seconds.</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] T Timer number, where positive numbers are considered to be 3GPP spec compliant timer numbers and are</span><br><span style="color: hsl(120, 100%, 40%);">+ * logged as "T1234", while negative numbers are considered Osmocom specific timer numbers logged as</span><br><span style="color: hsl(120, 100%, 40%);">+ * "X1234".</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] file Calling source file (from osmo_fsm_inst_state_chg macro)</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] line Calling source line (from osmo_fsm_inst_state_chg macro)</span><br><span style="color: hsl(120, 100%, 40%);">+ * \returns 0 on success; negative on error</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+int _osmo_fsm_inst_state_chg_keep_or_start_timer(struct osmo_fsm_inst *fi, uint32_t new_state,</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned long timeout_secs, int T,</span><br><span style="color: hsl(120, 100%, 40%);">+ const char *file, int line)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ return state_chg(fi, new_state, true, timeout_secs, T, file, line);</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /*! dispatch an event to an osmocom finite state machine instance</span><br><span> *</span><br><span> * Best invoke via the osmo_fsm_inst_dispatch() macro which logs the source</span><br><span>diff --git a/src/tdef.c b/src/tdef.c</span><br><span>index 7e79d68..692e2cf 100644</span><br><span>--- a/src/tdef.c</span><br><span>+++ b/src/tdef.c</span><br><span>@@ -220,7 +220,7 @@</span><br><span> *</span><br><span> * struct osmo_tdef_state_timeout my_fsm_timeouts[32] = {</span><br><span> * [MY_FSM_STATE_3] = { .T = 423 }, // look up timeout configured for T423</span><br><span style="color: hsl(0, 100%, 40%);">- * [MY_FSM_STATE_7] = { .T = 235 },</span><br><span style="color: hsl(120, 100%, 40%);">+ * [MY_FSM_STATE_7] = { .keep_timer = true, .T = 235 }, // keep previous timer if running, or start T235</span><br><span> * [MY_FSM_STATE_8] = { .keep_timer = true }, // keep previous state's T number, continue timeout.</span><br><span> * // any state that is omitted will remain zero == no timeout</span><br><span> * };</span><br><span>@@ -254,20 +254,25 @@</span><br><span> const char *file, int line)</span><br><span> {</span><br><span> const struct osmo_tdef_state_timeout *t = osmo_tdef_get_state_timeout(state, timeouts_array);</span><br><span style="color: hsl(0, 100%, 40%);">- unsigned long val;</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned long val = 0;</span><br><span> </span><br><span> /* No timeout defined for this state? */</span><br><span> if (!t)</span><br><span> return _osmo_fsm_inst_state_chg(fi, state, 0, 0, file, line);</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ if (t->T)</span><br><span style="color: hsl(120, 100%, 40%);">+ val = osmo_tdef_get(tdefs, t->T, OSMO_TDEF_S, default_timeout);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> if (t->keep_timer) {</span><br><span style="color: hsl(0, 100%, 40%);">- int rc = _osmo_fsm_inst_state_chg_keep_timer(fi, state, file, line);</span><br><span style="color: hsl(0, 100%, 40%);">- if (t->T && !rc)</span><br><span style="color: hsl(0, 100%, 40%);">- fi->T = t->T;</span><br><span style="color: hsl(0, 100%, 40%);">- return rc;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (t->T)</span><br><span style="color: hsl(120, 100%, 40%);">+ return _osmo_fsm_inst_state_chg_keep_or_start_timer(fi, state, val, t->T, file, line);</span><br><span style="color: hsl(120, 100%, 40%);">+ else</span><br><span style="color: hsl(120, 100%, 40%);">+ return _osmo_fsm_inst_state_chg_keep_timer(fi, state, file, line);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- val = osmo_tdef_get(tdefs, t->T, OSMO_TDEF_S, default_timeout);</span><br><span style="color: hsl(120, 100%, 40%);">+ /* val is always initialized here, because if t->keep_timer is false, t->T must be != 0.</span><br><span style="color: hsl(120, 100%, 40%);">+ * Otherwise osmo_tdef_get_state_timeout() would have returned NULL. */</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(t->T);</span><br><span> return _osmo_fsm_inst_state_chg(fi, state, val, t->T, file, line);</span><br><span> }</span><br><span> </span><br><span>diff --git a/tests/tdef/tdef_test.ok b/tests/tdef/tdef_test.ok</span><br><span>index 135951e..d9ef99b 100644</span><br><span>--- a/tests/tdef/tdef_test.ok</span><br><span>+++ b/tests/tdef/tdef_test.ok</span><br><span>@@ -130,9 +130,9 @@</span><br><span> --> A (configured as T1 100 s) rc=0; state=A T=1, 100.000000 s remaining</span><br><span> Time passes: 23.045678 s</span><br><span> state=A T=1, 76.954322 s remaining</span><br><span style="color: hsl(0, 100%, 40%);">- --> L (configured as T123(keep_timer) 1 s) rc=0; state=L T=123, 76.954322 s remaining</span><br><span style="color: hsl(120, 100%, 40%);">+ --> L (configured as T123(keep_timer) 1 s) rc=0; state=L T=1, 76.954322 s remaining</span><br><span> --> O (no timer configured for this state) rc=0; state=O T=0, no timeout</span><br><span style="color: hsl(0, 100%, 40%);">- --> L (configured as T123(keep_timer) 1 s) rc=0; state=L T=123, no timeout</span><br><span style="color: hsl(120, 100%, 40%);">+ --> L (configured as T123(keep_timer) 1 s) rc=0; state=L T=123, 1.000000 s remaining</span><br><span> - test T=0:</span><br><span> --> O (no timer configured for this state) rc=0; state=O T=0, no timeout</span><br><span> - test no timer:</span><br><span>diff --git a/tests/tdef/tdef_test_range_64bit.ok b/tests/tdef/tdef_test_range_64bit.ok</span><br><span>index eed58e6..7ec295d 100644</span><br><span>--- a/tests/tdef/tdef_test_range_64bit.ok</span><br><span>+++ b/tests/tdef/tdef_test_range_64bit.ok</span><br><span>@@ -158,9 +158,9 @@</span><br><span> --> A (configured as T1 100 s) rc=0; state=A T=1, 100.000000 s remaining</span><br><span> Time passes: 23.045678 s</span><br><span> state=A T=1, 76.954322 s remaining</span><br><span style="color: hsl(0, 100%, 40%);">- --> L (configured as T123(keep_timer) 1 s) rc=0; state=L T=123, 76.954322 s remaining</span><br><span style="color: hsl(120, 100%, 40%);">+ --> L (configured as T123(keep_timer) 1 s) rc=0; state=L T=1, 76.954322 s remaining</span><br><span> --> O (no timer configured for this state) rc=0; state=O T=0, no timeout</span><br><span style="color: hsl(0, 100%, 40%);">- --> L (configured as T123(keep_timer) 1 s) rc=0; state=L T=123, no timeout</span><br><span style="color: hsl(120, 100%, 40%);">+ --> L (configured as T123(keep_timer) 1 s) rc=0; state=L T=123, 1.000000 s remaining</span><br><span> - test T=0:</span><br><span> --> O (no timer configured for this state) rc=0; state=O T=0, no timeout</span><br><span> - test no timer:</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/13143">change 13143</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/13143"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: Id647511a4b18e0c4de0e66fb1f35dc9adb9177db </div>
<div style="display:none"> Gerrit-Change-Number: 13143 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>