netopt: Clarify documentation by jnohlgard · Pull Request #8655 · RIOT-OS/RIOT

jnohlgard · 2018-02-28T07:28:05Z

Contribution description

Improve the documentation for netopt_t to always specify data type, and some minor language/editorial changes to match the new format.

Added the word "Netopt" to the page title to make the page easier to find in the Doxygen sidebar modules table of contents.

Issues/PRs references

See #8631 for a related discussion.

miri64

Initial (and superficial) review

miri64 · 2018-02-28T10:24:05Z

sys/include/net/netopt.h

+     */
+    NETOPT_IS_CHANNEL_CLR,
+    /**
+     * @brief   (various, see below) link layer address


This can be any byte array. It just has to be long enough for the address type

miri64 · 2018-02-28T10:26:22Z

sys/include/net/netopt.h

-    NETOPT_CHANNEL,             /**< get/set channel as uint16_t in host
-                                 *   byte order */
-    NETOPT_IS_CHANNEL_CLR,      /**< check if channel is clear */
-    NETOPT_ADDRESS,             /**< get/set address in host byte order */


I actually don't think the address is ever returned in host byte order, but always in network byte order, which also would make more sense.

miri64 · 2018-02-28T10:26:52Z

sys/include/net/netopt.h

+     *
+     * Device type   | Type     | Meaning
+     * ------------- | -------- | -----
+     * IEEE 802.15.4 | uint16_t | device short address


This table could provide the minimum required length of the array by NETDEV_DEVICE_TYPE_….

miri64 · 2018-02-28T10:30:25Z

