drivers/can: fix doxygen documentation + typo #7289

aabadie · 2017-07-02T18:09:57Z

I found some problems in the CAN doxygen documentation:

some typos
grouping problems

This PR solves the 2.

miri64 · 2017-07-02T18:34:27Z

cpu/native/can/candev_linux.c

@@ -7,7 +7,7 @@
 */

 /**
- * @ingroup     native_cpu
+ * @ingroup     candev_linux


Not sure about this name... And technically it is in both, so I would keep the removed line.

If one keeps the two, it results in a very weird rendering of the parent groups breadcrumbs. I think removing is one of the best solution.
Maybe I can add a ref to native_cpu: @see native_cpu ?

C-files aren't rendered by our doxygen-config anyways (so there are no breadcrumbs), and @smlng made a move to do that grouping for C-files.

miri64 · 2017-07-02T18:35:14Z

cpu/native/include/candev_linux.h

@@ -7,9 +7,9 @@
 */

 /**
- * @ingroup     native_cpu
- * @ingroup     drivers_can
 * @defgroup    candev_linux SocketCAN driver


How about drivers_candev_linux instead?

makes sense

miri64 · 2017-07-02T18:39:36Z

drivers/doc.txt

+/**
+ * @defgroup    drivers_can CAN Drivers
+ * @ingroup     drivers
+ * @ingroup     can


(don't know where this is defined, but) we don't have a directory can in RIOTBASE, so NACK for the name alone. Second, why do we need it? Third, why ist there a whole subgroup under drivers, just for CAN alone?

Because CAN is also a separate stack. This group is defined in sys/include/can/doc.txt. There other opened PR (I 3 or 4) that will provide CAN hardware driver. I was unsure if it should go in the netdev_drivers.

LoRa is a seperate stack, too. This doesn't imply that it needs a whole new group. If can devices don't fit into drivers_netdev for some reason (because CAN isn't networking (?)) how about renaming it to something like "communication devices"?

renaming it to something like "communication devices"

Do you mean renaming "Network Device Drivers" (drivers_netdev) to "Communication Device Drivers" (drivers_communication) ?

Yes, but I don't get, why we can't keep the name, just because CAN doesn't use the netdev API... After all, CAN stands for Controller Area Network ;)

For simplicity, we can keep drivers_netdev, but the displayed name can become "Communication Device Drivers".

After all, CAN stands for Controller Area Network ;)

Sure!

I even would keep the displayed name...

TBH I don't even know, why it has its own API... Looking at the API, all of that could have been done with netdev...

miri64 · 2017-07-02T18:40:50Z

sys/include/can/can.h

@@ -7,16 +7,14 @@
 */

 /**
- * @ingroup     can
 * @defgroup    can_dll Data Link Layer


miri64 · 2017-07-02T18:41:04Z

sys/include/can/can.h

 * @defgroup    can_dll Data Link Layer
+ * @ingroup     can


miri64 · 2017-07-02T18:41:28Z

sys/include/can/common.h

 * @defgroup   can_common  Common
+ * @ingroup    can


miri64 · 2017-07-02T18:41:49Z

sys/include/can/common.h

@@ -7,8 +7,8 @@
 */

 /**
- * @ingroup    can
 * @defgroup   can_common  Common


sys_can_common

miri64

Names should adhere to the doc group naming scheme and this PR doesn't fix the grouping IMHO.

miri64 · 2017-07-02T18:42:19Z

sys/include/can/isotp.h

@@ -7,12 +7,11 @@
 */

 /**
+ * @defgroup    can_isotp ISOTP


sys_can_isotp

aabadie · 2017-07-02T18:44:49Z

Names should adhere to the doc group naming scheme and this PR doesn't fix the grouping IMHO.

For the moment, I'll update it so that it does

aabadie · 2017-07-02T20:29:14Z

@miri64, comments addressed:

drivers_can is now a subgroup of drivers_netdev
sys_can is now a subgroup of net

I removed the multi-parent grouping because of the bad breadcumbs rendering and put @see instead for cross referencing sys_can and drivers_can from each other.

miri64 · 2017-07-02T21:01:35Z

drivers/include/can/doc.txt

+ * @defgroup    drivers_can CAN Drivers
+ * @ingroup     drivers_netdev
+ * @brief       Drivers for CAN devices
+ * @see         sys_can


Can you add a @note These devices are not covered by the @ref drivers_netdev_api?

and add but by the @ref drivers_candev "candev API"

@miri64, comments addressed.

miri64 · 2017-07-02T21:08:46Z

sys/include/can/isotp.h

@@ -7,12 +7,11 @@
 */

 /**
- * @ingroup     can
- * @defgroup    isotp ISOTP
+ * @defgroup    sys_can_isotp ISOTP


Can you please pull up the wording from the brief up here (without the ISO number), but leave the @brief as is? ISOTP isn't really saying anything to me ;-).

aabadie · 2017-07-03T12:47:26Z

@miri64, are you ok with this one ?

miri64

Apart from some nitpicks: ACK. Please squash

miri64 · 2017-07-03T13:38:30Z

sys/include/can/conn/raw.h

@@ -7,15 +7,14 @@
 */

 /**
- * @ingroup     can
- * @defgroup    conn_can Connection
+ * @defgroup    sys_conn_can Connection


nitpicky: sys_can_conn

miri64 · 2017-07-03T13:38:54Z

drivers/can_trx/can_trx.c

@@ -7,16 +7,15 @@
 */

 /**
- * @defgroup    drivers_can transceiver
- * @ingroup     drivers
+ * @ingroup     drivers_trx_can


nitpicky: sys_can_trx

I guess you mean drivers_can_trx

miri64 · 2017-07-03T13:39:30Z

sys/include/can/isotp.h

@@ -7,12 +7,11 @@
 */

 /**
- * @ingroup     can
- * @defgroup    isotp ISOTP
+ * @defgroup    sys_can_isotp ISO transport protocol over CAN (ISO15765)


I think the ISO number can be removed here.

aabadie · 2017-07-03T18:44:29Z

@miri64, remaining nits addressed, directly squashed.

aabadie added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: doc Area: Documentation labels Jul 2, 2017

aabadie added this to the Release 2017.07 milestone Jul 2, 2017

aabadie assigned miri64 and vincent-d Jul 2, 2017

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 2, 2017

miri64 reviewed Jul 2, 2017

View reviewed changes

sys/include/can/can.h Outdated

@@ -7,16 +7,14 @@

*/

