REMOVEME? gcoap: reuse existing request memo for observe registrations

This is ugly but maybe helps discussing the underlying problem. This is meant to allow sending an explicit deregistration request (which has to use the same token as the initial registration request), whithout creating a duplicate memo entry. The desired functionality is that a gcoap client is able to to tell the server that it is not interested in a particular observe anymore. Since the gcoap_req_send API takes a prepared buffer it is pretty clumsy to do some checks that could be done a lot easier with access to the PDU: 'is this an observe (de)-register request?', and if so, get and reuse the same memo & token. This code assumes that the request already got the correct token injected. This should happen *after* gcoap_req_init() called, before sneding the request and is currently not part of the test application.

sys/include/net/nanocoap.h

@@ -2023,7 +2023,7 @@ static inline coap_method_flags_t coap_method2flag(unsigned code)
  * @returns     0 on success
  * @returns     <0 on error
  */
-int coap_parse(coap_pkt_t *pkt, uint8_t *buf, size_t len);
+int coap_parse(coap_pkt_t *pkt, const uint8_t *buf, size_t len);
 
 /**
  * @brief   Initialize a packet struct, to build a message buffer

sys/net/application_layer/gcoap/gcoap.c

@@ -1633,12 +1633,34 @@ ssize_t gcoap_req_send_tl(const uint8_t *buf, size_t len,
      * response or request is confirmable) */
     if ((resp_handler != NULL) || (msg_type == COAP_TYPE_CON)) {
         mutex_lock(&_coap_state.lock);
-        /* Find empty slot in list of open requests. */
-        for (int i = 0; i < CONFIG_GCOAP_REQ_WAITING_MAX; i++) {
-            if (_coap_state.open_reqs[i].state == GCOAP_MEMO_UNUSED) {
-                memo = &_coap_state.open_reqs[i];
-                memo->state = GCOAP_MEMO_WAIT;
-                break;
+        coap_pkt_t req;
+        /* Uargh *cough*, ahem yes.. I know, I hate it too :'(
+         * TODO: either make this less depressing, via API refactoring.
+         * Or we just drop this for now and rely on implicit deregistration
+         * only (i.e., gcoap_obs_req_forget()). */
+        ssize_t res = coap_parse(&req, buf, len);
+        assert(res == 0);
+
+        if (coap_has_observe(&req)) {
+            uint32_t obsval = coap_get_observe(&req);
+            if (obsval == COAP_OBS_REGISTER ||
+                obsval == COAP_OBS_DEREGISTER) {
+                /* Reuse memo of the existing observe request.
+                 * This is needed because the new request to be sent semantically
+                 * belongs to the already existing request.
+                 * Otherwise, this would initiate another request with the same
+                 * token and one of them would never be removed. */
+                memo = _find_req_memo_by_pdu_token(&req, remote);
+            }
+        }
+        if (!memo) {
+            /* Find empty slot in list of open requests. */
+            for (int i = 0; i < CONFIG_GCOAP_REQ_WAITING_MAX; i++) {
+                if (_coap_state.open_reqs[i].state == GCOAP_MEMO_UNUSED) {
+                    memo = &_coap_state.open_reqs[i];
+                    memo->state = GCOAP_MEMO_WAIT;
+                    break;
+                }
             }
         }
         if (!memo) {

sys/net/application_layer/nanocoap/nanocoap.c

@@ -59,7 +59,7 @@ NANOCOAP_RESOURCE(well_known_core) COAP_WELL_KNOWN_CORE_DEFAULT_HANDLER;
 #define coap_resources_numof XFA_LEN(coap_resource_t, coap_resources_xfa)
 #endif
 
-static int _decode_value(unsigned val, uint8_t **pkt_pos_ptr, uint8_t *pkt_end);
+static int _decode_value(unsigned val, uint8_t **pkt_pos_ptr, const uint8_t *pkt_end);
 static uint32_t _decode_uint(uint8_t *pkt_pos, unsigned nbytes);
 static size_t _encode_uint(uint32_t *val);
 
@@ -76,13 +76,13 @@ static size_t _encode_uint(uint32_t *val);
  * |1 1 1 1 1 1 1 1|    Payload (if any) ...
  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  */
-int coap_parse(coap_pkt_t *pkt, uint8_t *buf, size_t len)
+int coap_parse(coap_pkt_t *pkt, const uint8_t *buf, size_t len)
 {
     coap_hdr_t *hdr = (coap_hdr_t *)buf;
     pkt->hdr = hdr;
 
     uint8_t *pkt_pos = coap_hdr_data_ptr(hdr);
-    uint8_t *pkt_end = buf + len;
+    const uint8_t *pkt_end = buf + len;
 
     pkt->payload = NULL;
     pkt->payload_len = 0;
@@ -651,7 +651,7 @@ void coap_pkt_init(coap_pkt_t *pkt, uint8_t *buf, size_t len, size_t header_len)
  * return                -EBADMSG if val is 0xF, suggesting the full byte is
  *                                the 0xFF payload marker
  */
-static int _decode_value(unsigned val, uint8_t **pkt_pos_ptr, uint8_t *pkt_end)
+static int _decode_value(unsigned val, uint8_t **pkt_pos_ptr, const uint8_t *pkt_end)
 {
     uint8_t *pkt_pos = *pkt_pos_ptr;
     size_t left = pkt_end - pkt_pos;