[riot-notifications] [RIOT-OS/RIOT] pkg/tinydtls: documents usage and configuration options (#11936)

Martine Lenders notifications at github.com
Thu Sep 12 14:53:23 CEST 2019


miri64 requested changes on this pull request.



> + *
+ * Usage
+ * -----
+ *
+ * Add as a package in the Makefile of your application:
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ {.mk}
+ * USEPKG += tinydtls
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * Supported Cipher Suites
+ * -----------------------
+ *
+ * TinyDTLS only has support for `TLS_PSK_WITH_AES_128_CCM_8` and
+ * `TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8`. The cipher suites to be used can be
+ * chosen at compile time by adding `DTLS_PSK` or `DTLS_ECC` to the CFLAGS as

```suggestion
 * chosen at compile-time by adding `DTLS_PSK` or `DTLS_ECC` to the `CFLAGS` as
```

(might need a new line-break after that

> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * Supported Cipher Suites
+ * -----------------------
+ *
+ * TinyDTLS only has support for `TLS_PSK_WITH_AES_128_CCM_8` and
+ * `TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8`. The cipher suites to be used can be
+ * chosen at compile time by adding `DTLS_PSK` or `DTLS_ECC` to the CFLAGS as
+ * follows:
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ {.mk}
+ * # Uncomment to enable
+ * # This adds support for TLS_PSK_WITH_AES_128_CCM_8
+ * # CFLAGS += -DDTLS_PSK
+ * # This adds support for TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
+ * # CFLAGS += -DDTLS_ECC

Comments within comments? How about deviding up the doc for `DTLS_PSK` and `DTLS_ECC` and have an un-commented version of each Makefile snippet.

> + *
+ * TinyDTLS only has support for `TLS_PSK_WITH_AES_128_CCM_8` and
+ * `TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8`. The cipher suites to be used can be
+ * chosen at compile time by adding `DTLS_PSK` or `DTLS_ECC` to the CFLAGS as
+ * follows:
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ {.mk}
+ * # Uncomment to enable
+ * # This adds support for TLS_PSK_WITH_AES_128_CCM_8
+ * # CFLAGS += -DDTLS_PSK
+ * # This adds support for TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
+ * # CFLAGS += -DDTLS_ECC
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * If neither `DTLS_PSK` nor `DTLS_ECC` are specified, then tinyDTLS will
+ * enable `DTLS_PSK` by default.

I think this sentence can be then compressed into your `DTLS_PSK` doc by just stating "(default)".

> + * follows:
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ {.mk}
+ * # Uncomment to enable
+ * # This adds support for TLS_PSK_WITH_AES_128_CCM_8
+ * # CFLAGS += -DDTLS_PSK
+ * # This adds support for TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
+ * # CFLAGS += -DDTLS_ECC
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * If neither `DTLS_PSK` nor `DTLS_ECC` are specified, then tinyDTLS will
+ * enable `DTLS_PSK` by default.
+ */
+
+/**
+ * @defgroup    tinydtls_config Tinydtls configuration header

```suggestion
 * @defgroup    tinydtls_config TinyDTLS compile-time configuration
```

> +#define DTLS_HANDSHAKE_MAX  (2)
+#endif
+
+ /**
+ * @brief The maximum number of concurrently used cipher keys
+ */
+#ifndef DTLS_SECURITY_MAX
+#define DTLS_SECURITY_MAX   (DTLS_HANDSHAKE_MAX + DTLS_PEER_MAX)
+#endif
+
+ /**
+ * @brief The maximum number of hash functions that can be used in parallel
+ */
+#ifndef DTLS_HASH_MAX
+#define DTLS_HASH_MAX       (3 * DTLS_PEER_MAX)
+#endif

Shouldn't the macros mentioned in the detailed section also be in here?

E.g.

```c
#defined DOXYGEN
/**
 * @brief Adds support for TLS_PSK_WITH_AES_128_CCM_8 when defined
 * @note Activated by default and @ref DTLS_ECC is not defined
 */
#define DTLS_PSK

/**
 * @brief Adds support for TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
 */
#define DTLS_ECC
#endif

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11936#pullrequestreview-287400195
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190912/6ee1fce9/attachment-0001.htm>


More information about the notifications mailing list