It could happen as of current implementation that a TBF was created as
GPRS due to the MS being non-egprs, and later on the MS was upgraded to
EGPRS due to newly received information from the MS.
Hence, in order to infer if the data block is EGPRS or GPRS, let's
better check for the TBF info, which is the one really mandating the
kind of dl block to generate.
Change-Id: I49720fb3a69ca972cd1973de937ac8ee77615431
MS is always assigned to a BTS, since it's set during MS constructor.
Hence, the check removed in this patch would never hold true (and if it
did, it'd be a bug).
Change-Id: I86a71c64623f7bec031226938a54306148370ffb
it was noticed that gprs_pcu_get_bts_by_cgi_ps() sometimes failed to
return the BTS even if the CGI-PS fields matched, probably due to memcmp
checking too padding bytes which may not be zero-initialized in one of
the two memory regions being checked. Let's be on the safe side and use
libosmocore APIs to check them.
Depends: libosmocore.git Change-Id I00e329bc5be8674b30267dec238e7656ddfc21db
Change-Id: I7c8ee2c447634e45b367bb8f84adf0140ae48591
This code doesn't do anything yet app-related with the received RIM
messages, but already provides the initial infrastructure to handle them
in the future, and does first checkings.
Related: SYS#5103
Change-Id: Ia0ade0e97ea781ec655439c008b6cefaf3e90dec
This patch doesn't really tests whether osmo-pcu can work on a multi-bts
environment, but it prepares the data structures to be able to do so at
any later point in time.
Change-Id: I6b10913f46c19d438c4e250a436a7446694b725a
There's no real point in using C++ there, and using C++ makes the
compiler fail to use llist_head in multi-bts patches added later due to:
"""
'offsetof' within non-standard-layout type is conditionally-supported
"""
Change-Id: I8965b5cc5a713e64788b5b6aa183d3035341ddbb
There's no real use of C++ in that file, and it causes problems when
using llist_head entry macros in future patches adding initial support
for multiple BTS in PCU object, so let's move it to plain C.
Change-Id: Ic771a89fd78b5e66151a5384f0ff6a8895589466
Those files are not really being used other than for calling
get_current_fn() which is just a placeholder to call
bts_current_frame_number on the global bts object.
Change-Id: I6d50a8c15c1de5e2a308a24b313a7776f94ae54f
The bts_init/cleanup functions were kept during the C and C++ structure
merge process to make the patch simpler. It's not needed anymore,
let's move all the destructor logic into one function and keep that
together.
Change-Id: I73a9457d5c92f62261561ef6afe392953576aec4
Before, we used tho have a BTs object split into 2 parts, a C
gprs_rlcmac_bts struct and a C++ BTS struct, and "bts_data" naming was
used to distinguish them in variable names. Nowadays the struct is
finally combined into one, so there's no point in using this "bts_data"
terminology, we use always "bts".
Change-Id: I9852bf439292d1abc70711bea65698b21bde0ee8
There's no BTS single global object anymore, get rid of those APIs. Move
users to use "pcu->bts", which will evolve to a linked list in the
future.
Change-Id: I9cf762b0d3cb9e2cc3582727e07fa82c8e183ec5
Previous work on BTS class started to get stuff out of the C++ struct
into a C struct (BTS -> struct gprs_glcmac_bts) so that some parts of
it were accessible from C code. Doing so, however, ended up being messy
too, since all code needs to be switching from one object to another,
which actually refer to the same logical component.
Let's instead rejoin the structures and make sure the struct is
accessible and usable from both C and C++ code by rewriting all methods
to be C compatible and converting 3 allocated suboject as pointers.
This way BTS can internally still use those C++ objects while providing
a clean APi to both C and C++ code.
Change-Id: I7d12c896c5ded659ca9d3bff4cf3a3fc857db9dd
Both values (optionally) set (forced) by VTY and the values received
from PCUIF were stored in the same variable, meaning that for instance
the PCUIF values wouldn't really be used if someone applied eg "no cs"
during runtime.
This commit does something similar to what was already done for the
max_(m)cs fields. We store PCUIF values in one place and VTY ones in
another place, and then trigger a bts object internal process to find
out exactly which initial CS should it be using.
Change-Id: I80a6ba401f9c0c85bdf6e0cc99a9d2008d31e1b0
Currently the BTS object (and gprs_rlcmac_bts struct) are used to hold
both PCU global fields and BTS specific fields, all mangled together.
The BTS is even accessed in lots of places by means of a singleton.
This patch introduces a new struct gprs_pcu object aimed at holding all
global state, and several fields are already moved from BTS to it. The
new object can be accessed as global variable "the_pcu", reusing and
including an already exisitng "the_pcu" global variable only used for
bssgp related purposes so far.
This is only a first step towards having a complete split global pcu and
BTS, some fields are still kept in BTS and will be moved over follow-up
smaller patches in the future (since this patch is already quite big).
So far, the code still only supports one BTS, which can be accessed
using the_pcu->bts. In the future that field will be replaced with a
list, and the BTS singletons will be removed.
The cur_fn output changes in TbfTest are actually a side effect fix,
since the singleton main_bts() now points internally to the_pcu->bts,
hence the same we allocate and assign in the test. Beforehand, "the_bts"
was allocated in the stack while main_bts() still returned an unrelated
singleton BTS object instance.
Related: OS#4935
Change-Id: I88e3c6471b80245ce3798223f1a61190f14aa840
When allocating multiple slots for a UE the following example
is not allowed 'UU----UU' for a UE class 12.
The time slot number can not roll over 7 and move to 0.
44.060 or 45.002 only specifies contigous however it was unclear
it this is an allowed pattern.
Only the example 45.002 B.3 in release 12 cleared this up.
It gives an example for a multi slot class 5 UE which has 7 possible
configuration this means the rolled over is not allowed.
Multislot class type 2 UE doesn't have this limitation.
Further if a UE supports 8 time slots this is not a limitation because
the window size (45.002 B.1) can include all time slots.
Releated: SYS#5073
Change-Id: I16019bdbe741b37b83b62749b840a3b7f4ddc6c7
Under some platforms (RPI4, ARM) container older ASan, it will log false
positive log errors which will make unit test fail because then output
changes:
"""
pcu_l1_if.cpp:847:2: runtime error: member access within misaligned address 0xb3f0b78c for type 'struct GprsMs', which requires 8 byte alignment
"""
The pointer is indeed misaligned, but it's not actually a bug, because
the pointer is never derreferenced. That happens during
llist_for_each_entry operation where it does cast the pointer but it
only checks if the list has actually reached the end.
To workaround the issue, simply defer casting it by using llist_for_each
instead, where the pointer is assigned only in the case it really points
to a GprsMS struct.
Change-Id: I149fb42706501eb33f9c6fe48f76a03ddee5954a
Unfortunately, RACH.ind on the PCU interface contains no Uplink
measurements: neiter RSSI nor C/I. In order to avoid sending
garbage, let's zero-initialize 'struct pcu_l1_meas'.
Change-Id: I8c3210c428da17d23d798f3ef9df941ded6e162a
Fixes: CID#216512
Let's use usual osmocom rate_ctr instead of having one variable +
setter/getter functions, so we can easily add new counters and also
because it makes code more clear (no need to look at what the "update"
function is doing).
Using rate counter also provides info about how recently the MS has been
interacting with the network.
Related: OS#4907
Change-Id: I744507fde4291955c1dbbb9739b18a12a80145b1
It is expected that the tbf object is freed at any moment in time, for
instance if osmo-pcu drops PCUIF connection with osmo-bts. I couldn't
find any reason why it would e dangerous to free the tbf, so let's
remove this message.
related: OS#4779
Change-Id: I4ab5ccaa5bf6257b18d8fd5ba06baab083821817
This ensures spec compliance, because currently the base class
destructor would be called through a base class pointer to derived
class instead of the most derived one, which ist unexpected and actually
undefined behavior in c++11 and beyond.
Change-Id: Ic4abde1658a983bb0ccf9a526177dce50ff6dc23
There is a big difference between:
if ((a = foo() != 0xffffffff)) { ... }
and
if ((a = foo()) != 0xffffffff) { ... }
In the first case, 'a' is the result of '!=' operation, i.e. either
0 (false) or 1 (true). In the second case, 'a' will hold the value
returned by foo(), and this is exactly what must have been used in
gprs_rlcmac_rcv_rts_block().
The bug was there since SBA allocation feature was added in 2012.
Change-Id: Ifd607ae8a33382e48f9d9e50a28a4bdf4eaf73a2
Fixes: 07e97cf8a5
Related: CID#215835
As we integrate osmo-pcu more and more with libosmocore features, it
becomes really hard to use them since libosmocore relies heavily on C
specific compilation features, which are not available in old C++
compilers (such as designated initializers for complex types in FSMs).
GprsMs is right now a quite simple object since initial design of
osmo-pcu made it optional and most of the logic was placed and stored
duplicated in TBF objects. However, that's changing as we introduce more
features, with the GprsMS class getting more weight. Hence, let's move
it now to be a C struct in order to be able to easily use libosmocore
features there, such as FSMs.
Some helper classes which GprsMs uses are also mostly move to C since
they are mostly structs with methods, so there's no point in having
duplicated APIs for C++ and C for such simple cases.
For some more complex classes, like (ul_,dl_)tbf, C API bindings are
added where needed so that GprsMs can use functionalitites from that
class. Most of those APIs can be kept afterwards and drop the C++ ones
since they provide no benefit in general.
Change-Id: I0b50e3367aaad9dcada76da97b438e452c8b230c
The resulting code is cleaner since it becomes clear the relation
between all those pointers, which are set in one function and used in
another one, passed through the caller of the former two.
Moreover, if more tbf candidates are to be added for other type of
actions, having them in a struct is much easier since only one pointer
is passed.
Change-Id: I55482aa5af7be5a176a7b4879a672ad37e618020
NS2 introduce a ns dialect to differentiate
between the 4 possible dialects.
Related: OS#4472, OS#4890
Depends: libosmocore.git Ia118bb6f994845d84db09de7a94856f5ca573404
Change-Id: I16dc82c38eb75c2b9d1197640a955fec7df84efc
According to:
* 3GPP TS 44.060 version 16.0.0 "9.3.1a Delayed release of downlink Temporary Block Flow"
* 3GPP TS 44.064 version 16.0.0 "6.4.2.2 Unconfirmed Information (UI) Dummy command"
LLC UI Dummy frames are to be used when there no more data to send, only
in order to delay the release of a TBF. Hence, while not incorrect per
se, makes no sense to send those LLC UI Dummy frames inserted into
rlcmac blocks which already contain other LLC frames, since the MS in
that case is already being kept active.
It only makes sense to send those LLC UI Dummy frames when we have
nothing else to send, that is, alone inside a RLCMAC block without other
LLC frames.
Related: OS#4849
Change-Id: Ifae1a7b2b3dfad8df19585063088ba0df2749c8f
Use prim_str() method to get the human readable string.
Define unhandled events with a nop in the switch()
Depends-on: Ibf610fbd929dddc4a4e235152447caff522d4eb2 (libosmocore)
Change-Id: I50188afb83ac142e22d4bda4e8050eb4de962e70
In previous status, if USF for GPRS-only MS was selected, then EGPRS
TBFs were skipped and either a GPRS TBF was selected or a Dummy Block
was sent. That means the behavior was unfair towards EGPRS TBFs, because
sometimes they were skipped in favor of GPRS ones.
This patch imporves the situation in the above mentioned USF scenario, by
first, under specific conditions, allowing selection of an EGPRS TBF and
then forcing it to transmit in EGPRS-GMSK (MCS1-4) so that the
USF-targeted MS can still decode the USF, while at the same time
providing more fairness by allowing the EGPRS TBF to transmit data.
The specific conditions mentioned above are, mainly, related to the fact
that once a DL data block has been sent, and hence a BSN was assigned to
it, it cannot be retransmitted later using another MCS, since lower
MCS1-4 wouldn't be able to contain higher MCS RLC payload.
The set of conditions could be expanded in the future by also selecting
the EGPRS TBF if retransmition is required and the block to be
retransmitted was originally transmitted as MCS1-4.
Related: OS#4544
Change-Id: I9af23e175435fe9ae7b0e4119ad52fcd4707b9ca
Previous code relied on abort() switching sigaction to SIG_FDL +
retriggering SIGABRT in case the signal handler returns, which would
then generate the coredump + terminate the process.
However, if a SIGABRT is received from somewhere else (kill -SIGABRT),
then the process would print the talloc report and continue running,
which is not desired.
Change-Id: I8f02925eedd8855bb58555df20b443f79d5c6da8
Fixes: OS#4865
There's actually 3 errors:
* Its value should be updated, not the pointer itself
* Value should be increased, not decreased
* bitvec_read_field() API is already advancing it, no need to do it
Fixes: OS#4838
Change-Id: I009abc373794e148091e637ffee80c6461960945
dequeue_paging() dequeues the first paging (at the start of the list).
If a paging request is dequeued but later it cannot be added to the
message being sent, it has to be re-added to the list for later
processing on next message. However, existing code was enqueueing it at
the end, which meant that paging request was delayed for no reason.
Change-Id: Iad8e7045267d56e32f42db0fbb8448b1b1185f05
There are some restrictions to have both GPRS-only and EGPRS MS attached
to the same MS:
* Any MS needs to be able to successfully decode a DL block at least
every 18 DL blocks (360 ms). That means a Dl block with CS1-4 must be
sent at least once during that time.
* Any MS needs to be able to decode USF targeting it. GPRS-only MS can
successfully decode USF from DL blocks using GMSK: CS1-4 and MCS1-4.
In this patch, if USF of a GPRS-only MS is selected, then all DL EGPRS
TBFs are discarded from data block selection. However, this logic can be
further improved later by still allowing selection of DL EGPRS TBFs and
then forcing construction of a DL EGPRS data block using MCS1-4.
Sources:
* 3GPP TS 03.64 version 8.12.0 "6.6.4.1.1.2 Multiplexing of GPRS and EGPRS MSs"
* 3GPP TS 05.08 version 8.23.0 "10.2.2 BTS output power"
Related: OS#4544
Change-Id: Ib4991c864eda6864533363443f76ae5d999532ae
Recent commit removed the counter enum but forgot to remove the
description, so the descriptions were all shifted by 1 counter.
Fixes: 133fe4a852
Change-Id: I82ee9f36d60a1fd129ae3a864508fcd886e4bfef
The assumption that TLLI 0x00000000 is invalid and can be used
as the initializer is wrong. Similar to TMSI, 0x00000000 is a
perfectly valid value, while 0xffffffff is reserved - use it.
According to 3GPP TS 23.003, section 2.4, a TMSI/P-TMSI with
all 32 bits equal to 1 is special and shall not be allocated by
the network. The reason is that it must be stored on the SIM,
where 'ff'O represents the erased state. According to section
2.6 of the same document, a local/foreign TLLI is derived from
P-TMSI, so the same rule applies to TLLI.
I manually checked and corrected all occurances of 'tlli' in the
code. The test expectations have been adjusted with this command:
$ find tests/ -name "*.err" | xargs sed -i "s/0x00000000/0xffffffff/g"
so there should be no behavior change. The only exception is
the 'TypesTest', where TLLI 0xffffffff is being encoded and
expected in the hexdump, so I regenerated the test output.
Change-Id: Ie89fab75ecc1d8b5e238d3ff214ea7ac830b68b5
Related: OS#4844
This patch avoids enabling EGPRS on MS objects if BTS/VTY assigned no
MCS supported/available for use.
As a result, if NO MCS is enabled/supported EGPRS won't be used despite
the MS announcing through EGPRS MS class that it supports EGPRS.
Change-Id: Ib19e9e006d851c2147de15f4aec36ab65250bdd3
Also add a few more asserts to make sure a valid CS/MCS is passed in
some placed where we expect (M)CS to be set.
Change-Id: I0a973e10cd9477f72d8bd47a06048414b33ae96a
Properly clip initial_(m)cs values to be lower-equal than maximum
configured.
Regarding initial_mcs, use values provided by BTS, which were not used
before.
Change-Id: Ifc6bc7c2734d1ae404adc2497afec5366e4f9e50
BTS simply notifies the PCU about the supported MCS, and PCU is
responsible for providing correct data formatting supported for the BTS
and the target MS.
Related: OS#4544
Change-Id: Ifcf23771bd23afc64ca6fea38948f98f2d134ecb
For instance if PCU received DL data to be sent to an MS from an SGSN,
and the MS is not currently cached in the PCU (because there's no TBF
active for it), it will page it and transmit the DL data to it.
The SGSN is capable of sending (EGPRS) MS Class information in that same
DL data message, so it's the one responsible for providing that
information if not available at the PCU.
In the PCU if we don't have information about that MS and SGSN didn't
provide us information about it, we cannot assume the MS is going to be
EGPRS capable and even less expecting a specific EGPRS MS class.
So let's drop this code.
Related: OS#4544
Change-Id: Icce66cadb51af25ae0c3b3719940eccb548fe33b
EGPRS "enabled" or "not enabled" is a bit of confusing idea, since there
are different levels of EGPRS support. For instance we may have been
instructed by config to not transmit using MCS5-9 (8PSK), or not use
MCS1-9 at all (GMSK+8PSK). However, we cannot control what is sent to us
or what we receive on lower layers, so if the BTS PHY/receiver was able
to decode+receive, let's try to process it anyway...
Related: OS#4544
Change-Id: Ie70ec8e4a2d688762d7d320d6ad58d5a0cc52ea1
This VTY command was added due to EGPRS being introduced later as an
experimental feature. It's no longer needed and causes more problems
than goodness (since people sometimes forgets to enable it).
Let's rather simply enable EGPRS support based on what BTS/BSC requests
over PCUIF.
Related: OS#4544
Change-Id: Ic80970a136361584da9c912252a07e7c3c9d85d0
Some tests were wrong (TypesTest) and required modification, since they
were setting a EGPRS MS but then expecting a GPRS assignment.
Change-Id: I9d3ee21c765054a36bd22352e48bde5ffca9225a
Take into account the MCS values supported by the BTS. In osmo-bts,
in general all MCS are enabled if "mode egprs" is selected in BSC,
and none otherwise.
Change-Id: Ie8f0215ba17da1e545e98bec9325c02f1e8efaea
This way everytime any program or test initiates a BTS object, the
bts_data structure has the same values.
Change-Id: Iffd6eecb1f08bda0091f45e2ef7c9c63b42e10b3
If SGSN provides us with MS class information upon DL data, let's use it
and set it in an already existing MS object if not yet known.
Also remove all unneeded code passing ms_class to append_data() which
would simply try to (again) set the ms_class.
Change-Id: I4979c9344bffd3ba7657bbab94981d233eab801f
Before this patch, it would always allocate all TBFs on the first TRX
until all TFIs were filled, then second, and so on. But it would
actually fail around 8th MS requesting an UL TBF because despite a TFI
was successfuly assigned, because all USFs were already exhausted for
that PDCH.
Related: OS#1775
Change-Id: Iccfc8acfbfdc258ed16cc5af01f12b376fe73b72
Avoid passing tons of params to internal helper function
tbf_nel_dl_assignment() in order to either fetch again the ms object or
create a new one. Let's instead create the ms function earlier if needed
and fill it with all the discovered information prior to calling the
helper function. This provides cleaner code and also better log output.
This way we also avoid trying to fill the MS twice and unneeded
getter+setter for TA.
tbf::imsi(): There' always an ms, so simply forward call to
ms()->imsi().
We can also get rid of assign_imsi, since the modified code is the only
place where it's used and there's already some code in place there to
update the MS. We instead merge it with set_imsi and keep the
duplication code to catch possible bugs from callers.
Move merge_and_clear_ms from tbf class to GprsMS, where it really
belongs.
Change-Id: Id18098bac3cff26fc4a8d2f419e21641a1f4c83b
This is another step forward towards a more clear data model where a TBF
always has a MS object (which may be lacking some information, and at a
later point when more information is found, it may actually be a
duplicated MS object and hence one duplicate removed and the TBF moved
to the object being kept).
This helps for instance in removing duplicated information stored in
the TBF which is really per MS, like ms_class, ta, etc. Since there's
always a MS object there's no need to keep a duplicate in both classes
in case there's no MS object.
It can already be seen looking at unit test logging that this kind of
data model already provides better information.
Some unit test parts were needed to adapt to the new model too.
Change-Id: I3cdf4d53e222777d5a2bf4c5aad3a7414105f14c
There's no real need for having different copies of this method in each
children. Furthermore, having the method implemented in the base class
made me shoot my foot while trying to move this to the tbf constructor
(see next commit), so let's simplify this and avoid other people
following into the same issue.
enable_egprs() in tbf.h is moved to be public since it needed (as it was
for the duplicated children mehtods with same name), but anyway it will
be moved to private in next commit.
Change-Id: Id7de060318201a42e51f277f898463f4b9a84eba
Return an interface to the window base class so that the tbf base class
can access the common window methods, such as set_ws(). It will be used
in next commit to get rid of duplicated function enable_egprs in both
dl_tbf and ul_tbf subclasses.
The user of the function can then decide to access more specific
functionaltiites of the window class by static casting it to the
specific direction (which is known by the caller since it operates on a
ul_tbf or a dl_tbf).
Change-Id: Ia2e1decf91be1184668e28297c2126affb9c7ae4
In those cases since a string pointer was passed, it always printed
"single" instead of whatever really was being used, since the string
pointer was not NULL.
Change-Id: Idab7d18e8f519e10fc3df4007634661c46f9256d
In general we want to see explicitly the kind of requested allocation at
the start. The MS class is not needed since it's printed in the previous
log line in any case.
Change-Id: I9eb0a592c15be96da9d140ff373c1afead76b18c
Add support of the second NSVC in the info indication.
Add support to update a previous NS configuration.
Allow to update of a NS-VC while the NSE is still available over the
second.
Depends-on: I917f25ebd1239eae5855d973ced15b93731e33a0 (libosmocore)
Depends-on: I3a0cd305fd73b3cb9ec70246ec15ac70b83e57f2 (libosmocore)
Depends-on: I5a2bb95d05d06d909347e2fb084a446ead888cb3 (libosmocore)
Depends-on: I54f110acc3acccb362f6e554324d08cc42b7c328 (libosmocore)
Depends-on: Ia00753a64b7622a0864341f51ea49b6963543755 (libosmocore)
Depends-on: Ic8f6f8aca10da23a18fab8870be7806065a34b47 (libosmocore)
Depends-on: I5f67e6a9bf4cb322bd169061fee0a528012ed54d (libosmocore)
Change-Id: I589ebaa2a2b7de55b7e4e975d8fd6412dd5f214b
It's expected to receive subsequent INFO.ind messages at run-time,
e.g. when a dynamic TCH/F_TCH/H_PDCH timeslot is switched from
PDCH to TCH/F or TCH/H, osmo-bts would send us INFO.ind with the
updated PDCH slot-mask indicating that this timeslot is disabled.
In gprs_nsvc_create_and_connect(), do not bind() on the received
NSVC address unconditionally - we may already be bound to it.
Instead, return early and keep everything unchanged.
I don't know how the PCU is supposed to handle NSVC address change,
at least the new NS2 library does not handle this internally, nor
it provides any API for that. Let's leave it for later.
Change-Id: I159138e41e147cd30212da548b0ccd3f81d61b4e
Related: I4c3bc883d795e5d1ee5ab175ac03684924692a7c
Fixes: Ib389925cf5c9f18951af6242c31ea70476218e9a
Related: SYS#5108
The code in gprs_nsvc_create_and_connect() stores NSEI there for
no reason, despite it's already stored in struct gprs_ns2_nse.
Change-Id: Ib30152a12384cf0448104a1ee1cfb949f4a27553
The commandline option --vty-ref-xml is needed to enable automatic
generation of the VTY reference manual.
Change-Id: Ie1829a06b83f69f4cd8256adbf9437388ca3d7e0
Related: SYS#4937, OS#1601
It seems that some gcc versions do not consider the priority of
"C" __attribute__((constructor)) definitions in the same order as
they do C++ static initializers, which are called in the order in which
they appear in the compile unit (source file).
The problem has been observed at least in a
environment based on T2 SDE with GCC 6.3.0 and binutils 2.28.
Let's work around this by making sure the __attribute__((constructor))
function always gets the highest priority value permitted by gcc (101).
Closes: SYS#5093
Change-Id: I65de69a32ac929e6ddd4e58980027f9e76813153
Fixes consistent crash under some specific scenarios explained in
OS#4756.
The crash was caused due to a bug in channel allocator algorithm
incorrectly populating tbf->pdch[] array as a result of mismatching
first_ts and resulting pdch selected slot bitmask.
The issue happens because when allocating a UL TBF in allocator B, the
subset is always further forced into allocating one single TS. As a
result, on that branch several variables are updated, but first_ts was
not.
The field used to be updated in older versions, but a bug was introduced
during code refactoring in commit listed below (31 Jan 2018).
Fixes: 0cc7212cfd
Related: OS#4756
Change-Id: I79596803f7dab6f21b58bfe39c2af65d9c5b39d5
Otherwise the TBFs are kept, and hence PCU will continue reserving resources and
DL data queued will still be sent over the air, despite the MS not
listening anymore on the PDCH.
Change-Id: I4ae1c3706b2ed6e4d271cd16f7cd7f8937b84836
Output of all diag in different lines is really confusing, since the
user reads a timeout ocurred and then later in another line something
like "Downlink ACK was received" while no GSMTAP message shows any ACK.
Change-Id: I6a7d79c16c930f0712bc73b308409ececb1946ba
Introduce a address_type in the NSVC configuration pass the given
protocol. The remote_ip is network byte order, the default
encoding for in_addr and in6_addr.
Change-Id: Ia0852f9f4395f1248b39363ef90f6f5673b24e2f
Related: SYS#4915
This reverts commit 38aaa10ed4.
It was to early because the frequency hopping wasn't ready to be merged.
Change-Id: Ibf055d5adfd9bffaaf51cb8468c79be597467e0f
Introduce a address_type in the NSVC configuration pass the given
protocol.
The remote_ip is network byte order, the default encoding for in_addr and in6_addr.
Change-Id: If26958d5b584973dca79159cf9e7f3f266519ce9
This is a left-over from an earlier version of [1] that makes
Coverity think that there can be NULL pointer dereference,
even despite we assert(pdch != NULL).
[1] I8adc0cdb1b05a87b4df5d4bc196f6d381283a06f
Change-Id: I3490c38e0c1186dfd2fae63526a05c694547cebb
Fixes: CID#214230
The following test cases verify coding of the hopping parameters:
+ (RR) Immediate Assignment
- TC_pcuif_fh_imm_ass_ul_egprs,
- TC_pcuif_fh_imm_ass_ul,
- TC_pcuif_fh_imm_ass_dl,
+ (RLC/MAC) Packet Uplink/Downlink Assignment:
- TC_pcuif_fh_pkt_ass_ul,
- TC_pcuif_fh_pkt_ass_dl,
all of them pass with this (and the upcoming) change applied.
Change-Id: I8adc0cdb1b05a87b4df5d4bc196f6d381283a06f
Related: SYS#4868, OS#4547
It's quite odd to see that in write_packet_downlink_assignment()
we initialize an 'RlcMacDownlink_t', so then the caller can use
the power of CSN.1 codec to generate the final sequence of bytes
to be transmitted, while in write_packet_uplink_assignment() we
already compose the final RLC/MAC message straight away using
the low-level bitvec API (like bitvec_write_field()).
I guess the reason is that at the time of writing this code, the
CSN.1 codec was not stable enough, so it was safer to generate
the message 'by hand'. This would also explain why we *decode*
the final RLC/MAC message in create_ul_ass() right after encoding.
Rewrite write_packet_uplink_assignment(), so now it initializes
a caller-provided 'RlcMacDownlink_t' structure. Given that it's
allocated on heap using talloc_zero(), do not initialize presence
indicators of fields that are not present in the message.
This would facilitate handling of frequency hopping parameters
in the upcoming changes, in particular we can now introduce a
function that would compose Frequency Parameters IE for both
write_packet_{downlink,uplink}_assignment().
Tested manually by running a GPRS-enabled network, as well as by
running test cases from ttcn3-pcu-test => no regressions observed.
Change-Id: I2850b91e0043cdca8ae7498a5fc727eeedd029b6
Related: SYS#4868, OS#4547
In order to be able to encode frequency hopping parameters, let's
pass a const pointer to 'gprs_rlcmac_pdch' (PDCH slot) directly,
instead of passing all related parameters separately.
Change-Id: I6bccad508f0fdccc4a763211008dd847a9111a8d
Related: SYS#4868, OS#4547
According to 3GPP TS 44.060, section 12.12 "Packet Timing Advance",
the 'TIMING_ADVANCE_VALUE' field is optional, and takes 6 bits
if present. This means that a value that fits in range 0..63
(inclusive) can be encoded (0b111111 == 63).
It's possible that tbf->ta() returns GSM48_TA_INVALID == 220,
so the bitvec API would encode only 6 LSBs of it:
220 & 0b111111 == 28
Let's ensure that the 'TIMING_ADVANCE_VALUE' is present iff
tbf->ta() returns a correct (0 <= x <= 63), and absent otherwise.
Change-Id: I342288ea4ef1e218e5744e9be6a8e528d4e697fa
Initialize the bit vector to use already allocated memory,
so we would not need to allocate additional 23 bytes and
copy them from the bit vector to a msgb.
Change-Id: I4190707d7fa5b1c4c3db745635f88d5afb9e21ca
Let's create the MS object early if doesn't exist and fill in the
information, so that we can operate on it in an early way (for instance,
logging macros), this way it's easier to trace the lifecycle of
subscribers.
Change-Id: I3ec7eb970310698dd228ae6ad65ec5ca833bab3f
In get_paging_mi(), before this, an encoded buffer of Mobile Identity bytes is
returned. Code paths following this repeatedly decode the Mobile Identity
bytes, e.g. for logging. Also, in get_paging_mi(), since the TMSI is read in
from a different encoding than a typical Mobile Identity IE, the TMSI was
manually encoded into a typical Mobile Identity IE. This is essentially a code
dup of osmo_mobile_identity_encode(). Stop this madness.
Instead, in get_paging_mi(), return a decoded struct osmo_mobile_identity. Code
paths after this use the struct osmo_mobile_identity directly without repeated
decoding.
At the point of finally needing an encoded Mobile Identity IE (in
Encoding::write_paging_request()), do a proper osmo_mobile_identity_encode().
Since this may return errors, add an rc check for the caller of
write_paging_request(), gprs_rlcmac_paging_request().
A side effect is stricter validation of the Mobile Identity passing through the
Paging code path. Before, invalid MI might have passed through unnoticed.
Change-Id: Iad845acb0096b75dc453105c9c16b2252879b4ca
Note: subsequent patch Iad845acb0096b75dc453105c9c16b2252879b4ca will change to
passing a struct osmo_mobile_identity in the Paging code path, instead of
passing the encoded IE data.
Change-Id: Ibb03b8e601160427944f434761ca59811d1fc12f
It's totally fine to receive Packet Measurement Report messages with no
SBA present, since the MS also sends measurements on PACCH while
transmitting data.
Related: OS#4719
Change-Id: I8f642d9cdeb342df7d5f2fa30516ea69554a6270
In [1] I restricted L1 SAPI of PH-RA.ind to PDTCH and PTCCH, and
this seems to have caused a regression reported in [2]:
DL1IF ERROR sysmo_l1_if.c:251 Rx PH-RA.ind for unknown L1 SAPI PRACH
I assumed that PH-RA.ind belonging to a Control Acknowledgement
message (in format of 4 Access Bursts) would have PDTCH SAPI,
while apparently it's actually arriving on PRACH.
[1] I482d60a46b9d253dfe0b16140eac9fea6420b30c
[2] https://osmocom.org/issues/1526#note-39
Change-Id: Ib0a6da37de7a1db4cad2b96293b31b9f32e7d9eb
Related: OS#1526
This is not something that should normally happen. If it happens,
then it's definitely a bug, and we should not tolerate it.
Change-Id: I6e46ba42650f0db2399649b536a1d2b3f0fcbf04
The TLLI is tried to be updated later anyway during tbf_alloc_ul(), but
this way it's clear that information is stored where it belongs as soon
as possible. The change already shows clearer log lines in TbfTest.err.
Change-Id: I20ce4eb94ecf85ce2835275d0056d9ecd1b558c3
It's perfectly fine receiving a Resource Request message under some
circumstances (as stated in the comment added in the commit).
To print issues only under non-expected circumstances, the function
rcv_resource_request need to be refactored:
* Destroying older UL_TBF is delayed because it is needed further
down.
* When the old UL_TBF is FINISHED, it's an acceptable time to receive a
Resource request, so we check if that's the case and don't print a
warning in that case.
Change-Id: I4b4367126d6a16055cd2f45afc4a6b9c15a7c980
If the information is not found in the message, 0 (unknown MS class)
will be returned. If the MS already had some previous information on the
MS class, let's not lose it by setting it back to 0.
Take the opportunity to drop related log lines which are no needed,
since set_(egprs_)ms_class() functions already log the value changes.
Change-Id: Icd52209fd4395d78dc770e7869d1b1fe45a18ca0
There's no real good explanation on why the DL TBF is dropped there,
since PKT RESOUCE REQUEST is used basically during UL TBF establishment.
Also, as decribed by TS 44.060 11.2.16 "Packet Resource Request":
"""
This message is sent on the PACCH by the mobile station to the network
to request a change in the uplink resources assigned.
"""
Change-Id: Iab4afb66f0d671f7ad54909d2685a1613e12ab4d
According to 3GPP TS 44.018 sec 10.5.2.40, Timing Advance value is 8 bit
and range is 0-63 (0-219 on GSM400). Unsigned value (uint8_t) is used
everywhere else, so avoid using a signed one here, and simply check for
GSM48_TA_INVALID here, which we use everywhere else to initialize when the
value is not known. Ideally we should check for value based on band, but
it makes more sense to check that when receiving the data and storing in
in set_ta().
Change-Id: I82b13561d0fe5ebafb5c3a8b9a501045c29809bc
According to 3GPP TS 44.018 sec 10.5.2.40, Timing Advance value is 8 bit
and range is 0-63 (0-219 on GSM400). So there's no need for 16 bits to
store it. uint8_t is used in all other places in the code.
Change-Id: I38aa063ae30ca5680fef6252d2cef22cea98c123
For instance, that may happen because the len of the message is not
filling the expect size (because padding is missing for example). Still,
in this case we know the channel type, so we set it so that wireshark
tries to decode the message as a data one.
Change-Id: Ifea94095d669b528874e64ca823a776cd6e22b4b
Since commit 322456ed47 (and previous
one), it is expected that a tbf object ALWAYS has a MS object referend
to it, even if it's a temporary copy which will later be merged when
TLLI/IMSI is retrieved and it is found that several MS objects relate to
the same MS.
The purpose of set_tlli_from_ul was mainly to update TBF's ms() to
old_ms before going through usual tbf->update_ms() path. That's not
really needed since ms() is already always set and TBFs for old_ms are
already freed in update_ms() and children function.
Change-Id: Ie8795e7a02032336e53febb65c11f9150c36d2a0
Since not all the the information about the MS is known during TBF
creation in all scenrios, it may happen that when TBF is created it
creates a MS which later will end up being found a duplicate of an
already previously existing MS.
At that point, the old object is dropped and information retrieved from
both is merged into the new one.
The GPRS MS class was being transferred, but the EGPRS MS class was missing.
Change-Id: Ieb9929b60254b12f79392d6acb8b456d71cccb9e
According to 3GPP TS 44.004, section 7.4a, two alternative RACH
block formats are specified: 8 bit (1 octet) and 11 bit. The
bit order is LSB (right to left), byte order is MSB.
In PCUIF RACH.ind structure (see gsm_pcu_if_rach_ind) we use
a field of type uint16_t to store RA values regardles of the
block format. Thus when packing it to bytes, we cannot just
cast uint16_t* to uint8_t*, we need to do some bit shifting.
Change-Id: I08a0a908f855b0d8a002df732e02781126d27dfb
Let's finally use the API we introduced in [1].
[1] I96df3352856933c9140177b2801a2c71f4134183
Change-Id: Ia15761c33c8048d35c7f7bc93dbea781dd0894b7
Related: OS#1548
This patch is a set of tightly related changes:
- group all RACH.ind parameters into struct 'rach_ind_params';
- group Channel Request parameters into struct 'chan_req_params';
- get rid of egprs_mslot_class_from_ra(), priority_from_ra(),
and is_single_block(), introduce unified parse_rach_ind();
- improve logging, get rid of redundant information.
This is needed for proper EGPRS Packet Channel Request handling.
Change-Id: I5fe7e0f51bf5c9eac073935cc4f4edd667c67c6e
Related: OS#1548
According to 3GPP TS 44.004, section 7.4a, two alternative RACH block
formats are specified: 8 bit (1 octet) and 11 bit. This change adds
CSN.1 definitions for 11 bit EGPRS Packet Channel Request as per
3GPP TS 44.060, table 11.2.5a.2.
Change-Id: I96df3352856933c9140177b2801a2c71f4134183
Related: OS#1548
I faced a problem while working on EGPRS Packet Channel Request
coding support: the unit test I wrote for it was passing when
compiled with AddressSanitizer, but failing when compiled
without it o_O. Somehow this was observed only with GCC 10.
Here is a part the standard output diff for that unit test:
*** testEGPRSPktChReq ***
decode_egprs_pkt_ch_req(0x2b5) returns 0
- ==> One Phase Access
+ ==> unknown 0xdd5f4e00
decode_egprs_pkt_ch_req(0x14a) returns 0
- ==> One Phase Access
+ ==> unknown 0xdd5f4e00
decode_egprs_pkt_ch_req(0x428) returns 0
- ==> Short Access
+ ==> unknown 0xdd5f4e01
At the same time, debug output of the CSN.1 decoder looked fine.
So WYSINWYG (What You See Is *NOT* What You Get)! As it turned
out, this was happening because I used an enumerated type to
represent the sub-type of EGPRS Packet Channel Request.
typedef struct
{
EGPRS_PacketChannelRequestType_t Type; // <-- enum
EGPRS_PacketChannelRequestContent_t Content;
} EGPRS_PacketChannelRequest_t;
The problem is that length of an enumerated field, more precisely
the amount of bytes it takes in the memory, is compiler/machine
dependent. While the CSN.1 decoder assumes that the field holding
sequential number of the chosen element is one octet long, so its
address is getting casted to (guint8 *) and the value is written
to the first MSB.
// csnStreamDecoder(), case CSN_CHOICE:
pui8 = pui8DATA(data, pDescr->offset);
*pui8 = i; // [ --> xx .. .. .. ]
Let's make sure that none of the existing RLC/MAC definitions is
using enumerated types, and add a warning comment to CSN_CHOICE.
Affected CSN.1 definitions (unit test output adjusted):
- Additional_access_technologies_struct_t,
- Channel_Request_Description_t.
Change-Id: I917a40647480c6f6f3b0e68674ce9894379a9e7f
The current implementation is not capable of handling more than
256 (UCHAR_MAX) selectors in the choice list. Let's document
this and add a guard check to the M_CHOICE handler.
Change-Id: I40c3c5b9be892804c6cd71cbb907af469ce5d769
This is not a functional change, just fixing misleading function
name. Access Bursts on PTCCH/U have nothing to do with PDTCH.
Change-Id: I4ab710ba026315301cc6970263967616401a9fc8
We have same kind of object splitted into two layers, in coding_scheme
and gprs_coding_scheme. Let's merge them together and get rid of the
class, which is not really useful because it's only a set of functions
operating on one enum value.
This change also fixes gcc 10.1.0 error about memseting a complex type
in rlc.h init().
Change-Id: Ie9ce2144ba9e8dbba9704d4e0000a2929e3e41df
In normal conditions ACKing of UL blocks is only sent every
SEND_ACK_AFTER_FRAMES (20) frames. Which means if CV=0 is received (and
hence no more packets are received) less than 20 frames before a lost,
the PCU won't ask for a retransmission and wait there until some timer
destroys the TBF.
This issue is shown by TTCN3 test PCU_Tests.ttcn
TC_ul_intermediate_retrans.
Unit tests triggering this condition are adapted. Some similar tests are
not triggering it because BSN/CV relation being used is totally wrong
(like CV=0 being sent on a BSN with previous value than others).
Change-Id: I9b4ef7b7277efa645bdb5becf2e9f6b32c99a9b1
Newer gcc 10.1.0 is erroring due to memset being applied on a complex
type, so let's start by removing this only function outside of the
struct.
Change-Id: I20426557d9b3049ab275fadb92e10ea8a860a119
It's super annoying seeing lots of functions being called everywhere
only to find out they are only incrementing a counter. Let's drop all
those functions and increment the counter so people looking at code
doesn't see dozens of code paths evyerwhere.
Most of the commit was generated by following sh snippet:
"""
#!/bin/bash
define_pattern="^CREATE_COUNT_ADD_INLINE"
generic_func="do_rate_ctr_add"
grep -r -l "${define_pattern}" . | xargs cat | grep "${define_pattern}("| tr -d ",;" | tr "()" " " | awk '{ print $2 " " $3 }' >/tmp/hello
while read -r func_name ctr_name
do
#echo "$func_name -> $ctr_name";
files="$(grep -r -l "${func_name}(" .)"
for f in $files; do
echo "$f: $func_name -> $ctr_name";
sed -i "s#${func_name}(#${generic_func}(${ctr_name}, #g" $f
done;
done < /tmp/hello
grep -r -l "void ${generic_func}" | xargs sed -i "/void ${generic_func}(CTR/d"
grep -r -l "$define_pattern" | xargs sed -i "/$define_pattern/d"
"""
Change-Id: I966221d6f9fb9bb4f6068bf45ca2978008a0efed
It's super annoying seeing lots of functions being called everywhere
only to find out they are only incrementing a counter. Let's drop all
those functions and increment the counter so people looking at code
doesn't see dozens of code paths evyerwhere.
Most of the commit was generated by following sh snippet:
"""
#!/bin/bash
grep -r -l ^CREATE_COUNT_INLINE . | xargs cat | grep "^CREATE_COUNT_INLINE("| tr -d ",;" | tr "()" " " | awk '{ print $2 " " $3 }' >/tmp/hello
while read -r func_name ctr_name
do
#echo "$func_name -> $ctr_name"
files="$(grep -r -l "${func_name}()" .)"
for f in $files; do
echo "$f: $func_name -> $ctr_name";
sed -i "s#${func_name}()#do_rate_ctr_inc(${ctr_name})#g" $f
done;
done < /tmp/hello
grep -r -l "void do_rate_ctr_inc" | xargs sed -i "/void do_rate_ctr_inc(CTR/d"
grep -r -l "CREATE_COUNT_INLINE" | xargs sed -i "/^CREATE_COUNT_INLINE/d"
"""
Change-Id: I360e322a30edf639aefb3c0f0e4354d98c9035a3
The function is simply setting the ta on the ms, so simply make sure ta
is set on callers before passing the ms object.
Change-Id: Iebb9c57f458690e045ddc45c800209ad8cf621e0
Variable found is used to always call Guard() on MS to avoid possible
unexpected freeing regressions.
Change-Id: I62f24fe04ca10fca19bedda288fe3ed3ce75413f
The default loglevels of some log categories are configured to
LOGL_INFO. This is still to verbose, lets use LOGL_NOTICE here.
Change-Id: Ibb1cd1a94fb4fdd0147e073f8c1c82562c2c14ef
Related: OS#2577
It's really non-sense from architectural point of view to pass an
optional pointer to the MS holding the TBF and creating it otherwise.
TBFs shouldn't be creating MS they belong too.
This simple change requiring so many code line changes really exhibits
how badly entangled the object relationship is.
Another commit will follow doing the same for dl tbf.
Change-Id: I010aa5877902816ae246e09ad5ad87946f96855c
New define is available since libosmocore 1.1.0, and we already require
1.3.0, so no need to update dependenices.
Let's change it to avoid people re-using old BSC_FD_* symbols when
copy-pasting somewhere else.
Change-Id: Ida8fd3bd7347163567acde34ad67aefee913b0ea
MultislotClass is 5 bit long, so an uint8_t is enough.
In most places we are already storing multislot class as uint8_t.
Change-Id: I1dcaff9d69379453a0b794e5f36b820f5f78531f
In osmo-pcu datatructures, the variables holding multislot classes
simply contain an integer referring to the multislot class number,
instead of coding from 3GPP TS 44.060 Table 11.2.5.3 and Table
11.2.5a.3.
So coding Multislot class 3 is stored as 0x03 in osmo-pcu variables,
while in 3GPP TS 44.060 coding it's coded as 0x02 (N-1).
This allows us using value 0x00 to designate a "yet unknown (EGPRS) Multislot
class".
Hence, we need to add 1 to the decoded value to match our data
structures.
Change-Id: Id3b121272bb7e84c0542ae9b4ce09598c6054edd
This function is actually returning an EGPRS multislot class, so let's
update naming. The variable using the return value was already being
passed as egprs_ms_class to tbf_alloc_ul_tbf().
Change-Id: Idb51836c8c9dd4e865bf2cb0b0c24155662f2ae8
Even if we don't accept it, let's submit GSMTAP with correct channel.
We don't return error like in code below, because otherwise the generic
UNKNOWN gsmtap message will be sent.
Change-Id: I853679ce8907d46fcb84ae4127335c10623f09c9
It's actually counter-productive when analyzing wireshark traces, since
one may not spot a decoding issue and assume PACCH is sent on the wrong
TS.
Change-Id: I7a96148f1ca1ebfa88a3ff714ea3bb8798866046
It was removed in wireshark.git e8407dd6c1378427daee77e8de540d0b5f7a0b73
and it's not there anymore in current master.
Change-Id: I73f4eeca3fd4f00a5bc4f06ef7a9bb9b8a70e37b
Port from wireshark.git 428ee66ae1c524b49f9043729b1f1e9b4f52f409, from
Pascal Quantin.
The original commit is also changing the RRC_Container field to
M_CALLBACK, but we leave them as M_VAR_ARRAY since the callback is
basically used to add more dissection information in wireshark.
Change-Id: I0f374e78300efddff00c4df26a401adcdee18a12
Port of patch (+ later fixes squashed) of wireshark.git commit
dea5452b95dfaf18e38670a8e2b3b38f9175fdfd, from Lei Chen:
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=6856
Squashed wireshark.git fix commits:
774be29de0b4d93d01aecb1518c41d7d551071a9
51c31cd7bd3d8fc196a9f90a8af466ad84e9e6a8
6aca10831f86c562970b13efa811f46e25ee3091
c1ceac58cdb77051e9bd14c1f6f7669cf5779a86
Change-Id: I08523bc1bbdffde479ef974b4c7b56cfa5639591
Original file from wireshark.git (packet-gsm_csn1.c) is being built and
maintained as a C file. There's no real need for us to maintain it as a
C++, and doing so will make both files derive over time (as already
happened). Let's keep it as a C compiler (which btw seems to be more
strict) to make it easier to port patches back and forth wireshark.git.
Take the chance to move some declarations we added to csn1.h to be able
to build it out of wireshark. Let's keep those in a separate header file
to ease looking for differences.
Change-Id: I818a8ae947f002d35142f9f5473454cfd80e1830
When switching to C compiler, it will warn/error. Use #if 0 as in the
original wireshark.git epan/dissectors/packet-gsm_rlcmac.c code.
Change-Id: If1be50947c02208f15892d99edeb394fb4f52b75
Header files included from libosmocore may potentially contain
some language constructions allowed in C but not in C++, such
as type casting. Let's add 'extern "C" { ... }' and be safe.
Change-Id: I7197f7b34f30b49d5397506ce9d67cbf0e2cc196
Including the <new> header is required as explained by the c++ specs [1]
osmo-pcu/src/tbf.cpp: In function ‘gprs_rlcmac_ul_tbf* tbf_alloc_ul_tbf(gprs_rlcmac_bts*, GprsMs*, int8_t, uint8_t, uint8_t, bool)’:
osmo-pcu/src/tbf.cpp:1002:39: error: no matching function for call to ‘operator new(sizetype, gprs_rlcmac_ul_tbf*&)’
1002 | new (tbf) gprs_rlcmac_ul_tbf(bts->bts);
| ^
Most of the times this issue is not detected because other STL headers
are already including <new>.
[1] http://www.cplusplus.com/reference/new/operator%20new/
Change-Id: Ie5fb536ae29dcf40e2a0dbe67432bebd61b8c7aa
This problem problem was discovered by the Undefined Behavior Sanitizer:
pdch.cpp:210:4: runtime error: load of misaligned address
0x60c00002abf2 for type 'uint32_t',
which requires 4 byte alignment
Do not convert TMSI to number, use osmo_mi_name() from libosmocore.
Also use this function to print other MI types (IMSI or IMEISV).
Change-Id: Icf8836f216793e342b239c8e6645aac1e82bf324
The TMSI/P-TMSI IE in BSSGP PAGING-PS/CS comes without the MI type
header, that must be present in RR Paging Request. Prepend it.
TTCN-3 test case: I7fbec5b2c5c3943a7413417b623f55c135c152d7
Change-Id: I97fd5ffc15a4a58112d7c37c69b7ac42b0741a0e
This patch corrects an error introduced in
6fd8ffb6fe
That commit allowed us to send the data over GSMTAP even
if the Uplink Control Block had invalid content,
that is to say, if decode_gsm_rlcmac_uplink() returned error.
However the check for ul_control_block->u.MESSAGE_TYPE
was place before decode_gsm_rlcmac_uplink()
Change-Id: Ic47602e5c6a13571b92c0a939fc3514110b82444
There's two variants for the Ms Radio Access Capabilities.
* The usual encoding with spare bits (usually to fill up to octet boundary)
as defined in TS 24.008 Table 10.5.146
And there's too:
* MS Radio Access Capabilities 2 IE from TS44.060 section 12.30, which is
the same but removing all spare bits, and which is used in messages like
Packet Resource Request and Additional MS RAC messages.
The later is used basically for messages having extra IEs after the MS
Radio Access capabilities IE, since they are encoded immediatelly
afterwards.
So this patch does:
* Adds the expected spare bits (M_PADDING) to MS_Radio_Access_capability_t
* Creates a new MS_Radio_Access_capability2_t without padding
* Updates code to use the new "2" version where needed.
Note RLCMACTest long de/encoding line logs change only because the name
of the struct changes (the "2" is added).
Change-Id: Ibd756f80a03452a651e2771dbc628d701e55ac4b
It seems the assumptions regarding maximum number of RA capabilitites
in one message were wrong. Doing some rough calculations, each RA
capabilitiy value (without extensions) can take around 20ish bits, which
means for a message containing up to 52 bytes that quite a lot of
different values could be theoretically fed in. Let's be safe and
increase the array size to be able to handle all different access
technologies listed in See TS 24.008 table 10.5.146 following
restrictions:
* "The MS Radio Access capability is a type 4 information element, with a maximum length of 52 octets."
* "Among the three Access Type Technologies GSM 900-P, GSM 900-E and GSM 900-R only one shall be present."
* "the mobile station should provide the relevant radio access
capability for either GSM 1800 band OR GSM 1900 band, not both".
Wireshark requires similar fix (it's not important though because it
currently uses another ad-hoc decoder for RAcap).
Related: OS#4463
Change-Id: I5334eaacfbc238fae8bea50c9e9667c2117f81ff
This way if CSN1 encoded bitstream contains more elements than what the
defintion expects it will fail instead of overflowing the decoded
buffer.
RA cap struct placed in unit test is taken from a real android phone
sending the value when attaching to the network. Then SGSN sends it back
and osmo-pcu would crash similar to unit test:
*** stack smashing detected ***: terminated
Process terminating with default action of signal 6 (SIGABRT): dumping core
at 0x4C62CE5: raise (in /usr/lib/libc-2.31.so)
by 0x4C4C856: abort (in /usr/lib/libc-2.31.so)
by 0x4CA62AF: __libc_message (in /usr/lib/libc-2.31.so)
by 0x4D36069: __fortify_fail (in /usr/lib/libc-2.31.so)
by 0x4D36033: __stack_chk_fail (in /usr/lib/libc-2.31.so)
by 0x124706: testRAcap2(void*) (RLCMACTest.cpp:468)
Related: OS#4463
Change-Id: I9fe0e55e0a6a41ae2cc885fba490c1d4a186231e
This way the macros can be used to access the arrays themselves and
calculate its static size to enable validation later.
In the case of Packet_Access_Reject_t, modify the description to use a
M_REC_TARRAY_1 object to get rid of access to 2nd element. The new
description is the correct one, since the first element is mandatory
according to TS 44.060 Table 11.2.1.
Change-Id: I6f10350d4734360c7a15a702c72b59efd84987ee
low-level text decodes of CSN.1 messages certainly are not NOTICEable
events, but rather something used for debugging.
Right now we get various text CSN.1 log output of osmo-pcu in it's
default configuration. Despite all log levels being relatively high
(NOTICE), we still see those messages as they simply are logged
at the wrong level.
Related: OS#2577
Change-Id: I7b42c9e21ad8d8a5b54e7a3b68490934ce3d3198
This commit is basically a revert of
f4bb42459c, which disabled the code. That
commit claimed the SGSN may have providen inacurate or wrong data at the
time, but then it should be fixed in the SGSN.
Related: OS#1525, OS#3499
Change-Id: Ie36ae23203110018d4b5ae47591e0a64989e23a0
We should really be using monotonic clock in all places that
gettimeofday is used right now. Since clock_gettime() uses timespec,
let's move all code to use timespecs instead to avoid having to convert
in several places between timespec and timeval.
Actually use osmo_clock_gettime() shim everywhere to be able to control
the time everywhere from unit tests.
Change-Id: Ie265d70f8ffa7dbf7efbef6030505d9fcb5dc338
Both csnStreamDecoder() and csnStreamEncoder() shall not return 0
prematurely if no more bits left in the input / output bit-vector.
Returning CSN_ERROR_NEED_MORE_BITS_TO_UNPACK might make more sense,
however we don't know in advance (i.e. without entering the loop)
whether it's an error or not. Some CSN.1 definitions have names
like 'M_*_OR_NULL', what basically means that they're optional
and can be ignored or omitted.
Most of the case statements do check whether the number of remaining
bits is enough to unpack / pack a value, so let's leave it up to
the current CSN_* handler (pointed by pDescr) if no bits left.
Return CSN_ERROR_NEED_MORE_BITS_TO_UNPACK only if the number of
remaining bits is negative as this is an error in any case.
Change-Id: Ie3a15e210624599e39b1e70c8d34efc10c552f6c
Port from wireshark.git de028e81c53f9c45ccc5adb3bffd2f16ae2017bf
This commit breaks transcoding of the test vectors containing
the MS RA Capability IE due to the reasons explained in [1].
The more fields we add, the longer gets the output of the CSN.1
encoder. This is not critical, since we never need to encode
messages containing the MS RA Capability IE on practice.
[1] Ibb4cbd3f5865415fd547e95fc24ff31df1aed4c0
Ported-by: Pau Espin Pedrol <pespin@sysmocom.de>
Change-Id: Ibb4cbd3f5865415fd547e95fc24ff31df1aed4c0
This change fixes a bug that was reported by Keith Whyte and
confirmed in [1]. The problem is that a user-defined handler
in case of CSN_SERIALIZE may parse only a part of the given
bit-stream, leaving some bits unhandled. This is expected
because the sender (i.e. the MS) may use more recent RLC/MAC
message definitions containing new fields at the end.
Those bits that were left unhandled by serialize() shall not be
interpreted as continuation of the message, they shall be skipped.
Note that the encoded vector in the RLCMAC unit test still does
not match the original one. That's a known bug explained in [2].
[1] If5873355d52d7ddb06c2716154a88d34100f6ab5
[2] Ic46d6e56768f516203d27d8e7a5adb77afdf32b7
Change-Id: Id4cc042fed68fc54aca0355dcb986cab3f6b49ea
Related: OS#4338
Move the call to send_gsmtap() before the call to decode_gsm_rlcmac_uplink() as if
the latter returns error we return and never get to see the packet on the GSMTAP.
Change-Id: Ia6af9f40590f28fcae3fef50d9c601d8435412cd
The main idea of this change is to demonstrate a weakness of the
CSN.1 codec that most likely causes a unit test breakage in [1].
The problem seems to be that the transitional structures, where
the CSN.1 decoder stores the results, do not contain any details
about presence of the optional fields (such as M_UINT_OR_NULL).
In other words, it's impossible to know whether some optional
field is omitted in the encoded message (NULL), or is it just
set to 0. This means that the encoder will always include all
optional fields, even if they're not present in the original
message.
[1] Ibb4cbd3f5865415fd547e95fc24ff31df1aed4c0
Change-Id: Ic46d6e56768f516203d27d8e7a5adb77afdf32b7
As seen in OS#4420, setting the MetaInfo.recv_time outside of
llc_queue before calling llc_queue::enqueue() and later on using that
value in llc_queue itself at dequeue time is not a good idea, since it
can provoke errors if the recv_time was not set correctly.
For instance, LlcTest was not setting the value for recv_time on some
test, which ended up with a huge millisec value when substracting now()
from it:
"""
llc.cpp:215:29: runtime error: signed integer overflow: 1582738663 * 1000 cannot be represented in type 'long int'
"""
This issue only appeared when started building on a raspberrypi4.
Let's better set/store the MetaInfo.recv_time internally during
llc_queue::enqueue(). Then, enqueue() only needs the
MetaInfo.expire_time, so let's change its arg list to only receive that
to avoid confusions.
Take the chance to move the llc_queue APIs to use osmo_gettimeofday,
since we need to fake the time now that the API itself sets that time.
Also take the chance during this refactor to disallow passing null
pointer by default since no user needs that.
Finally, update the LlcTest accordingly with all API/behavior changes.
Related: OS#4420
Change-Id: Ief6b1464dc779ff22adc2b02da7a006cd772ebce
Found while doing differential analysis (comparison against the
original implementation from Wireshark).
Change-Id: Ibd0b7400d78f7873c2a8d45267332f511b5c6fbb
Found while doing differential analysis (comparison against the
original implementation from Wireshark).
Change-Id: I9f7fa9c3f2f4ff5213dded930cee7ec509b9d799
Found while doing differential analysis (comparison against the
original implementation from Wireshark).
Change-Id: Id2a4f03035cd8354d3fba0ad37571453d3986d21
The implementation of CSN.1 codec was taken from Wireshark, where
it's implemented in pure C. For some reason it was mixed with C++
specific features, mostly using references in parameter
declaration. Not sure what are the benefits.
Change-Id: I56d8b7fbd2f9f4e0bdd6b09d0366fe7eb7aa327a
This would allow us to catch more bugs. Note that I had to remove
printing of pointer address to make the output deterministic.
Change-Id: I1a77441eb957353c919bc73f8e3a2e38f4a383a9
As was discovered recently (see OS#4388), bitvec_read_field()
would never return a negative value because its return type
is unsigned (uint64_t).
We don't really need to get more than one bit, so let's just
use the bitvec_get_bit_pos() instead.
Change-Id: I763a295cd955cd33f542292c85d97ff82f6b49bc
Related: OS#4388
After the recent changes [1], it was noticed that one of the unit
tests fails. In particular, a decode-encode cycle of Packet
Polling Request produces a different vector:
vector1 = 49 13 e0 08 50 88 40 13 a8 04 8b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b
vector2 = 49 13 01 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b
vector1 == vector2 : FALSE
As it turns out, the original (input) vector itself is malformed
because it contails no valid identity, and thus violates the
specs. The CSN.1 decoder from Pycrate [2] throws an exception
while trying to decode it. I believe we should do the same.
Let's stop decoding the bit stream and return an error in case
if neither of a given list of the choice items matched.
[1] Ia0f8cc224a4c38e80699f834fd83d4c0d99322ea
[2] https://github.com/P1sec/pycrate
Change-Id: I420144773ed5e80372534e0f18db5e74cdb2999d
Fixes: OS#4392
The problem is that bitvec_free() is not NULL-safe. Ideally we
need to fix it in libosmocore [1], but let's also fix it here,
so OsmoPCU can be safely used with older libosmocore versions.
[1] https://gerrit.osmocom.org/c/libosmocore/+/17114
Change-Id: I7647d17b3d03f8e193ef6e793a2d3c1967744eef
Fixes: CID#208181, CID#208179
Output was incorrect before this patch. LOPC was being called without
having any initial LOGP, and trailing newline was usually missing at the
end.
Since csnDecoder/encoder functions are recursive, it's difficult to
handle logging state in a coherent way inside them. Let's better simply
control start/end of logging related topics in the callers of those
functions, and simply use LOGPC everywhere in csn1.cpp.
Change-Id: I50da7560939fac360b7545e2a6bfaf45ed0c4832
It was noticed that OsmoPCU leaks memory when trying to reconnect
to the BTS. It could be easily fixed, but we don't really need to
allocate the PCU socket state on heap as we never have more than
one connection.
Change-Id: Iea8930f443caa16f522f7c5375e0004e4e2315cb
Since I2b32b4fe20732728db6e9cdac7e484d96ab86dc5, go_parent_cb()
is completely optional. It no longer has the task to determine
the correct parent node. The is_config_node() callback is no
longer needed too. Get rid of them.
Since Ic5e69a396df659933fd4d50298b9925e837a6861 we depend on 1.3.0.
Change-Id: Id7ce8c4e1ac43747ad40a06d01433c366da07b42
As was discovered by pespin, changing logging level of DCSN1 makes
the CSN.1 decoder behave differently (see OS#4375). In particular,
this makes RLCMACTest (encode / decode test) fail.
I did a quick investigation and noticed that some of the logging
statements call bitvec_read_field(). By definition this function
moves the internal pointer (current bit position) of a given
vector and increments readIndex by a given amount of bits.
The problem is that LOGPC would not evaluate its format string if
the logging message is not going to be printed, e.g. if a given
logging level is lower than the current one, or in case if
logging is not enabled at all.
The first two conditional calls to bitvec_read_field() are related
to CSN_PADDING_BITS, so that's not critical because padding is
always in the end of messages. The later two are related to
CSN_RECURSIVE_ARRAY and CSN_RECURSIVE_TARRAY respectively.
Let's use bitvec_get_uint() instead to keep readIndex unchanged.
Change-Id: Ia331048db9f790ca407fd341ced01df12d10a233
Fixes: OS#4375
Same API is kept to more easily keep code compatibility with wireshark's
packet-csn1.c implementation.
Change-Id: I1ce2c52e2357841aa1f31babfdce9011435f866b
The 'gprs_llc' is defined as a pure C structure with C++ specific
extensions (methods), so it's rather a class. Accessing its field
'frame' statically causes Clang to throw a compilation error:
gprs_bssgp_pcu.cpp:111:29: error: invalid use of non-static data member 'frame'
if (len > sizeof(gprs_llc::frame))
Let's avoid this and use LLC_MAX_LEN as the size limitation.
God knows what to expect from such a mix of C++ and C...
Change-Id: I7f84bd776cc780a45880f136107f6e0bc56241d1
This is rather a cosmetic change aimed to make ASAN / Coverity happy.
In general, we never pass any input from an untrusted source.
Change-Id: I26d654da4c3bf5fd86a298c3027fd9820c932308
It does not make sense since INT_MAX is always less than LONG_MAX.
Found by Clang [-Wtautological-constant-out-of-range-compare].
Change-Id: I9934e05aa050bf93b3c795376f5dca3a848a7e11
(as they are part of the RlcMacUplink_t structure that is also used to call csnStreamDissector function).
Port from wireshark.git commit 9f8b638cfa8a660fb64c54dcadb83e6747db0a15.
Ported-by: Pau Espin Pedrol <pespin@sysmocom.de>
Change-Id: If46f8cc3f21f527f911dcac6ff1b78f182104a00
Port of wireshark.git 8626bb4cbb4d9926f7b56663585d9ef66252f93f.
We don't really need the other fields added there, let's keep only the
value out of the union.
Change-Id: Ia8889252ee7518a919a15d749815c2803b4b23cd
Port of wireshark.git commit 6aca10831f86c562970b13efa811f46e25ee3091.
From Mike Morrin:
Fix pedantic compiler warnings in csn.1 dissectors.
There is some tricky casting going on in csn.1 structures. To eliminate all
the warnings, the function pointers needed to be moved out of the object
pointer unions. Fortunately macros (mostly) hide these changes from the
protocol dissector tables.
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7686
svn path=/trunk/; revision=44899
Change-Id: Ia1a8c50c4b024ca6df4e3fbbf891cd33591ccc9b
This is a port of wireshark.git commit
2f024256bf337400ef3a82fa75e6d48d5707e059.
From 78516187d821b8d19d16987b1d6bc855ee7cbe10 Mon Sep 17 00:00:00 2001
From: Sylvain Munaut <tnt@246tNt.com>
Date: Sat, 4 Feb 2012 10:00:22 +0100
Subject: [PATCH 4/6] packet-csn1: Allow CHOICE elements to re-process the bits used for the choice
We may want to display more detail, or the sub-element should be
displayed with its headers or whatever ...
Change-Id: I3a5a95d5f918b8f17a2400a6d0c4d855ecacea7e
Port of wireshark.git 2f024256bf337400ef3a82fa75e6d48d5707e059.
From c6ee558d3bb00bfd25cca7c534448bf60df3c7cf Mon Sep 17 00:00:00 2001
From: Sylvain Munaut <tnt@246tNt.com>
Date: Sat, 4 Feb 2012 10:24:01 +0100
Subject: [PATCH 6/6] packet-csn: Extend CSN_SERIALIZE to allow 0 bit of length
In some coding there is no 'length' field at the top of a serialized
block, or it's more complex than a single field, in which case we
have to rely on the serialize decoder to consume the correct number
of bits.
We extend the CSN_SERIALIZE processing so that if a '0 bit' length
field is specified, then the length is not displayed and the
consumed bits by the serialize function is taken as the length
at posteriori.
The processing keeps the same behavior for any length > 0.
Change-Id: I9fadc99218594447001f7bb9943f4514b9877799
So that they always occur next to an increment of bit_offset.
Port from wireshark.git 1c81971d4292438ffdf83e9f9b9ab96c133c785b.
Ported-by: Pau Espin Pedrol <pespin@sysmocom.de>
Change-Id: I7474e9d632e068d6e33b0a502b81d4fff1f48802
Port from iwireshark.git commit cc6d4341e65ef2e8d8488fe0ac0f236ece0dd844.
It looks like it makes no difference to us now, but other EGPRS messages
may use it in the future.
Ported-by: Pau Espin Pedrol <pespin@sysmocom.de>
Change-Id: I34039370c292e62790a38abb59f55c69fffa88e8
Currently code using that function in osmo-pcu is disabled, allegadly
because SGSN was sending incorrect values, but it looks more like a CSN1
issue.
Related: OS#1525, OS#3499
Change-Id: I92c86397f988afaa791871d823a45fa85054f3bb
P-TMSI is optional IE, but IE is mandatory and hence always available.
Since the encoding is actually a Mobile Identity, the IMSI is used in
case P-TMSI is not available.
Change-Id: I4dbf8db04e81f98352a42ce34a5d91326be9bfd1
It's not really needed to have those together in some function calls,
and makes it more difficult to follow the code. Furthermore, new callers
not having content already aligned (len+value) will be using these
functions in forthcoming commits.
Change-Id: Ifb9d3997bfb74b35366c3d1bc51ce458f19abf16
Others projects don't contain a dash in there, and it seems to cause
problems with TTCN3 VTY expectations.
Change-Id: I3430abb5fc622dec293457466e760de95fa3a05c
Some are used to control (M)CS values for downlink while some do it for
uplink. Let's make clear which one is used for what. Take the chance to
document the fields a bit better than they were.
Some more information about the origin of cs_downgrade_threshold can be
found in the commit introducing it: 70b96aa232.
Related: OS#4286
Change-Id: I4e890e924b094a1937fbd3794de96704cf0421a8
So far there was a memory leak, because free()ing 'the_pcu.bctx'
would cause ASAN to complain. And that's reasonable, because it
needs to be freed properly. Moreover, 'the_pcu.bctx' may simply
be uninitialized in some cases, e.g. when OsmoPCU is terminated
before connecting to the SGSN.
Let's use the new bssgp_bvc_ctx_free() from libosmogb.
Change-Id: I274e79e1746c7678b81720ec11e8a564befe38ba
Depends: Ia78979379dbdccd6e4628c16f00d0c06d9212172
Both BSSGP SUSPEND ACK and NACK messages use BVCI=0 (signaling),
which always exists. Claiming that BVCI=0 is unknown is wrong.
Instead of adding both BSSGP_PDUT_SUSPEND_{ACK,NACK} to the 'if'
statement, let's rather avoid rejection for all BVCI=0 messages,
as there may be other unlisted message types.
Change-Id: I780657c1e8f67e0bef0e92a31db7ba61b57d7ec4
Related: OS#4111
Recent commit added an assertion to check for buffer boundaries and it
actually gets hit.
One of the 2 code paths calling pcu_l1if_tx_pch() was passing a buffer
of 23 bytes while one of maximum 22 is expected (because plen is not set
in the buffer but set inside pcu_l1if_tx_pch()).
So it seems before the assert, that code path was actually writing 1
byte outside the boundaries of data buffer, since bitvec_pack() uses
data_len field of bitvec.
Related: OS#4228
Fixes: 8dc09e73d0
Change-Id: I84c5dfd4d5580e9d4c00ed21887cb51bd9abbd2e
For a long time the VTY command to show all active TBFs was broken.
The TBF filtering (by allocation origin) logic allows one to show
TBFs allocated on CCCH, PACCH, or on both of them. In the latter
case we have been checking whether a TBF was allocated on both
logical channels at the same time.
Let's fix this by passing a flag-mask instead of boolean arguments.
To be able to use GPRS_RLCMAC_FLAG_* definitions from "tbf.h", let's
exclude them from "#ifdef __cplusplus ... #endif" block.
Change-Id: I1c9f401368af880a97d32905c4cce0da481ffc21
paging group is 3 bytes and imm assign with plen prepended is 23 bytes,
so there's 1 extra byte not needed and makes code confusing.
Change-Id: Id7835e5aa1506505ff54e019b38f30111f79b5dc
Otherwise, a new meas object is allocated in the stack in upper layers
which doesn't contain the link_qual information (have_link_qual=0),
outputting following error:
osmo-pcu/src/gprs_ms.cpp:644 Unable to update UL (M)CS CS-2 because we don't have link quality measurements.
Change-Id: I1980ca325c8d65f3f6310fa697dd810eec7ab077
Move code in rcv_block_gprs() only needed for rcv_control_block() into
the later. This way rcv_block_gprs() is simplified and shows similar
code paths with regards to rcv_data_block().
It can now be seen that the main difference between both is the meas
param no being passed in the control case.
Change-Id: I2a0133463edced93c72ccc743a0cf00d1d6922cf
This commit would also remove the option from config_write_pcu() since
it's automatically filled in by osmo_tdef, but there was actually a bug
because that param was never printed when saving the config...
Change-Id: Id8e70b0f44ef2f7e20ecdb3fd8ca93ae2a05b9a3
Receive an Application Information Request from the BTS via PCU
interface. Construct a Packet Application Information message from it
(3GPP TS 44.060 11.2.47) and send it to all MS with active TBF.
The TTCN-3 test infrastructure to test this feature is not quite ready
yet, so I've added C unit tests instead.
Related: OS#4048
Change-Id: Ie35959f833f46bde5f2126314b6f96763f863b36
This will allow for configuration of some of the timers by the user,
and allow him to inspect current values being used.
It will be also useful for TTCN3 tests which may want to test some of
the timers without having to wait for lots of time.
Timers are splitted into 2 groups: BTS controlled ones and PCU controlled
ones. The BTS controlled ones are read-only by the user (hence no
"timer" VTY command is provided to change them).
TbfTest.err output changes due to timers being set up correctly as a
consequence of changes. Other application such as pcu_emu.cpp and
pcu_main.cpp had to previosuly set the initial values by hand (and did
so), but apparently TbfTest.c was missing that part, which is now fixed
for free.
Depends: libosmocore.git Id56a1226d724a374f04231df85fe5b49ffd2c43c
Change-Id: I5cfb9ef01706124be262d4536617b9edb4601dd5
Pau Espin
Alexander Couzens
Vadim Yanitskiy
Eric Wild
Harald Welte
Philipp Maier
Keith Whyte
Oliver Smith