feat(websocket): allow using external TCP transport handle (IDFGH-12825)

[huming2207]

Hi there,

This PR was originally done last year (May 2023), but I forgot to submit it... 😅

Anyway, this PR is for adding support to let WebSocket client to take external handles, similar to my another PR for MQTT client, see:

• https://github.com/espressif/esp-mqtt/blob/master/include/mqtt_client.h#L339
• mqtt: allow using external TCP transport handles (IDFGH-10290) esp-mqtt#260

This is very useful for scenarios where:

1. The application requires proxies to access network (via my esp_tcp_transport_addons hack for HTTP proxy access, or a SOCK4 proxy via the transport_socks_proxy.c in tcp_transport component) ;
2. The application somehow requires external TCP/IP stack on a modem via serial port, see this example

Currently for my company, we are using my HTTP proxy library mentioned above to achieve HTTP proxy access feature. But we are currently using my modified esp-websocket-client and we wish to merge this into mainline soon instead.

Please provide suggestion if there's any and I will modify accordingly. Thanks.

Regards,
Jackson

[CLAassistant]

All committers have signed the CLA.

[huming2207]

cc @suren-gabrielyan-espressif and @david-cermak

[huming2207]

Any updates on this...?

[huming2207]

any updates...? @gabsuren

[gabsuren]

@huming2207 Thank you for your contribution, and we apologize for the delayed response. We are looking into it and will update you soon.

[huming2207]

@gabsuren any updates? it's been a month...

[euripedesrocha]

@huming2207 this is in our pipeline for evaluation as Suren mentioned.
I looked into the code changes and in the current state we'll need some changes before accepting it.
You are passing only the parent transport. We should have the actual transport that will be used since it is more flexible. Some users might want to have their own implementation of the websocket transport level and if only the parent customization is needed it is easy to compose before sending to websocket client.

Also we need you to install the pre-commit hook as explained in our contributing guide and fix the issues.

[huming2207]

Hi @gabsuren @euripedesrocha

You are passing only the parent transport. We should have the actual transport that will be used since it is more flexible.

Just to clarify, it seems like esp-mqtt did the same like I did, except it calls esp_transport_destroy() at deinit. Do you mean I should add the esp_transport_destroy()? Or something else?

Also we need you to install the pre-commit hook as explained in our contributing guide and fix the issues.

Sure, I will do that.

Regards,
Jackson

[huming2207]

Also, just to be clear, this PR is solely for adding parent transport support, so that we are able to connect our stuff over a HTTP proxy provided by our partner companies. We don't need to, and better off not to touch the WS transport itself. Because that's not our use case at all.

[euripedesrocha]

Just to clarify, it seems like esp-mqtt did the same like I did, except it calls esp_transport_destroy() at deinit. Do you mean > I should add the esp_transport_destroy()? Or something else?

Not exactly the same way. MQTT client uses the external transport as is, not the base parent, as your proposal. In MQTT client, we don't even construct the transport_list

About the cleanup, we want to keep the transport valid as long as the client is in use and simplify user task on tracking the lifetime of the transport, but it is not what I'm suggesting here. We might even go back on that decision for mqtt client.

Also, just to be clear, this PR is solely for adding parent transport support, so that we are able to connect our stuff over a HTTP proxy provided by our partner companies. We don't need to, and better off not to touch the WS transport itself. Because that's not our use case at all.

I understand that this is your use case, and to be honest, probably the most common use case but, we need to provide the feature for the general case. And given that the workaround of constructing the websocket transport is just 1 or 2 extra lines it is better to do it that way.

[huming2207]

Not exactly the same way. MQTT client uses the external transport as is, not the base parent, as your proposal. In MQTT client, we don't even construct the transport_list

About the cleanup, we want to keep the transport valid as long as the client is in use and simplify user task on tracking the lifetime of the transport, but it is not what I'm suggesting here. We might even go back on that decision for mqtt client.

Ah crap, yes I misread the code.

I will modify the code accordingly,

[huming2207]

Hi @gabsuren @euripedesrocha

I've committed the changes. Please re-review. Thanks.

[huming2207]

Thanks for the approval, but it seems like the CI is failing because of lacking heaeders in the logging library? Should I provide a workaround or wait till you fix it?

[gabsuren]

@huming2207 the failure is unrelated to your PR. I will fix it separately. Thank you for contribution!