fprime icon indicating copy to clipboard operation
fprime copied to clipboard

Add Open Port Request on Sending Side of IpSocket

Open csmith608 opened this issue 10 months ago • 5 comments

Related Issue(s) #2553
Has Unit Tests (y/n) y
Documentation Included (y/n) n


Change Description

Update IpSocket to check socket state on sending side and open socket if necessary

Rationale

This fixes a bug encountered while using IpSocket. The server would close the TCP connection and then the client would try to send and fail because previously IpSocket would only re-open a TCP connection when it received over the socket.

Testing/Review Recommendations

In my project's version of fprime we diverged at fprime 3.0. I added implementations for the close() port in the TcpClient, TcpServer, and Udp as well as unit tests. This change also fixed the issue we were seeing with sending data, but the socket not being there. This does not include the unit tests or implementations for close() in the ByteStreamDriverModel because I wanted to at least get this part of the change in. I'll work on adding them. I also ended up commenting out line 85 of Drv/Ip/SocketReadTask.cpp because when I was running other testing without the TCP server, I was being spammed with that logger message.

Future Work

Adding implementations for close() to TcpClient, TcpServer, and Udp as well as unit tests.

(Pipeline failed on "spammed" but I stand behind my spelling, but perhaps not that comment)

csmith608 avatar Apr 16 '24 00:04 csmith608

Looks like it passed CI. We'll need to correct spelling. Your spelling is right, so we need to add the word to the expect list.

LeStarch avatar Apr 16 '24 15:04 LeStarch

The list is at: .github/actions/spelling/expect.txt. Just place "spammed" in there is mostly alphabetical order. I can help this afternoon, but the above shows you how to should you get to it sooner.

LeStarch avatar Apr 16 '24 15:04 LeStarch

Fixed the spelling issue. Will review now.

LeStarch avatar Apr 16 '24 23:04 LeStarch

@LeStarch I added a unit test to TcpClient and TcpServer to show the socket auto opening, so I think this should be okay to merge (if it passes the checks).

csmith608 avatar Jun 27 '24 22:06 csmith608

Better way of doing this:

  • Have an active component with one thread that processes events
  • Events are send events that come from outside
  • Another thread is blocking on input that sends the receive events
  • If a send event is processed and the socket isn't open, it opens
  • If on a receive the socket isn't open it opens
  • All mediated by a queue
  • One thread is from the active TCP*Impl.cpp and the receive thread

csmith608 avatar Aug 03 '24 22:08 csmith608

@LeStarch other than the RPi known issue, I think this PR is ready for review

csmith608 avatar Sep 19 '24 23:09 csmith608