Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bump(websocket): 1.2.1 -> 1.2.2 #466

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

gabsuren
Copy link
Contributor

1.2.2
Bug Fixes

1.2.2
Bug Fixes
- continuation after FIN in websocket client (espressif#460) (774d1c7)
- Re-applie refs to common comps idf_component.yml (9fe44a4)
@gabsuren gabsuren force-pushed the websocket_bump_1.2.2 branch from 970ab3f to eb76993 Compare January 16, 2024 08:17
Copy link
Collaborator

@david-cermak david-cermak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked the original PR and added few comments: #368

I'd suggest looking at and addressing these three issues:

  1. esp_websocket_client_send_text() API might not include FIN flag
  2. esp_websocket_client_send_bin() API might send message without the mandatory FIN flag.
  3. Updateesp_websocket_client_send_with_exact_opcode() to include one additional parameter indicating whether or not to add the FIN flag in the last fragment.
  4. These APIs do not lock the client not check parameters:
  • esp_websocket_client_send_text_partial()
  • esp_websocket_client_send_cont_msg()
  • esp_websocket_client_send_bin_partial()
  • esp_websocket_client_send_fin()
  1. Check and fix the behaviour under CONFIG_ESP_WS_CLIENT_ENABLE_DYNAMIC_BUFFER=y

Testing

  1. single API:
  • esp_websocket_client_send_text() or bin version with size <= ws_client buffer size and check in wireshark for correct headers.
  • same but with size > ws_client buffer and carefully check for ws message headers
  1. multi message API:
  • fist check with size <= ws_client buffer
  • then check with size > ws_client buffer
  • Test the same thing with CONFIG_ESP_WS_CLIENT_ENABLE_DYNAMIC_BUFFER

Copy link
Collaborator

@david-cermak david-cermak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go ahead and create a new release, but please fix the issue later

@gabsuren gabsuren merged commit d63f831 into espressif:master Jan 19, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants