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=0ed60f68b86...
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.
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=459791c488c6b66a5cd... 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
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.
On Sun, Jan 14, 2018 at 11:40:30PM +0600, Vadim Yanitskiy wrote:
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.
fine with me.
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
baseband-devel@lists.osmocom.org
-
Harald Welte -
Vadim Yanitskiy