-
Notifications
You must be signed in to change notification settings - Fork 30
QNEthernet higher latency #38
Comments
Hi @5b4wn Thanks for your bug report as well as good solution, which I'll add to the new release. Could you also try to use the QNEthernet's new and better It's even better you make a new PR for this enhancement.
I'm waiting for your new tests and reply. Regards, |
Hi @5b4wn Just tested the modifications using Can you test using your code and use-case and be sure everything is OK and better. |
Hi
yes that seems to work OK. Many thanks for the speedy response.
Of note is that I find QNEthernet (since @ssilverman
<https://github.com/ssilverman> suggestion) more reliable than Native for
multiple sockets on the teensy 4.1.
Completely off topic, is websockets2 potentially capable of supporting wss
as a server? (at least for teensy 4.1)?
Thanks again for your help
BW
@5b4wn
…On Sat, 26 Feb 2022 at 21:54, Khoi Hoang ***@***.***> wrote:
Hi @5b4wn <https://github.com/5b4wn>
Just tested the modifications using client.writeFully() and working OK.
Can you test using your code and use-case and be sure everything is OK and
better.
—
Reply to this email directly, view it on GitHub
<#38 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEE4JM7JQ4UQNZWVM3GTMQ3U5FDXZANCNFSM5PNRLKGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Marios
|
Good news. Will you make a PR or I'll do it. Any way, your contribution will be noted.
I agree it's more reliable for many more applications, and have spent more time on that new and better QNEthernet library.
Currently ws server is OK, not wss. I'm also very reluctant, even against to do so, as wss server requires lot more power to fulfill the TLS wss encryption / decryption, and have to check / update all the certs frequently to ensure safety. I suggest you to use |
Hi @5b4wn The new WebSockets2_Generic releases v1.10.1 has just been published. Your contribution is noted in Contributions and Thanks I'm looking forward to receiving more of your contributions (bug report, enhancement, PR, etc.) Best Regards, Release v1.10.1
|
In my view, there should be an option to not send all data immediately. It’s not something that’s always desired. I would add a flush() function to It’s not the case that QNEthernet has high latency, it’s that there’s no option in the websockets library to flush data that’s been accumulated. It’s true that some libraries always send data immediately, but I wanted to provide both options: one option to accumulate and another to send immediately. Another option (and I haven’t played with it much) is to disable Nagle’s algorithm with a call to EthernetClient::setNoDelay(true). That might remove the need for flush(). |
Hi @ssilverman OR @khoih-prog is there way for another method such as
or
|
Thanks for trying out setNoDelay(true). My opinion is that, if it’s decided to provide an optional flush-after-write, it’s better to have two functions rather than one with a Boolean because Booleans, when reading the code, don’t have an obvious meaning. Some options:
etc. But the word “send” does seem to imply that the data actually gets sent, so if that was the real intent behind this function, maybe option 3 would better fit expectations of the names. I also like your sendImmediately() suggestion (sendNow() would be shorter, but that’s a personal preference). That’s similar to options 2 and 3. |
In the normal and independent library, I agree it's better to create new functions with correct names. But this enhancement, IMO, has to follow the That's why I agree with @5b4wn to use the same The best way to do this, IMO, is inside QNEthernet library, with the Anyway, I prefer to keep the same as it is now, until there are some real issue posted by the users that the current implementation is buggy. |
There isn’t a “standard” way, nor is one way “good” and the other “bad”. Nor does one way provide “better performance.” It depends what you’re optimizing for. Some libraries I use (eg. implementations of Arduino’s own This isn’t my library so I don’t really have a say, but my opinion is that the library should support both approaches, and it sounds like you’re doing that with an additional Boolean One reason to flush for every call is it avoids delays. One reason to not flush for every call is that it reduces the network traffic, which is desirable in other cases. It’s not appropriate for QNEthernet to flush on every write() call as that’s not the intent of the Bottom line: I’m glad you’re using my library and I’m glad you’re using it in a manner that works for you. We all want to make great software here. Side point: QNEthernet does send buffered traffic if it hasn’t been flushed after a short time. That time depends on how the underlying lwIP stack is configured. |
First, many thanks for an awesome library!
I would like to report a bug and a possible solution but not sure if this introduces any other issues but this seems to have improved the problem for me..
Describe the bug
When using QNEthernet on a teensy 4.1 websocket server, there seems to be 5-6x latency compared to Native ethernet when sending data
using client.send
Reading https://github.com/ssilverman/QNEthernet#write-immediacy
It seems that the client.write waits for a timeout or enough data to send that adds about 150ms delay for each call
This change in Teensy41_QNEthernet_tcp.hpp seems to correct the problem
BW
Marios
The text was updated successfully, but these errors were encountered: