espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.32k stars 7.2k forks source link

Not possible to handle continuation frame frames in esp_websocket_client (IDFGH-6737) #8367

Closed MacDue closed 2 years ago

MacDue commented 2 years ago

I may be missing something, but It does not appear that it is possible to handle continuation frames in esp_websocket_client. I believe to tell if you're expecting a continuation frame you have to check for the fin flag in the websocket frame.

The current esp_websocket_event_data_t does not expose this flag, nor does it appear transport_ws.c parses it.

typedef struct {
    const char *data_ptr;                   /*!< Data pointer */
    int data_len;                           /*!< Data length */
    uint8_t op_code;                        /*!< Received opcode */
    esp_websocket_client_handle_t client;   /*!< esp_websocket_client_handle_t context */
    void *user_context;                     /*!< user_data context, from esp_websocket_client_config_t user_data */
    int payload_len;                        /*!< Total payload length, payloads exceeding buffer will be posted through multiple events */
    int payload_offset;                     /*!< Actual offset for the data associated with this event */
} esp_websocket_event_data_t;

The flag should be parsed here: https://github.com/espressif/esp-idf/blob/master/components/tcp_transport/transport_ws.c#L385 I think *data_ptr & 0x80 -- i.e. the MSB of that byte would be the fin flag (but it seems to be ignored).

Describe the solution you'd like

The bool fin flag should be added to the ws_transport_frame_state_t structure, and based on the first MSB bit of the frame (i.e. *data_ptr & 0x80), then the fin flag should also be exposed in the esp_websocket_event_data_t structure.

Describe alternatives you've considered

I'm don't believe is possible to correctly handle continuation frames without this flag.

david-cermak commented 2 years ago

@MacDue Thanks for reporting and posting the fix!

That's correct, continuation frames are not supported in the ws-transport and current version of esp_websocket_client.

I've checked your changes and they work for me. Here's my example of creating (a very simple) fragmented frame:

--- a/examples/protocols/http_server/ws_echo_server/main/ws_echo_server.c
+++ b/examples/protocols/http_server/ws_echo_server/main/ws_echo_server.c
@@ -109,6 +109,20 @@ static esp_err_t echo_handler(httpd_req_t *req)
     if (ret != ESP_OK) {
         ESP_LOGE(TAG, "httpd_ws_send_frame failed with %d", ret);
     }
+    memset(&ws_pkt, 0, sizeof(httpd_ws_frame_t));
+    ws_pkt.payload = (uint8_t*)"cont.";
+    ws_pkt.len = 5;
+    ws_pkt.type = HTTPD_WS_TYPE_TEXT;
+    ws_pkt.fragmented = true;
+    ws_pkt.final = false;
+
+    httpd_ws_send_frame(req, &ws_pkt);
+    ws_pkt.payload =  (uint8_t*)"frame";
+    ws_pkt.len = 5;
+    ws_pkt.type = HTTPD_WS_TYPE_CONTINUE;
+    ws_pkt.fragmented = true;
+    ws_pkt.final = true;
+    httpd_ws_send_frame(req, &ws_pkt);
     free(buf);
     return ret;
 }

The ws-client was able to handle it with your changes and just a simple change to propagate the FIN flag to ws data event: components/esp_websocket_client/esp_websocket_client.c

     event_data.op_code = client->last_opcode;
     event_data.payload_len = client->payload_len;
     event_data.payload_offset = client->payload_offset;
+    event_data.fin = esp_transport_ws_get_fin_flag(client->transport);

     if ((err = esp_event_post_to(client->event_handle,
                                  WEBSOCKET_EVENTS, event,
MacDue commented 2 years ago

Glad it works well, I'll make a the follow up PR for esp-protocols now & close this issue.