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

doc: add serialization group to Doxygen #9840

Merged
merged 7 commits into from
Oct 8, 2018

Conversation

jia200x
Copy link
Member

@jia200x jia200x commented Aug 24, 2018

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 under sys group.

The following modules where touched and added to the corresponding groups:

  • cbor: sys_serialization
  • base64: sys_serialization
  • ubjson: sys_serialization
  • cayenne_lpp: sys_serialization, pkg
  • cn-cbor: sys_serialization, pkg
  • jsnm: sys_serialization, pkg
  • minmea: sys_serialization, pkg (I might add a Parsers section later, if applies)

Testing procedure

make doc. Go to System>Serialization in the generated documentation and see how these components are grouped.

Issues/PRs references

#8479

@jia200x jia200x requested review from aabadie and miri64 August 24, 2018 15:46
@jia200x jia200x added Area: doc Area: Documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 24, 2018
@aabadie
Copy link
Contributor

aabadie commented Aug 27, 2018

@jia200x, this looks good in general but please rebase and fix the doxygen warning raised by the static tests.

@jia200x jia200x force-pushed the pr/doc_serializer branch from 3cc23eb to cbe30af Compare August 28, 2018 16:31
@@ -8,9 +8,7 @@
*/

/**
* @defgroup sys_base64 base64 encoder decoder
Copy link
Member

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.

Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

ping @smlng

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

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. ;-)

Copy link
Member Author

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 :)

@miri64 miri64 removed their request for review September 25, 2018 19:14
@smlng
Copy link
Member

smlng commented Oct 5, 2018

btw. please rebase and resolve conflicts!

@jia200x jia200x force-pushed the pr/doc_serializer branch 2 times, most recently from dc6b645 to 8622dc9 Compare October 5, 2018 12:03
@jia200x
Copy link
Member Author

jia200x commented Oct 5, 2018

@smlng done!

* @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
Copy link
Member

Choose a reason for hiding this comment

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

BSON?

Copy link
Member

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.

Copy link
Member Author

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

@jia200x jia200x force-pushed the pr/doc_serializer branch from 8622dc9 to 7df54ce Compare October 5, 2018 13:06
@jia200x
Copy link
Member Author

jia200x commented Oct 5, 2018

@miri64 done!

* @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
Copy link
Member

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)...

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. Sorry

@jia200x jia200x force-pushed the pr/doc_serializer branch from 7df54ce to 560ee54 Compare October 5, 2018 13:33
@jia200x
Copy link
Member Author

jia200x commented Oct 5, 2018

@miri64 ok, fixed the @defgroup and @brief. Now it should be OK.

Copy link
Member

@miri64 miri64 left a 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.

@@ -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
Copy link
Member

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.

@jia200x jia200x force-pushed the pr/doc_serializer branch from 560ee54 to 470cec8 Compare October 5, 2018 13:38
@jia200x
Copy link
Member Author

jia200x commented Oct 5, 2018

@miri64 oh yes, there seemed to be a problem with the last push. Now it should be fixed

Copy link
Member

@miri64 miri64 left a 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 😁 👍

@jia200x
Copy link
Member Author

jia200x commented Oct 5, 2018

yay 🎉

@miri64
Copy link
Member

miri64 commented Oct 5, 2018

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.

@jia200x
Copy link
Member Author

jia200x commented Oct 5, 2018

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

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 5, 2018
@miri64
Copy link
Member

miri64 commented Oct 5, 2018

xtimer_now64_continuity caused false negatives again :-/

@jia200x
Copy link
Member Author

jia200x commented Oct 8, 2018

xtimer_now64_continuity caused false negatives again :-/

Now it passed, can we merge then? :)

@miri64
Copy link
Member

miri64 commented Oct 8, 2018

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 ;-).

@miri64 miri64 merged commit bbae45c into RIOT-OS:master Oct 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants