Skip to content

[MessageControl] Make sure no messages are missed via the UDP output #362

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.

Sign up for GitHub

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

Merged
pwielders merged 7 commits into from
Sep 20, 2025
Merged

[MessageControl] Make sure no messages are missed via the UDP output #362

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6ed437d
Use a queue of ProxyType<string> instead of a buffer for UDP output t…
VeithMetro Aug 5, 2025
57119a0
Remove ProxyType entirely, as the lifetime of the string is managed b…
VeithMetro Aug 6, 2025
bd1e613
Make sure to use std::move
VeithMetro Aug 6, 2025
df1c6e2
Indeed no need for this std::move..
VeithMetro Aug 6, 2025
219f0de
Make the string const
VeithMetro Aug 6, 2025
8a0c03a
Optimize with std::move to avoid all possible copies
VeithMetro Aug 21, 2025
0877d7f
Optimize Text::Convert to create less copies
VeithMetro Aug 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions MessageControl/MessageOutput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,8 @@ namespace Publishers {
//UDPOutput
UDPOutput::Channel::Channel(const Core::NodeId& nodeId)
: Core::SocketDatagram(false, nodeId.Origin(), nodeId, Messaging::MessageUnit::Instance().DataSize(), 0)
, _loaded(0)
, _queue()
{
::memset(_sendBuffer, 0, sizeof(_sendBuffer));
}
UDPOutput::Channel::~Channel()
{
Expand All @@ -142,13 +141,21 @@ namespace Publishers {

uint16_t UDPOutput::Channel::SendData(uint8_t* dataFrame, const uint16_t maxSendSize)
{
uint16_t actualByteCount = 0;

_adminLock.Lock();

uint16_t actualByteCount = (_loaded > maxSendSize ? maxSendSize : _loaded);
memcpy(dataFrame, _sendBuffer, actualByteCount);
_loaded = 0;
if (_queue.empty() == true) {
Comment thread
VeithMetro marked this conversation as resolved.
Comment thread
VeithMetro marked this conversation as resolved.
Comment thread
VeithMetro marked this conversation as resolved.
Comment thread
VeithMetro marked this conversation as resolved.
_adminLock.Unlock();
}
else {
string msg = _queue.front();
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

Consider using 'const string& msg = _queue.front();' to avoid unnecessary string copy, since the message is only used for reading.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@VeithMetro VeithMetro Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good idea to make it const, but it cannot be string&, since it is removed from the queue in the next line 😄

Copy link
Copy Markdown
Contributor

@pwielders pwielders Aug 21, 2025

Choose a reason for hiding this comment

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

Uhhhh, I think CoPilot does have a point here. Since I recently learned that the std::string is nolonger refcounted, and I know that this is a bit of a length string (it is typically longer than 32 bytes) I suggest to make a move iterator. As I was not sure, I did test it in godbolt. Here is the example code (change queue -> dequeu)

#include <iomanip>
#include <iostream>
#include <iterator>
#include <list>
#include <string>
#include <deque>
 
int main()
{
    std::deque<std::string> s;

    s.push_back("een hele lange string sie absoluut niet op de stack in het object kan.......");
    s.push_back("twee");
    s.push_back("drie");

    auto index (std::make_move_iterator(s.begin()));

    printf ("Address in queue: %p\n", s[0].c_str());

    std::string frontElement (*index);
    s.pop_front();

    for(auto element : s) {
        printf("Array holds: %s\n", element.c_str());
    }

    printf("string is now (in a seperate eobject: %p\n", frontElement.c_str());
}

_queue.pop();
_adminLock.Unlock();
Comment thread
VeithMetro marked this conversation as resolved.

_adminLock.Unlock();
actualByteCount = std::min<uint16_t>(msg.size(), maxSendSize);
memcpy(dataFrame, msg.c_str(), actualByteCount);
Comment thread
VeithMetro marked this conversation as resolved.
Comment thread
VeithMetro marked this conversation as resolved.
}

Comment thread
VeithMetro marked this conversation as resolved.
return (actualByteCount);
}
Expand All @@ -166,14 +173,7 @@ namespace Publishers {
{
_adminLock.Lock();

ASSERT((_loaded + text.length() + 1) < sizeof(_sendBuffer));

if ((_loaded + text.length() + 1) < sizeof(_sendBuffer)) {
Core::FrameType<0> frame(_sendBuffer + _loaded, sizeof(_sendBuffer) - _loaded, sizeof(_sendBuffer) - _loaded);
Core::FrameType<0>::Writer frameWriter(frame, 0);
frameWriter.NullTerminatedText(text);
_loaded += frameWriter.Offset();
}
_queue.push(std::move(text));
Comment thread
VeithMetro marked this conversation as resolved.
Outdated

_adminLock.Unlock();

Expand Down
5 changes: 1 addition & 4 deletions MessageControl/MessageOutput.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,6 @@ namespace Publishers {

class UDPOutput : public IPublish {
private:
static constexpr uint16_t UDPBufferSize = 4 * 1024;

class Channel : public Core::SocketDatagram {
public:
Channel() = delete;
Expand All @@ -364,8 +362,7 @@ namespace Publishers {
uint16_t ReceiveData(uint8_t*, const uint16_t) override;
void StateChange() override;

uint8_t _sendBuffer[UDPBufferSize];
uint16_t _loaded;
std::queue<string> _queue;
Core::CriticalSection _adminLock;
};

Expand Down
Loading