Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: bsdlib: rename to nrf_modem #3394

Merged
merged 1 commit into from
Dec 15, 2020
Merged

Conversation

lemrey
Copy link
Contributor

@lemrey lemrey commented Nov 25, 2020

Rename bsdlib to nrf_modem (library) and libmodem nrf_modem_lib (glue).
reminder! The two commits must be squashed.

@lemrey lemrey requested a review from umapraseeda November 25, 2020 09:42
@lemrey lemrey force-pushed the libmodem-release branch 3 times, most recently from 86dcf91 to efe43e6 Compare November 25, 2020 13:50
@evenl
Copy link
Contributor

evenl commented Nov 30, 2020

I am a bit confused about the different naming conventions used for this library:

Before with bsdlib:

Files: libbsd_nrf9160_xxaa.a, bsd_os.c, bsdlib.c, nrf91_sockets.c
Folder: bsdlib (both nrf and nrfxlib)
Kconfig nrfxlib: CONFIG_BSD_LIB_
Kconfig nrf: CONFIG_BSD_LIBRARY_, CONFIG_NRF91_SOCKET

Today with modemlib:

File libmodem.a (nrfxlib), libmodem.c (nrf), nrf91_sockets.c (nrf), nrf_modem_os.c (nrf)
Folder: nrf_modem (nrfxlib), libmodem (nrf)
Kconfig nrfxlib: CONFIG_NRF_MODEM
Kconfig nrf: CONFIG_LIBMODEM, CONFIG_NRF91_SOCKETS_

I think it make more sense to call the folder in nrf for nrf_ modem (same as nrfxlib) instead of libmodem? I think it also make more sense to rename libmodem.c to nrf_modem.c, and the CONFIG_LIBMODEM kconfig parameters to CONFIG_NRF_MODEM_LIB_. I am fine with the .a file other that it should probably contain some device information like nrf9160 at least.

A common convention is to name the library file (.a) libfoobar.a, but any references to the library is usually "foobar library" or "foobar lib". Of course both conventions can be found, but for me nrf_modem, nrf_modem_lib or nrf_modem_library make more sense for folders, files and kconfig paramters extern to the library it self, and align more with the naming convention used for bsdlib.

@lemrey
Copy link
Contributor Author

lemrey commented Nov 30, 2020

@evenl The point was to create a distinction (or rather maintain one) between the glue and the library. I think there needs to be.
Here we have libmodem.h, libmodem_init, libmodem.c (under /libmodem) and it's clear that it's glue and it belongs to sdk-nrf.

Following your suggestion, if I renamed the glue to nrf_modem we'd end up with:

  • nrf_modem.h, duplicate of main header in nrfxlib/nrf_modem/include/nrf_modem.h
  • nrf_modem.c, which would not be the implementation of above header from nrfxlib
  • nrf_modem_init in sdk-nrf nrf_modem.c, duplicate of nrf_modem_init in above header

So in conclusion it would be hard to make that distinction anymore (including the distinction between library-defined configurations and glue-defined configurations) and we'd have collisions.

@evenl
Copy link
Contributor

evenl commented Dec 1, 2020

Yes, I understand, but my point is that it is not clear to me that the libmodem prefixed stuff and the nrf_modem stuff belongs together. For bsdlib it was bsd_ for the nrfxlib stuff, and bsdlib_ for the OS integration part, which I think is fairly easy to understand that it belongs together. I think it would have been much more logical with for instance nrf_modem_ for the nrfxlib stuff and nrf_modem_lib_ for the OS integration part.

@rlubos
Copy link
Contributor

rlubos commented Dec 1, 2020

I think that Even might have a point here, that libmodem might not be easily connected to nrf_modem for someone not familiarized with the nomenclature. nrf_modem_lib sounds to be better associated with the actual nrfxlib component, the name (and the prefix) is a bit long, but to be honest, nothing better comes to my mind.

@evenl
Copy link
Contributor

evenl commented Dec 1, 2020

Yes, I agree that it's a bit long, but same here, I can not come up with any better.

@lemrey
Copy link
Contributor Author

lemrey commented Dec 1, 2020

I see, point taken. I can change to nrf_modem_lib though I preferred libmodem :)

@@ -112,11 +112,11 @@ nRF9160
* nRF Connect SDK now uses upstream CoAP implementation. The :ref:`mqtt_simple_sample` sample was rewritten to use the upstream library, and the downstream CoAP was removed.
* The :ref:`http_application_update_sample` sample has been updated to use the :ref:`lib_fota_download` library.

Modem library
BSD library
-------------
Copy link
Contributor Author

@lemrey lemrey Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose these should stop at the same column as BSD library, check elsewhere too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed these..

@lemrey lemrey requested review from eibe85 and removed request for ioannisg, anangl and lats1980 December 7, 2020 11:49
@lemrey lemrey force-pushed the libmodem-release branch 2 times, most recently from 96e3b10 to 055054c Compare December 7, 2020 12:43
@lemrey lemrey force-pushed the libmodem-release branch 3 times, most recently from 4c9ebc8 to 424bd2b Compare December 9, 2020 13:12
@lemrey
Copy link
Contributor Author

lemrey commented Dec 9, 2020

Rebased again to try and get CI green.

@lemrey lemrey force-pushed the libmodem-release branch 4 times, most recently from fffcf02 to 5f3a90f Compare December 14, 2020 15:12
@lemrey lemrey force-pushed the libmodem-release branch 2 times, most recently from 79fea44 to 4dd0b2c Compare December 15, 2020 11:39
west.yml Outdated Show resolved Hide resolved
Rename bsdlib to nrf_modem (library) and nrf_modem_lib (integration).

Signed-off-by: Emanuele Di Santo <emdi@nordicsemi.no>
Signed-off-by: Uma Praseeda <uma.praseeda@nordicsemi.no>
@lemrey lemrey removed the DNM label Dec 15, 2020
@rlubos rlubos merged commit 82210bf into nrfconnect:master Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval. manifest manifest-nrfxlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants