net: lib: coap_client: Improve cancel function

Improve coap_client_cancel_requests(). Ensure it can be
called from a callback. Report error to waiting callbacks.
Clear active flag.

This is useful when the network becomes unavailable
or prior to disconnecting in order to save power.

Signed-off-by: Pete Skeggs <[email protected]>

Author: plskeggs

include/zephyr/net/coap_client.h

@@ -84,6 +84,7 @@ struct coap_client_internal_request {
 	uint32_t last_id;
 	uint8_t request_tkl;
 	bool request_ongoing;
+	atomic_t in_callback;
 	struct coap_block_context recv_blk_ctx;
 	struct coap_block_context send_blk_ctx;
 	struct coap_pending pending;
@@ -140,14 +141,14 @@ int coap_client_req(struct coap_client *client, int sock, const struct sockaddr
 /**
  * @brief Cancel all current requests.
  *
- * This is intended for canceling long-running requests (e.g. GETs with the OBSERVE option set)
- * which has gone stale for some reason.
+ * This is intended for canceling long-running requests (e.g. GETs with the OBSERVE option set,
+ * or a block transfer) which have gone stale for some reason. It is also intended for responding
+ * to network connectivity issues.
  *
  * @param client Client instance.
  */
 void coap_client_cancel_requests(struct coap_client *client);
 
-
 /**
  * @}
  */

subsys/net/lib/coap/coap_client.c

@@ -27,6 +27,7 @@ static atomic_t coap_client_recv_active;
 static int send_request(int sock, const void *buf, size_t len, int flags,
 			const struct sockaddr *dest_addr, socklen_t addrlen)
 {
+	LOG_HEXDUMP_DBG(buf, len, "Send CoAP Request:");
 	if (addrlen == 0) {
 		return zsock_sendto(sock, buf, len, flags, NULL, 0);
 	} else {
@@ -37,11 +38,17 @@ static int send_request(int sock, const void *buf, size_t len, int flags,
 static int receive(int sock, void *buf, size_t max_len, int flags,
 		   struct sockaddr *src_addr, socklen_t *addrlen)
 {
+	ssize_t err;
+
 	if (*addrlen == 0) {
-		return zsock_recvfrom(sock, buf, max_len, flags, NULL, NULL);
+		err = zsock_recvfrom(sock, buf, max_len, flags, NULL, NULL);
 	} else {
-		return zsock_recvfrom(sock, buf, max_len, flags, src_addr, addrlen);
+		err = zsock_recvfrom(sock, buf, max_len, flags, src_addr, addrlen);
+	}
+	if (err > 0) {
+		LOG_HEXDUMP_DBG(buf, err, "Receive CoAP Response:");
 	}
+	return err;
 }
 
 static void reset_block_contexts(struct coap_client_internal_request *request)
@@ -282,6 +289,7 @@ int coap_client_req(struct coap_client *client, int sock, const struct sockaddr
 	struct coap_client_internal_request *internal_req = get_free_request(client);
 
 	if (internal_req == NULL) {
+		LOG_DBG("No more free requests");
 		return -EAGAIN;
 	}
 
@@ -320,6 +328,7 @@ int coap_client_req(struct coap_client *client, int sock, const struct sockaddr
 	reset_internal_request(internal_req);
 
 	if (k_mutex_lock(&client->send_mutex, K_NO_WAIT)) {
+		LOG_DBG("Could not immediately lock send_mutex");
 		return -EAGAIN;
 	}
 
@@ -380,8 +389,13 @@ int coap_client_req(struct coap_client *client, int sock, const struct sockaddr
 static void report_callback_error(struct coap_client_internal_request *internal_req, int error_code)
 {
 	if (internal_req->coap_request.cb) {
-		internal_req->coap_request.cb(error_code, 0, NULL, 0, true,
-					      internal_req->coap_request.user_data);
+		if (!atomic_set(&internal_req->in_callback, 1)) {
+			internal_req->coap_request.cb(error_code, 0, NULL, 0, true,
+						      internal_req->coap_request.user_data);
+			atomic_clear(&internal_req->in_callback);
+		} else {
+			LOG_DBG("Cannot call the callback; already in it.");
+		}
 	}
 }
 
@@ -400,7 +414,9 @@ static int resend_request(struct coap_client *client,
 {
 	int ret = 0;
 
-	if (internal_req->pending.timeout != 0 && coap_pending_cycle(&internal_req->pending)) {
+	if (internal_req->request_ongoing &&
+	    internal_req->pending.timeout != 0 &&
+	    coap_pending_cycle(&internal_req->pending)) {
 		LOG_ERR("Timeout in poll, retrying send");
 
 		/* Reset send block context as it was updated in previous init from packet */
@@ -765,10 +781,16 @@ static int handle_response(struct coap_client *client, const struct coap_packet
 
 	/* Call user callback */
 	if (internal_req->coap_request.cb) {
-		internal_req->coap_request.cb(response_code, internal_req->offset, payload,
-					      payload_len, last_block,
-					      internal_req->coap_request.user_data);
-
+		if (!atomic_set(&internal_req->in_callback, 1)) {
+			internal_req->coap_request.cb(response_code, internal_req->offset, payload,
+						      payload_len, last_block,
+						      internal_req->coap_request.user_data);
+			atomic_clear(&internal_req->in_callback);
+		}
+		if (!internal_req->request_ongoing) {
+			/* User callback must have called coap_client_cancel_requests(). */
+			goto fail;
+		}
 		/* Update the offset for next callback in a blockwise transfer */
 		if (blockwise_transfer) {
 			internal_req->offset += payload_len;
@@ -820,8 +842,21 @@ static int handle_response(struct coap_client *client, const struct coap_packet
 void coap_client_cancel_requests(struct coap_client *client)
 {
 	for (int i = 0; i < ARRAY_SIZE(client->requests); i++) {
-		client->requests[i].request_ongoing = false;
+		if (client->requests[i].request_ongoing == true) {
+			LOG_DBG("Cancelling request %d", i);
+			/* Report the request was cancelled. This will be skipped if
+			 * this function was called from the user's callback so we
+			 * do not reenter it. In that case, the user knows their
+			 * request was cancelled anyway.
+			 */
+			report_callback_error(&client->requests[i], -ECANCELED);
+			client->requests[i].request_ongoing = false;
+		}
 	}
+	atomic_clear(&coap_client_recv_active);
+
+	/* Wait until after zsock_poll() can time out and return. */
+	k_sleep(K_MSEC(COAP_PERIODIC_TIMEOUT));
 }
 
 void coap_client_recv(void *coap_cl, void *a, void *b)