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

chore: add connect/disconnect callbacks #106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

T-vK
Copy link

@T-vK T-vK commented Nov 18, 2024

Implements #44

So you can do sth. like this:

webSerial.onConnect([]() {
  instance->webSerial.println("Hello client, I am the server!");
});

The callbacks work fine, but there seems to be bug causing messages that are sent to the client to only show up when you manually send messages from the client to the server. Not sure what that is all about. Maybe someone else would be kind enough to give it a try.

Copy link
Contributor


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@mathieucarbou
Copy link
Contributor

@T-vK : what would be a use case to now about the client connect and disconnect for WebSerial ?

@T-vK
Copy link
Author

T-vK commented Nov 18, 2024

Good question. The thought behind having the client as a parameter in the callback was that it could be useful to know which client connected, so that you could specifically send a message only to those. For example, let's say you want to print a welcome message with possible commands that are supported. You wouldn't want to send this potentially long message to every single client, every time one client connects. You would only want to send that message to that one client.

The general use-case for having any kind of connnect callback is that you want to know when the client is ready to receive messages, so that important information doesn't get lost.

@mathieucarbou
Copy link
Contributor

mathieucarbou commented Nov 18, 2024

But isn't that change the goal of the project ? WebSerial is meant to stream a Serial output (like logs) to a console, which can be views by 0..n clients. It is not meant by design to be used in a way to do one-to-one "chat" with specific clients.
It's API for example allows it to be used like a Stream interface, like Serial: to read and write.
The commande-line is used to send commands to the system.

Sending a specific message to one client console would require:

  • to know about the clients (which would require to pass the client info to the onConnect callbacl)
  • but also to have some application-level info about this client, which you cannot have right now since the frontend is agnostic to any client: there is no client name for example to say: "Hello david!"

For example, a use case example is in an application with a modem (i.e. NB-IoT / LTE-M cell chip) where Webserial would show all AT commands issued to the modem and the input would allow a user to interact with the modem by sending some AT commands.

@T-vK
Copy link
Author

T-vK commented Nov 18, 2024

I realize that this goes a bit against the current design, but how would you suggest the problem should be solved? Let's say the first thing that is printed to the WebSerial is the list of available AT commands. How would you ensure that every client (no matter when it connects) will get that important message?

@mathieucarbou
Copy link
Contributor

mathieucarbou commented Nov 18, 2024

I realize that this goes a bit against the current design, but how would you suggest the problem should be solved? Let's say the first thing that is printed to the WebSerial is the list of available AT commands. How would you ensure that every client (no matter when it connects) will get that important message?

That's the problem here: the use case you describe is a server-client com.
For that, you need WebSocket and a specific page handling a server-client com (per browser tab).

Webserial is a server-to-many-clients com (one to many).
There is no concept of client in Webserial: what is streamed to WebSerial should be equivalent to a Serial device on a ESP32 (hence its name).

This is like using Serial.println(...) : you could even say that WebSerial could be treated as a virtual Serial device on the ESP32, which is just streaming its content to the web.

Also, what is happening if you have 2 browser opened and browser one sends an AT comment ? The onMessage() callback will receive the command, but there is no way to tell which client sent it.

It's like a Serial: when receiving or sending data, there is no concept of who receives or sends it, there is no contextual data sent with the message or received with the data.

@T-vK
Copy link
Author

T-vK commented Nov 18, 2024

I get that, but let's say you have your phone connected to the WebSerial, then the server sends the list of AT commands to the clients and then you connect your laptop to the WebSerial. Now the console will be empty on your laptop while on your phone you can see the list of AT commands.
In my opinion there should be a mechanism that ensures all clients display the same data. If we don't have that we need to know when a new client connects so that we can send this message again when it happens.
The use-case simply being:
Whenever a user connects to the WebSerial, they should see the messages that have been printed before (e. g. a list of available AT commands).

@mathieucarbou
Copy link
Contributor

mathieucarbou commented Nov 18, 2024

In my opinion there should be a mechanism that ensures all clients display the same data

This requires that the esp32 is buffering all the data that is sent to webserial since it's startup time.
Webserial is meant to stream, a use case is to stream logs to the console.

If you starts the esp, powered by the 5v pin, then connect the computer to the serial data (in recent boards), you see the data since the point where you connect, you don't see the previous data .

This is the same thing for webserial: it is meant to stream (read/write/ data from the esp32 to the web. Webserial implements the Print class.

So there is no concept of client, connection, disconnection and time or bufferring.

I really feel you are trying to reuse webserial UX for something it was not designed for.

For example for the use case where some messages should be printed when the console appears, the UX should be changed instead, to add a sort of help with available commands. I think this would be a valid feature that @ayushsharma82 coule implement : the ability to send a list of messages at connect time that the UI will take and display as a notice / help bubble somewhere.

@T-vK
Copy link
Author

T-vK commented Nov 18, 2024

I see. Some UI adjustments would indeed be a viable solution for that use-case.

Could you maybe also give me your opinion on a second use-case I have:
I am logging debug messages to the WebSerial and it is important that I'm going to be able to see every log entry.
The logging might start before a Wifi connection is even established. In that case I could buffer them outside of WebSerial and I could also buffer them until a client has connected. But I would need some way to know when I can push the buffer to WebSerial.

Would you say WebSerial is not meant to be used like that and that there is no interest in adding a way of making this possible?

@mathieucarbou
Copy link
Contributor

I see. Some UI adjustments would indeed be a viable solution for that use-case.

In that case I could buffer them outside of WebSerial and I could also buffer them until a client has connected. But I would need some way to know when I can push the buffer to WebSerial.

  • MCU have limited memory so if no client connects, the board will crash
  • For such queuing needs, usually a unbounded queue is NEVER used. An unbounded queue is only ever used in cases where there i a guarantee that the queue will never overflow the available memory.
  • Otherwise a bounded queue is used

http and websocket are by nature not in control of the connection but the client is: the client decides when it connects and disconnects. The only way to keep logs and not run out of memory if by storing them on disk (LittleFS), until a client connects, and if no client connects, have a sort of pruning mechanism that is removing the file and starts a new one for example.

But besides the log retention issue, you will then face a delivery guarantee issue. There is no way on earth you can have WebSerial assert that a message was delivered. The way it is designed to send a message to all connected clients, you could have 1 client connected, 1 disconnecting or undergoing a network failure. You can even have client connecting, so the message will be only sent to one client and not the other one. You can have a client that is temporary disconnected and reconnects, and you do not know that in WebSerial: a client goes away, another one is created from the browser, but this is still the same browser tab that is opened. For example if you unfocused the browser tab, it could decide to close the websocket connection after a while. So then refocusing the tab makes the websocket connection reconnects. Similar if computer goes to sleep, or screensaver triggers.

@T-vK
Copy link
Author

T-vK commented Nov 18, 2024

I understand that there is no perfect solution, but the connection (once established) has always been very stable for me. So when trying to debug something the only realistic issue I'm running into is that I send messages before the client is connected.
For my use case I could easily reserve a couple of MB for a buffer (got 8MB of PSRAM), but honestly a couple of KB would easily be enough, considerin that I'd only have a buffer for a second or so.

So my simple use-case really only needs a callback for connect and additional ones for reconnect and disconnect would be nice to have.

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.

2 participants