-
Notifications
You must be signed in to change notification settings - Fork 2k
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
doc: add serialization
group to Doxygen
#9840
Conversation
@jia200x, this looks good in general but please rebase and fix the doxygen warning raised by the static tests. |
3cc23eb
to
cbe30af
Compare
sys/include/base64.h
Outdated
@@ -8,9 +8,7 @@ | |||
*/ | |||
|
|||
/** | |||
* @defgroup sys_base64 base64 encoder decoder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these changes, IIRC I had this discussion before and we concluded to use doc.txt
for group definition only when there is no proper header file. However, here we have these header files, so no need to introduce doc.txt
files, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean for sys_base64
, sys_ubjson
and the like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmmm, I could revert this change. I was adding doc.txt files because it's easier to write per-module documentation in a doc file rather than headers. Shall I revert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @smlng
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as said I had this discussion in one of my own PRs and the consensus was to use header files where possible and limit the usage of doc.txt
to cases where no header file is available for the module or group.
TL;DR: in this case I would suggest to revert, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miri64 IIRC we had this discussion, any thought/comments here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that adding doc.txt
when not actually needed is not the way to go. The documentation stays with the code this way and isn't "hidden" in some file nobody ever really looks at. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will revert them :)
btw. please rebase and resolve conflicts! |
dc6b645
to
8622dc9
Compare
@smlng done! |
sys/include/ubjson.h
Outdated
* @brief A library to read and write UBJSON serialized data. | ||
* @defgroup sys_ubjson Universal Binary JSON deserializer | ||
* @ingroup sys_serialization | ||
* @brief Provides a BSON deserializer library to RIOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BSON?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify my question after a little bit of research: I don't think BSON and UBJSON are the same thing. According to Wikipedia UBJSON is aimed to be the successor of BSON, but that still wouldn't be the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will differentiate them
8622dc9
to
7df54ce
Compare
@miri64 done! |
sys/include/ubjson.h
Outdated
* @brief A library to read and write UBJSON serialized data. | ||
* @defgroup sys_ubjson Universal Binary JSON deserializer | ||
* @ingroup sys_serialization | ||
* @brief Provides a UBJSON deserializer library to RIOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still weird, that the original doc said, the lib is able to read and write UBJSON, but your new doc states its only able to deserialize (i.e. read)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Sorry
7df54ce
to
560ee54
Compare
@miri64 ok, fixed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the drippy non-review. Every time I look at this PR I see something new 🙄. But this should be my last comment now.
doc/doxygen/src/getting-started.md
Outdated
@@ -39,7 +39,7 @@ simplest way to compile and link an application with RIOT, is to set up a | |||
Makefile providing at least the following variables: | |||
|
|||
* `APPLICATION`: should contain the (unique) name of your application | |||
* `BOARD`: specifies the platform the application should be build for by | |||
* `BOARD`: specifies the platform the application should be built for by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was also fixed in #10113 so you might not have rebased properly.
560ee54
to
470cec8
Compare
@miri64 oh yes, there seemed to be a problem with the last push. Now it should be fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As promised, now I'm fine with this PR as well 😁 👍
yay 🎉 |
Wait -.- as far as I can see, lora-serilization is missing (#9863 was merged after this PR was opened). But I guess we can do that as a follow-up. |
Sure, I can open the follow up |
|
Now it passed, can we merge then? :) |
Yupp let's do it. Even though the build is already a few days old we have to consider that this is just a doc update, so I think we don't need to retest ;-). |
Contribution description
This PR is the first of a series of PR for improving the documentation.
Here I group several serialization/deserialization format in a
Serialization
group, so they are easier to find for the end user.A
sys_serialization
group was defined undersys
group.The following modules where touched and added to the corresponding groups:
Testing procedure
make doc
. Go toSystem>Serialization
in the generated documentation and see how these components are grouped.Issues/PRs references
#8479