Hi.
Right now in osmobts when sending/receiving frames with osmo_rtp_* it's
assumed that no frame is lost and timestamp is always advanced in 160ms
steps. In practice (especially when DTX is in place) frames do get lost
so we have to adjust the step to compensate.
I've tried to do it as follows:
- store frame number of last used frame on receiving/sending
- check how far current frame number from old one
- convert frame number delta to ms: each frame is 4.615 ms long
- if delta in ms is bigger than default 160 than use it
However the result sound not much better than using hardcoded value
which suggest that I might be doing FN -> ms conversion (or smth else)
wrong. Any ideas/advices?
--
Max Suraev <msuraev(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
On Wed, Jun 01, 2016 at 09:02:47PM +0200, Holger Freyther wrote:
> > On 01 Jun 2016, at 14:21, Neels Hofmeyr <nhofmeyr(a)sysmocom.de> wrote:
> >
> > It would also be nice if Gerrit didn't complain about merge conflicts that are
> > in fact no merge conflicts -- I'm stumped on that topic. Any help would be
> > appreciated.
>
> https://code.google.com/p/gerrit/issues/detail?id=2734
Thanks! #5 is extremely helpful in understanding Gerrit: I did absolutely not
expect the term "merge conflict" to actually mean "path conflict".
https://code.google.com/p/gerrit/issues/detail?id=2734#c5
What the post says to be "Automatically resolve conflicts" is probably "Allow
content merges" in our Gerrit version. It's now enabled in our projects (thanks
Holger).
Next I'd like to retry the "Rebase if necessary". Currently openbsc was on
"Merge if necessary", which, it seems, creates merges that make the history
more complex than rebases would.
I've put openbsc to "Rebase if necessary" now and posted the next couple iups
commits to for/master.
This time I don't see "Merge conflicts", probably thanks to "Allow content
merges"; excellent.
I have yet another problem: it seems I can't re-submit a branch; during push, I
get an error saying:
! [remote rejected] HEAD -> refs/for/master (no new changes)
I've resolved "no new changes" before by manually removing the Change-Id from
each commit message (kind of unsatisfactory solution). This means I can't
submit fixes to the branch commits. I would like to add the NULL check
described in the comment here:
https://gerrit.osmocom.org/#/c/171/1/openbsc/src/gprs/gprs_gmm.c
but it seems I would have to abandon the entire set and submit it again, and
would also have to amend every commit message to generate a new Change-Id in
it. (This particular change could well be added in a later commit, but it
makes me wonder how we work with requests to tweak such a commit in general.)
https://groups.google.com/forum/?_escaped_fragment_=topic/repo-discuss/NeuQ…
^ This looks like gerrit and branches just don't mix too well.
https://groups.google.com/forum/?_escaped_fragment_=topic/repo-discuss/tmiy…
^ In here there's also the suggestion to push a merge-commit to for/master
instead, so I might try it next time...
BTW I think I was wrong about the shuffling of commits in the listing. It
appears the newest branch commit is always on the bottom. (Except in "Related
changes", where it is always on top.)
I guess we should have a "submitting branches in gerrit" wiki page once we've
settled for our favourite strategy.
~Neels
--
- Neels Hofmeyr <nhofmeyr(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschäftsführer / Managing Directors: Holger Freyther, Harald Welte
Review at https://gerrit.osmocom.org/168
gprs_gmm.c: Don't try to de-reference NULL mmctx
There was a comment in the code that certain GMM messages require a
valid mmctx pointer. However, nothing actually checked if that pointer
was in fact non-NULL. We plainly crashed if a MS would send us the
wrong message in the wrong state.
Original patch by Harald Welte, but it broke message validity checking,
resulting in sgsn_test failure. This re-implements the NULL check in a
different way, as explained by in-code comment.
Change-Id: I34b47b9e63691c9bc9904573000c74877217f679
---
M openbsc/src/gprs/gprs_gmm.c
1 file changed, 32 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/68/168/1
diff --git a/openbsc/src/gprs/gprs_gmm.c b/openbsc/src/gprs/gprs_gmm.c
index 1e10701..e788e3d 100644
--- a/openbsc/src/gprs/gprs_gmm.c
+++ b/openbsc/src/gprs/gprs_gmm.c
@@ -1339,6 +1339,15 @@
return rc;
}
+ /*
+ * For a few messages, mmctx may be NULL. For most, we want to ensure a
+ * non-NULL mmctx. At the same time, we want to keep the message
+ * validity check intact, so that all message types appear in the
+ * switch statement and the default case thus means "unknown message".
+ * If we split the switch in two parts to check non-NULL halfway, the
+ * unknown-message check breaks, or we'd need to duplicate the switch
+ * cases in both parts. Just keep one large switch and add some gotos.
+ */
switch (gh->msg_type) {
case GSM48_MT_GMM_RA_UPD_REQ:
rc = gsm48_rx_gmm_ra_upd_req(mmctx, msg, llme);
@@ -1348,20 +1357,30 @@
break;
/* For all the following types mmctx can not be NULL */
case GSM48_MT_GMM_ID_RESP:
+ if (!mmctx)
+ goto null_mmctx;
rc = gsm48_rx_gmm_id_resp(mmctx, msg);
break;
case GSM48_MT_GMM_STATUS:
+ if (!mmctx)
+ goto null_mmctx;
rc = gsm48_rx_gmm_status(mmctx, msg);
break;
case GSM48_MT_GMM_DETACH_REQ:
+ if (!mmctx)
+ goto null_mmctx;
rc = gsm48_rx_gmm_det_req(mmctx, msg);
break;
case GSM48_MT_GMM_DETACH_ACK:
+ if (!mmctx)
+ goto null_mmctx;
LOGMMCTXP(LOGL_INFO, mmctx, "-> DETACH ACK\n");
mm_ctx_cleanup_free(mmctx, "GPRS DETACH ACK");
rc = 0;
break;
case GSM48_MT_GMM_ATTACH_COMPL:
+ if (!mmctx)
+ goto null_mmctx;
/* only in case SGSN offered new P-TMSI */
LOGMMCTXP(LOGL_INFO, mmctx, "-> ATTACH COMPLETE\n");
mmctx_timer_stop(mmctx, 3350);
@@ -1380,6 +1399,8 @@
osmo_signal_dispatch(SS_SGSN, S_SGSN_ATTACH, &sig_data);
break;
case GSM48_MT_GMM_RA_UPD_COMPL:
+ if (!mmctx)
+ goto null_mmctx;
/* only in case SGSN offered new P-TMSI */
LOGMMCTXP(LOGL_INFO, mmctx, "-> ROUTING AREA UPDATE COMPLETE\n");
mmctx_timer_stop(mmctx, 3350);
@@ -1398,6 +1419,8 @@
osmo_signal_dispatch(SS_SGSN, S_SGSN_UPDATE, &sig_data);
break;
case GSM48_MT_GMM_PTMSI_REALL_COMPL:
+ if (!mmctx)
+ goto null_mmctx;
LOGMMCTXP(LOGL_INFO, mmctx, "-> PTMSI REALLLICATION COMPLETE\n");
mmctx_timer_stop(mmctx, 3350);
mmctx->t3350_mode = GMM_T3350_MODE_NONE;
@@ -1409,6 +1432,8 @@
rc = 0;
break;
case GSM48_MT_GMM_AUTH_CIPH_RESP:
+ if (!mmctx)
+ goto null_mmctx;
rc = gsm48_rx_gmm_auth_ciph_resp(mmctx, msg);
break;
default:
@@ -1419,6 +1444,13 @@
}
return rc;
+
+null_mmctx:
+ LOGP(DMM, LOGL_ERROR,
+ "Received GSM 04.08 message type 0x%02x,"
+ " but no MM context available\n",
+ gh->msg_type);
+ return -EINVAL;
}
static void mmctx_timer_cb(void *_mm)
--
To view, visit https://gerrit.osmocom.org/168
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I34b47b9e63691c9bc9904573000c74877217f679
Gerrit-PatchSet: 1
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr(a)sysmocom.de>