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

Hardcoded buffer size in TcpClient/TcpDriver/Udp #2074

Closed
thomas-bc opened this issue Jun 8, 2023 · 8 comments
Closed

Hardcoded buffer size in TcpClient/TcpDriver/Udp #2074

thomas-bc opened this issue Jun 8, 2023 · 8 comments
Assignees
Labels
enhancement High Priority High Priority issue that needs to be resolved.

Comments

@thomas-bc
Copy link
Collaborator

F´ Version
Affected Component

Problem Description

See

return allocate_out(0, 1024);
}

We should do like UARTDriver (see here) and configure the size at configure() time so that it can be configured for each instance in the topology. Set a default size of 1024 so that it doesn't break any existing code.

@LeStarch
Copy link
Collaborator

Also in UDP and possibly UART. Should be a configuration parameter (on startup).

@LeStarch LeStarch changed the title Hardcoded buffer size in TcpClient/TcpDriver Hardcoded buffer size in TcpClient/TcpDriver/Udp Mar 27, 2024
@LeStarch LeStarch added the High Priority High Priority issue that needs to be resolved. label Mar 27, 2024
@menaman123
Copy link
Contributor

So would a fix be having an allocation parameter in the getBuffer method?

@LeStarch
Copy link
Collaborator

LeStarch commented Jun 7, 2024

@menaman123 the fix would be to add an "allocation size" to the configure method:

SocketIpStatus TcpClientComponentImpl::configure(const char* hostname,
const U16 port,
const U32 send_timeout_seconds,
const U32 send_timeout_microseconds) {
return m_socket.configure(hostname, port, send_timeout_seconds, send_timeout_microseconds);

@bdshenker
Copy link
Collaborator

This issue appears to have been fixed in commit 7FF002C by menamen123:

TcpClientComponentImpl.hpp
  PROTECTED: FwSizeType m_allocation_size;
  SocketIpStatus configure(const char* hostname,
                             const U16 port,
                             const U32 send_timeout_seconds = SOCKET_SEND_TIMEOUT_SECONDS,
                             const U32 send_timeout_microseconds = SOCKET_SEND_TIMEOUT_MICROSECONDS,
                             FwSizeType buffer_size = 1024);
TcpClientComponentImpl.cpp
  Fw::Buffer TcpClientComponentImpl::getBuffer() {
      return allocate_out(0, static_cast<U32>(m_allocation_size));
  }

If I am correct then this ticket can now be closed per GIT ticket # 2759.

@LeStarch
Copy link
Collaborator

@bdshenker the above ticket did resolve the problem in TcpClient, but not TcpServer nor Udp.

return allocate_out(0, 1024);

@bdshenker
Copy link
Collaborator

bdshenker commented Oct 29, 2024 via email

bdshenker pushed a commit to bdshenker/fprime that referenced this issue Oct 29, 2024
@bdshenker
Copy link
Collaborator

@LeStarch I completed Git#2074 and reran regression suite locally.
However, when making pull request, one of the integration tests failed.
Please advise how I should proceed.

LeStarch added a commit that referenced this issue Nov 6, 2024
* #2074 Parameterize buffer size in UDP and TCP Server.

* Fixing m_allocate pssing

* Fixing cast

---------

Co-authored-by: bdshenker <boris.shenker@jpl.nasa.gov>
Co-authored-by: M Starch <LeStarch@googlemail.com>
@LeStarch
Copy link
Collaborator

Closing as complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement High Priority High Priority issue that needs to be resolved.
Projects
None yet
Development

No branches or pull requests

4 participants