osmo-pcu[master]: EDGE: fix wrong encoding of LH bits

Harald Welte gerrit-no-reply at lists.osmocom.org
Tue Oct 10 12:43:41 UTC 2017


Patch Set 3: -Code-Review

> > > I got an error about missing QEMU when running make check
 > without
 > > > our LC15 toolchain built from Yocto framework.
 > >
 > > I suspect you're still doing a cross-compile, i.e. the
 > > './configure' was executed with the environment for your
 > > cross-toolchain?
 > >
 > > > Any suggestion?
 > >
 > > If you do a native compile on your PC, rendering an x86/x68_64
 > > executable for osmo-pcu, make check should work for master.  This
 > > is what all of us execute on our various development machines
 > (all
 > > flavors of Linux from Gentoo to Arch to Debian to Ubuntu), and
 > > which is also what our jenkins.osmoocom.org is testing
 > continuously
 > > e.g. for verifying patch submissions.
 > >
 > > In case of doubt, feel free to open a redmine issue attaching
 > your
 > > config.log and "make check" output.  Thanks!
 > 
 > I found out an issue with patch set #2 using two successive
 > bitvec_set_bit(dest, H ,2) calls making bit index increased by 4
 > bits which is not correct.
 
In my original comments I had suggested to use two consecutive 
"bitvec_set_bit(dest, H)" in order to append two "H" bits, not
to duplicate the "bitvec_write_field(dest, &wp, 3, 2);", which will
in fact append four bits (two times '2' bits), both of them '1' as
3 = 11b.

 > I have also noticed that my MS is able to send packet control ack
 > as 4 successive PRACH bursts without this patch using a clean Yocto
 > image built with up-to-date libraries.

This is good to hear, but it could simply be coincidence. Whether or
not H=1 or H=0 depends on the bit offset into the MAC block (due to
the 0x2B padding).

I still think your patch has found an important bug in the encoding
that should be fixed.

Did you validate the rest octet contents of the old code with an independent
decoder like a TEMS Pocket phone or the like?  Did you confirm the
current osmo-pcu code is performing correct encoding irrespective of
the offset of the rest octets (depends on variable-length data ahead
of the rest octets)?

 > Therefore, I would prefer to drop this patch.

I would prefer to keep it, with the "bitvec_set_bit(dest, H)" as
suggested earlier.  It should fix an encoding bug, let's not
abandon it unless we know for sure the old code is correct.

Thanks again!

-- 
To view, visit https://gerrit.osmocom.org/3991
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I75dd5bebc74eea85edf9582607c774d0bba0d2a6
Gerrit-PatchSet: 3
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: Minh-Quang Nguyen <minh-quang.nguyen at nutaq.com>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Minh-Quang Nguyen <minh-quang.nguyen at nutaq.com>
Gerrit-HasComments: No


More information about the gerrit-log mailing list