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

Keeps track of the number of connected clients #447

Merged
merged 3 commits into from
Oct 17, 2020

Conversation

balazsracz
Copy link
Collaborator

in the GridConnect TCP hub.

@balazsracz balazsracz requested a review from bakerstu October 17, 2020 12:19
@atanisoft
Copy link
Collaborator

Perhaps this can be extended slightly further to actively reject new clients when the current client count is over a certain limit?

src/openlcb/SimpleStack.hxx Outdated Show resolved Hide resolved
src/utils/GcTcpHub.hxx Show resolved Hide resolved
@balazsracz
Copy link
Collaborator Author

Perhaps this can be extended slightly further to actively reject new clients when the current client count is over a certain limit?

I don't need that feature. Do you?

@atanisoft
Copy link
Collaborator

atanisoft commented Oct 17, 2020

I don't need that feature. Do you?

Possibly, the esp32 heap quickly reduces (and may crash) when both CAN and TCP/IP GC Hub are enabled and more than a couple clients are connected. I've considered dropping Hub support on the esp32 entirely for this reason and have already disabled it on the esp32s2 (slightly lower heap to start with).

If the GC connections were sharing the payload it may not be as much of an issue though.

@balazsracz
Copy link
Collaborator Author

I don't need that feature. Do you?

Possibly, the esp32 heap quickly reduces (and may crash) when both CAN and TCP/IP GC Hub are enabled and more than a couple clients are connected. I've considered dropping Hub support on the esp32 entirely for this reason and have already disabled it on the esp32s2 (slightly lower heap to start with).

If the GC connections were sharing the payload it may not be as much of an issue though.

Did you debug why this crashes? Or under which condition?

  • e.g. does it need a heavy load of gridconnect packets (e.g. a firmware upgrade or a CDI download?)
  • does it need the remote end to be unresponsive (i.e. having a lot of unacked TCP packets in the window).

Did you try setting SO_SNDBUF to a low value (e.g. 2 kbytes)?

@atanisoft
Copy link
Collaborator

Did you debug why this crashes?

Not fully yet, I'll be delving into it before taking any action to disable.

Did you try setting SO_SNDBUF to a low value (e.g. 2 kbytes)?

I'll need to do some testing with that, the default is 5744 bytes (4x MSS).

create_gc_port_for_can_hub(canHub_, fd, nullptr, use_select);
{
AtomicHolder h(this);
numClients_++;
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure if modern compilers are smart enough to substitute a pre-increment if they can, but a pre-increment should be less instruction cycles.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The recommendation to use pre-increment instead of post-increment comes from Scott Meyers' Effective C++ book, and specifically applies to class types that define the pre- and post-increment operator. The most typical place where we encounter such classes are STL iterators. The recommendation was true until about GCC 2.95, beyond which however GCC figured out how to optimize away the copies even from iterators if the implementation available for inlining. It also means that this recommendation has never been true for primitive types.

AtomicHolder h(this);
if (numClients_)
{
numClients_--;
Copy link
Owner

Choose a reason for hiding this comment

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

same.

@balazsracz
Copy link
Collaborator Author

Did you debug why this crashes?

Not fully yet, I'll be delving into it before taking any action to disable.

Did you try setting SO_SNDBUF to a low value (e.g. 2 kbytes)?

I'll need to do some testing with that, the default is 5744 bytes (4x MSS).

I will defer the client count limiting, because it really has to be a feature of the SocketListener instead. When we have limiting we must make sure not to call the accept() after the connection limit has been reached. So the accept thread needs to be paused and resumed, which needs API changes there.
Feel free to file a feature request.

@balazsracz balazsracz merged commit 172a4c3 into bracz-wifi-sta-count Oct 17, 2020
@balazsracz balazsracz deleted the bracz-gchub-num-clients branch October 17, 2020 15:33
@balazsracz balazsracz restored the bracz-gchub-num-clients branch October 17, 2020 16:02
balazsracz added a commit that referenced this pull request Oct 17, 2020
Keeps track of the number of connected clients in the GridConnect TCP hub.
Conversation is in #447.

* Adds an accessor to fetch the number of connected stations.

* fix whitespace

* Keeps track of the number of connected clients
in the GridConnect TCP hub.

* fix whitespace

* Fix typo
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