/**

* @ingroup can

* @defgroup can_dll Data Link Layer

Copy link

Member

miri64 Jul 2, 2017

Choose a reason for hiding this comment

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

sys_can

miri64 reviewed Jul 2, 2017

View reviewed changes

sys/include/can/can.h Outdated

* @defgroup can_dll Data Link Layer

* @ingroup can

Copy link

Member

miri64 Jul 2, 2017

Choose a reason for hiding this comment

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

sys

miri64 reviewed Jul 2, 2017

View reviewed changes

sys/include/can/common.h Outdated

* @defgroup can_common Common

* @ingroup can

Copy link

Member

miri64 Jul 2, 2017

Choose a reason for hiding this comment

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

sys_can

miri64 reviewed Jul 2, 2017

View reviewed changes

sys/include/can/common.h Outdated

@@ -7,8 +7,8 @@

*/

/**

* @ingroup can

* @defgroup can_common Common

Copy link

Member

miri64 Jul 2, 2017

Choose a reason for hiding this comment

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

sys_can_common

miri64 requested changes Jul 2, 2017

View reviewed changes

sys/include/can/isotp.h Outdated

@@ -7,12 +7,11 @@

*/

/**

* @defgroup can_isotp ISOTP

Copy link

Member

miri64 Jul 2, 2017

Choose a reason for hiding this comment

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

sys_can_isotp

aabadie force-pushed the can_fix_doc branch from 47d78c9 to af6a5c5 Compare July 2, 2017 20:26

miri64 requested changes Jul 2, 2017

View reviewed changes

aabadie force-pushed the can_fix_doc branch from af6a5c5 to b5ac157 Compare July 2, 2017 21:27

miri64 approved these changes Jul 3, 2017

View reviewed changes

drivers/can: fix doxygen documentation + typo

268c6b6

aabadie force-pushed the can_fix_doc branch from b5ac157 to 268c6b6 Compare July 3, 2017 18:44

miri64 merged commit 7ea64e3 into RIOT-OS:master Jul 3, 2017

aabadie deleted the can_fix_doc branch February 26, 2018 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/can: fix doxygen documentation + typo #7289

drivers/can: fix doxygen documentation + typo #7289

aabadie commented Jul 2, 2017

miri64 Jul 2, 2017

aabadie Jul 2, 2017

miri64 Jul 2, 2017

miri64 Jul 2, 2017

aabadie Jul 2, 2017

miri64 Jul 2, 2017

aabadie Jul 2, 2017

miri64 Jul 2, 2017

aabadie Jul 2, 2017

miri64 Jul 2, 2017

aabadie Jul 2, 2017

miri64 Jul 2, 2017

miri64 Jul 2, 2017

miri64 Jul 2, 2017

miri64 Jul 2, 2017

miri64 Jul 2, 2017

miri64 Jul 2, 2017

miri64 left a comment

miri64 Jul 2, 2017

aabadie commented Jul 2, 2017

aabadie commented Jul 2, 2017

miri64 Jul 2, 2017

miri64 Jul 2, 2017

aabadie Jul 3, 2017

miri64 Jul 2, 2017

aabadie commented Jul 3, 2017

miri64 left a comment

miri64 Jul 3, 2017

miri64 Jul 3, 2017

aabadie Jul 3, 2017

miri64 Jul 3, 2017

miri64 Jul 3, 2017

aabadie commented Jul 3, 2017

drivers/can: fix doxygen documentation + typo #7289

drivers/can: fix doxygen documentation + typo #7289

Conversation

aabadie commented Jul 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie commented Jul 2, 2017

aabadie commented Jul 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie commented Jul 3, 2017

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie commented Jul 3, 2017