sys/include/net/netopt.h


    /**
-     * @brief    get/set long address in host byte order
+     * @brief   (various, see below) long link layer address


Can also be an array.

miri64 · 2018-02-28T10:30:41Z

sys/include/net/netopt.h

+     * Device type   | Type         | Meaning
+     * ------------- | ------------ | -----
+     * IEEE 802.15.4 | @ref eui64_t | device long address (EUI-64)
+     * Ethernet      | FIXME        | device MAC address??


Ethernet returns -ENOTSUP ;-)

miri64 · 2018-02-28T10:32:17Z

sys/include/net/netopt.h

+    NETOPT_ENCRYPTION,
+
+    /**
+     * @brief   (various, see below) set encryption key


Also array? Also it doesn't really specify a type below.

miri64 · 2018-03-07T11:56:04Z

@gebart ping?

jnohlgard · 2018-03-07T12:40:09Z

Will address comments tonight.

jnohlgard · 2018-03-08T05:06:26Z

Comments addressed

jnohlgard · 2018-03-13T08:23:29Z

ping @miri64

miri64 · 2018-03-13T15:51:59Z

sys/include/net/netopt.h

+     * Device type   | Length   | Meaning
+     * ------------- | -------- | -----
+     * IEEE 802.15.4 | 8        | device long address (EUI-64), @ref eui64_t
+     * Ethernet      | -ENOTSUP | not used


Just leave it out. Otherwise you would have to add cc110x, BLE etc. here as well ;-).

miri64 · 2018-03-13T18:22:20Z

sys/include/net/netopt.h

    /**
-     * @brief   get IPv6 addresses of an interface as array of @ref ipv6_addr_t
-     *          or add an IPv6 address as @ref ipv6_addr_t to an  interface
+     * @brief   (@ref ipv6_addr_t) get IPv6 addresses of an interface as array


For get it can be an array of ipv6_addr_t.

miri64 · 2018-03-13T18:23:24Z

sys/include/net/netopt.h

+     * Device type   | Length | Meaning
+     * ------------- | ------ | -----
+     * IEEE 802.15.4 | 2      | device short address
+     * Ethernet      | 6      | device MAC address


Maybe add the other types of NETDEV_DEVICE_TYPE here as well?

miri64 · 2018-03-13T18:24:27Z

sys/include/net/netopt.h

+     * @brief   (uint32_t) reception timeout of a frame
     *
-     * Values are retrieved/passed as uint32_t in host byte order.
+     * TODO in what time unit?


Use @todo tag?

miri64 · 2018-03-13T18:24:36Z

sys/include/net/netopt.h

+     * @brief   (uint32_t) transmission timeout of a frame
     *
-     * Values are retrieved/passed as uint32_t in host byte order.
+     * TODO in what time unit?


Use @todo tag?

miri64 · 2018-03-13T18:26:07Z

sys/include/net/netopt.h

+     */
+    NETOPT_IS_CHANNEL_CLR,
+    /**
+     * @brief   (byte array, see below) link layer address


somewhere here you should mention that the address is to be in network byte order (see your comment above).

miri64 · 2018-03-13T18:26:12Z

sys/include/net/netopt.h


    /**
-     * @brief    get/set long address in host byte order
+     * @brief   (byte array, see below) long link layer address


somewhere here you should mention that the address is to be in network byte order (see your comment above).

miri64 · 2018-03-13T18:27:56Z

sys/include/net/netopt.h


    /**
-     * @brief Test mode for the radio, e.g. for CE or FCC certification
+     * @brief   (byte array) set encryption key


I'm not sure if this is in host or network byte order, but network byte order makes more sense for me here.

Does an encryption key have a byte order, or is it simply regarded as an array of bytes?
e.g. do implementations actually reverse 128 bit AES keys when distributing them or are the algorithm specifications written to treat the keys as byte blobs when implementing?

I honestly don't know, so I guess it is best to keep it unspecified for now.

jnohlgard · 2018-03-23T13:56:01Z

@miri64 ping

miri64 · 2018-03-26T13:29:27Z

sys/include/net/netopt.h


    /**
-     * @brief (@ref netopt_enable_t) Phy link status.
+     * @brief   (uint8_t) device type


This is supposed to be uint16_t, at least GNRC expects it to be ;-).

jnohlgard · 2018-03-26T14:16:20Z

@miri64 comments addressed

miri64

ACK

miri64 · 2018-03-27T16:02:31Z

(please squash)

jnohlgard · 2018-03-30T15:51:52Z

@miri64 Thanks for the review!

Documentation of the option types was clarified in RIOT-OS#8655. I only noticed after merging RIOT-OS#8884, that `NETOPT_BLE_CTX` was not documented using that new style. So I deliver the change myself to make it quicker.

jnohlgard added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Area: doc Area: Documentation labels Feb 28, 2018

jnohlgard added this to the Release 2018.04 milestone Feb 28, 2018

jnohlgard assigned miri64 Feb 28, 2018

jnohlgard requested a review from miri64 February 28, 2018 07:28

jnohlgard mentioned this pull request Feb 28, 2018

at86rf2xx/kw2xrf: scalar NETOPT options checked as arrays #8631

Closed

miri64 requested changes Feb 28, 2018

View reviewed changes

jnohlgard force-pushed the pr/netopt-docs branch from bcddb9c to 0cd75a4 Compare March 8, 2018 05:05

jnohlgard added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 8, 2018

miri64 requested changes Mar 13, 2018

View reviewed changes

jnohlgard force-pushed the pr/netopt-docs branch from 0cd75a4 to af37d86 Compare March 16, 2018 08:37

miri64 mentioned this pull request Mar 17, 2018

netopt/drivers: Add phy link status output #8601

Merged

2 tasks

jnohlgard force-pushed the pr/netopt-docs branch from af37d86 to 0fb1c5f Compare March 23, 2018 14:01

miri64 requested changes Mar 26, 2018

View reviewed changes

miri64 approved these changes Mar 26, 2018

View reviewed changes

netopt: Clarify documentation

cf8b370

jnohlgard force-pushed the pr/netopt-docs branch from 9bd73f4 to cf8b370 Compare March 29, 2018 17:49

jnohlgard merged commit 59d6f10 into RIOT-OS:master Mar 29, 2018

jnohlgard deleted the pr/netopt-docs branch March 29, 2018 18:28

This was referenced Apr 6, 2018

netopt: move NETOPT_BLE_CTX to new netopt doc style #8892

Merged

net: add Skald, a BLE advertising stack #8884

Merged

Conversation

jnohlgard commented Feb 28, 2018

Contribution description

Issues/PRs references

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Mar 7, 2018

Uh oh!

jnohlgard commented Mar 7, 2018

Uh oh!

jnohlgard commented Mar 8, 2018

Uh oh!

jnohlgard commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Mar 26, 2018

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Mar 27, 2018

Uh oh!

jnohlgard commented Mar 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants