13 Jan
2018
13 Jan
'18
9:19 p.m.
Hi Sylvain,
as you probably already know, I made a few changes in GAPK
and now they are in a separate branch called 'fixeria/lib'.
In a few words, the main idea of this set of changes is to
compose a shared library called libosmogapk and add some
basic test coverage. Please see:
http://git.osmocom.org/gapk/log/?h=fixeria/lib
As you're the maintainer and author of this project, I would
like to ask you: is it possible to merge this branch into
the mainline?
At the moment, the new library is used by OsmocomBB, see:
http://git.osmocom.org/osmocom-bb/commit/?h=fixeria/audio&id=0ed60f68b8…
and moreover I am developing a set of GNU Radio blocks for
GSM audio transcoding based on the GAPK library. Probably,
some other projects may also benefit from a possibility to
link against a shared library and use its features...
If you support this idea, please let me know, which way
of merging the branch is better for you:
- using Gerrit, where I am not sure: if I will send a
merge commit for review, would it 'drag' the whole
long chain of commits, or the only one?
- or using your write access rights.
In both cases, I'll provide you detailed description of
the merge commit. But first, I need to know your decision.
With best regards,
Vadim Yanitskiy.
14 Jan
14 Jan
11:28 a.m.
Hi Vadim,
just some feedback "from the side":
On Sun, Jan 14, 2018 at 02:19:46AM +0600, Vadim Yanitskiy wrote:
I've done some casual review and it looks great to me. There are
some minor issues:
* I would suggest internal symbols (those not with osmo_gapk_ prefix,
like gapk_log_subsys, LOGPAPK, ...)
use an internal header file which is not installed in the system.
The header files installed during 'make install' should only describe
the external API of the library
* the source/sink/coec category name stringification at
http://git.osmocom.org/gapk/commit/?h=fixeria/lib&id=459791c488c6b66a5c…
should not define those strings in every file, but have a value_string
or at least some #defines or the like.
This is just my point of view, detailed review of course depends on Sylvain.
I suggest you simply rebase your chain of commits on current master and push
it into gerrit. That's what we have it for, and as indicated in
http://lists.osmocom.org/pipermail/openbsc/2017-December/011523.html
gapk has been introduced to gerrit.
Regards,
Harald
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
6:40 p.m.
Hi Harald,
I've done some casual review and it looks great to
me.
Thanks.
I would suggest internal symbols (those not with
osmo_gapk_ prefix,
like gapk_log_subsys, LOGPAPK, ...) use an internal header file which
is not installed in the system. The header files installed during
'make install' should only describe the external API of the library
The mentioned logging related symbols are not installed:
noinst_HEADERS = \
osmocom/gapk/logging.h \
...
The only thing I am not sure about is 'get_cycles.h' header. At the
moment, it is installed together with other headers, because it's
a dependency of 'benchmark.h'. But, it seems I just got an idea,
how to avoid this dependency - simply use 'unsigned long long'
instead of the 'cycles_t', which is imported from 'get_cycles.h'.
the source/sink/coec category name stringification
should not
define those strings in every file, but have a value_string
or at least some #defines or the like.
Thanks, good idea.
I suggest you simply rebase your chain of commits on
current
master and push it into gerrit.
But this way, each commit of the whole chain would require +2
and V+1, because 'make check' was implemented at the end of
the chain.
What if I merge 'fixeria/lib' into a local 'master' and send
it for review? I think, this would be simpler. Or would this
anyway send the whole chain to review? :)
With best regards,
Vadim Yanitskiy.
7:35 p.m.
On Sun, Jan 14, 2018 at 11:40:30PM +0600, Vadim Yanitskiy wrote:
fine with me.
The mentioned logging related symbols are not
installed:
ok, thanks.
I suggest you
simply rebase your chain of commits on current
master and push it into gerrit.
But this way, each commit of the whole chain would require +2
and V+1, because 'make check' was implemented at the end of
the chain. What if I merge 'fixeria/lib' into a local
'master' and send
it for review? I think, this would be simpler. Or would this
anyway send the whole chain to review? :)
I don't really know what gerrit does in case of merges, as we don't
generally use merges much but prefer a linear history. You could try,
if you're interested.
In any case, let's wait and see what Sylvain says regarding the code
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
2302
days inactive
2303
days old
baseband-devel@lists.osmocom.org
3 comments
2 participants
participants (2)
-
Harald Welte -
Vadim Yanitskiy