signetlabdei / lorawan

An ns-3 module for simulation of LoRaWAN networks
GNU General Public License v2.0
182 stars 130 forks source link

Complete the doxygen documentation #156

Closed non-det-alle closed 5 months ago

non-det-alle commented 5 months ago

I took sometime to manually write down all the missing documentation following the warnings produced by Doxygen. This allows us to solve the last failing check in the CI pipline.

On top of Doxygen warnings, I did some maintenance to default comments for constructor, destructor and GetTypeId functions following the indications of the ns-3 manual on the subject. Some further maintenance was to make sure that all sentences start with a capital letter and end with a column. I did not fully review existing documentation, but I think this is currently enough.

This pull request exclusively concerns code documentation. With the exception of a couple of function reordering in headers, no changes were applied to the running part of the code. That said, this manual review lead to me finding many problems: for that I left \todo tags in the Doxygen comments.

I tried to keep the commit history as clean as possible for review purposes. With few exceptions, commit comments starting with document concern one single header file among the ones affected by warnings. At the end, we can squash them all together.

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 89.39394% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 76.24%. Comparing base (69cb2e3) to head (bc53a0d). Report is 1 commits behind head on develop.

:exclamation: Current head bc53a0d differs from pull request most recent head 39f1b8a. Consider uploading reports for the commit 39f1b8a to get more accurate results

Files Patch % Lines
model/end-device-status.cc 0.00% 4 Missing :warning:
helper/lorawan-mac-helper.cc 0.00% 2 Missing :warning:
test/lorawan-test-suite.cc 83.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #156 +/- ## =========================================== - Coverage 76.27% 76.24% -0.04% =========================================== Files 69 69 Lines 5451 5434 -17 =========================================== - Hits 4158 4143 -15 + Misses 1293 1291 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

non-det-alle commented 5 months ago

Thank you @pagmatt and @albertogara for the enormous effort of reviewing the API documentation. Let me know if you need more time.

I fixed most of your suggestions, leaving some yet to do or open for further discussion, and rebased onto develop. I am trying to generalize your comments to the whole API documentation, because many of your remarks are also present in other API comments I had not touched with the initial changes intended to solve the Doxygen warnings. Tomorrow I'll close up what's left to do.

You may have noticed that we are having problems with the CI pipeline after upgrading to ns-3.41. It appears that one test suite will always fail if we reuse cached build files, while it will work when the cache is not used. I have not been able to reproduce the problem locally yet, but I'll try to fix it before release.

Best, Alessandro

pagmatt commented 5 months ago

Thank you @pagmatt and @albertogara for the enormous effort of reviewing the API documentation. Let me know if you need more time.

I fixed most of your suggestions, leaving some yet to do or open for further discussion, and rebased onto develop. I am trying to generalize your comments to the whole API documentation, because many of your remarks are also present in other API comments I had not touched with the initial changes intended to solve the Doxygen warnings. Tomorrow I'll close up what's left to do.

You may have noticed that we are having problems with the CI pipeline after upgrading to ns-3.41. It appears that one test suite will always fail if we reuse cached build files, while it will work when the cache is not used. I have not been able to reproduce the problem locally yet, but I'll try to fix it before release.

Best, Alessandro

From my side, I honestly don't think I will have the time (surely not in the next few days) to go through the documentation again. So I suggest to simply finish discussing the few open points, then merge this and thus move forward with the updated release and finally have a fully-working CI pipeline.

non-det-alle commented 5 months ago

Hello, if you agree we can merge the branch once the CI pipeline is done.