Skip to content

Add TCP Listen and Accept#196

Open
MarkusLassila wants to merge 2 commits intonrfconnect:mainfrom
MarkusLassila:tcp-listen-accept
Open

Add TCP Listen and Accept#196
MarkusLassila wants to merge 2 commits intonrfconnect:mainfrom
MarkusLassila:tcp-listen-accept

Conversation

@MarkusLassila
Copy link
Contributor

@MarkusLassila MarkusLassila commented Feb 27, 2026

Add AT#XLISTEN and AT#XACCEPT for TCP server functionality.

Jira: SM-240

@MarkusLassila MarkusLassila changed the title Tcp listen accept Add TCP Listen and Accept Feb 27, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds TCP server support to the Serial Modem AT socket layer by documenting and implementing AT#XLISTEN / AT#XACCEPT, plus extending AT#XSOCKET? output to include remote peer info for accepted sockets.

Changes:

  • Extend AT#XSOCKET? read response to optionally include accepted peer address/port.
  • Add AT#XLISTEN (listen) and AT#XACCEPT (accept) AT commands in implementation and docs.
  • Update unit tests and test harness wiring (AT wrapper routing + kernel alloc stubs) to cover listen/accept behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
doc/app/at_socket.rst Documents #XLISTEN / #XACCEPT and extends #XSOCKET? response format with peer info.
app/src/sm_at_socket.c Implements listen/accept, tracks bound port + listen state, and stores peer address for accepted sockets.
app/tests/at_socket/src/nrf_modem_at_wrapper.c Routes AT#XLISTEN / AT#XACCEPT to custom-command wrappers in tests.
app/tests/at_socket/src/test_at_socket.c Adds tests for listen/accept and updates socket-count assumptions.
app/tests/at_socket/CMakeLists.txt Links new kernel stubs into the at_socket test target.
app/tests/stubs/kernel_stubs.c Provides k_malloc/k_free stubs for the unit-test environment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{
int ret;

if (sock->type != NRF_SOCK_STREAM || sock->local_port == 0 ||
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

do_listen() rejects listening unless sock->local_port != 0, but #XBIND explicitly documents the port range as 0–65535. If a socket is bound with port 0 (ephemeral), it is still bound but will be treated as “not bound” here. Consider tracking a separate “is_bound” flag (or querying getsockname) instead of using local_port == 0 as a proxy.

Suggested change
if (sock->type != NRF_SOCK_STREAM || sock->local_port == 0 ||
if (sock->type != NRF_SOCK_STREAM ||

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks valid, right? 0 is valid port number.
Add a unit test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on how the modem deals with bind to 0. But in any case either this or the port range in bind is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the way this works is that the 0 assigns an ephemeral port, which we cannot currently be aware as we lack the getsockname(). So we are right to deny this here as without being bound to specific port we would not know which port the listen went to, which makes it useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note in the bind about 0 being a special value.

Copy link
Collaborator

@trantanen trantanen left a comment

Choose a reason for hiding this comment

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

Going through doc and parts of code.

{
int ret;

if (sock->type != NRF_SOCK_STREAM || sock->local_port == 0 ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks valid, right? 0 is valid port number.
Add a unit test for this.


The listening socket is set to non-blocking mode when ``#XLISTEN`` command is executed.
If there are no incoming connections when the ``#XACCEPT`` command is executed, it will return an error.
The ``#XAPOLL`` command can be used to receive asynchronous notifications (``POLLIN``) for incoming connections on the listening socket.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is POLLIN something we've defined as our proprietary solution?
Should this be mentioned in XAPOLL description of POLLIN too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the poll() returns POLLIN if there is a new connection.

new_sock->fd = ret;
new_sock->family = remote.sa_family;
new_sock->type = NRF_SOCK_STREAM;
new_sock->role = AT_SOCKET_ROLE_SERVER;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the new socket anymore a server?

Where do we use this role information?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nowhere. This the desired TLS role on the socket. Modem takes this parameter but doesn't really support TLS server. We should have removed this altogether before 1.0 but we didn't so it will hang in there for nothing...

But yes, this should probably be client socket here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The I recommend that we use this "role" to mark sockets that are listening.

So we can drop the sock->local_port and sock->listen values as well.

It is not relevant to know in a boolean if you already called nrf_listen() or not, because nrf_accept() will fail if you did not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that we have no means of getting the ephemeral port from the modem, I think that it still makes sense to track whether we have bound to non-ephemeral port.

It is better to give an error than allow the user to do listen() to a port that they have no hope of getting.

Copy link
Contributor Author

@MarkusLassila MarkusLassila Mar 10, 2026

Choose a reason for hiding this comment

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

Actually, the role makes 0 sense as it is. No need to waste uint16_t for it when a bit field is enough.

Edit: That is, if we can just ignore the server parameter given in AT#XSOCKET and not return the same value when AT#XSOCKET? is done.

@trantanen: Does that constitute as breaking AT-interface?

-> Discussed offline, it does, so we keep it and return it in AT#XSOCKET?. Parameter does not have any effect internally.

Copy link
Contributor Author

@MarkusLassila MarkusLassila Mar 10, 2026

Choose a reason for hiding this comment

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

Also, AT#XLISTEN? currently returns the listen sockets and the ports that they are bound. Keep or remove?

-> Discussed offline: Keep.

@MarkusLassila MarkusLassila force-pushed the tcp-listen-accept branch 2 times, most recently from 207ba63 to ac3bea8 Compare March 10, 2026 09:16
Add AT#XLISTEN and AT#XACCEPT for TCP server functionality.

Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>
Basic unit tests for AT#XLISTEN and AT#XACCEPT.

Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>
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.

4 participants