Skip to content

Commit

Permalink
Fix for too long G-code replies (#1073)
Browse files Browse the repository at this point in the history
Added extra parameter to OutputBuffer::Allocate to specify if the buffer
to be allocated may be taken from the output buffer reserve. This is
always true except if appending to an existing output buffer chain or
when the request may be repeated without downsides.
Also reduced the number of reserved output buffers to 2 on Duet 2 series
  • Loading branch information
chrishamm committed Feb 4, 2025
1 parent bd4715a commit a3498d3
Show file tree
Hide file tree
Showing 9 changed files with 17 additions and 16 deletions.
2 changes: 1 addition & 1 deletion src/Config/Configuration.h
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ constexpr size_t MinimumBuffersForObjectModel = 20; // Minimum number of free b
#elif SAM4E || SAM4S
constexpr size_t OUTPUT_BUFFER_SIZE = 256; // How many bytes does each OutputBuffer hold?
constexpr size_t OUTPUT_BUFFER_COUNT = 26; // How many OutputBuffer instances do we have?
constexpr size_t RESERVED_OUTPUT_BUFFERS = 4; // Number of reserved output buffers after long responses, enough to hold a status response
constexpr size_t RESERVED_OUTPUT_BUFFERS = 2; // Number of reserved output buffers after long responses, enough to hold a status response
constexpr size_t MinimumBuffersForObjectModel = 20; // Minimum number of free buffers we want before we start assembling a request for the object model
#else
# error Unsupported processor
Expand Down
4 changes: 2 additions & 2 deletions src/GCodes/GCodes2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -994,7 +994,7 @@ bool GCodes::HandleMcode(GCodeBuffer& gb, const StringRef& reply) THROWS(GCodeEx
}
else
{
if (!OutputBuffer::Allocate(outBuf))
if (!OutputBuffer::Allocate(outBuf, false))
{
return false; // cannot allocate an output buffer, try again later
}
Expand Down Expand Up @@ -3276,7 +3276,7 @@ bool GCodes::HandleMcode(GCodeBuffer& gb, const StringRef& reply) THROWS(GCodeEx
}

// Need a valid output buffer to continue
if (!OutputBuffer::Allocate(outBuf))
if (!OutputBuffer::Allocate(outBuf, false))
{
// No buffer available, try again later
return false;
Expand Down
2 changes: 1 addition & 1 deletion src/GCodes/GCodes5.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ GCodeResult GCodes::HandleM486(GCodeBuffer &gb, const StringRef &reply, OutputBu
if (!seen)
{
// List objects on build plate
if (!OutputBuffer::Allocate(buf))
if (!OutputBuffer::Allocate(buf, false))
{
return GCodeResult::notFinished;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Movement/Move.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3071,7 +3071,7 @@ GCodeResult Move::ConfigureStallDetection(GCodeBuffer& gb, const StringRef& repl
drivers = LocalDriversBitmap::MakeLowestNBits(numSmartDrivers);
}

if (!OutputBuffer::Allocate(buf))
if (!OutputBuffer::Allocate(buf, false))
{
return GCodeResult::notFinished;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Networking/HttpResponder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1523,7 +1523,7 @@ void HttpResponder::Diagnostics(MessageType mt) const noexcept
OutputBuffer *_ecv_null buffer = gcodeReply.GetLastItem();
if (buffer == nullptr || buffer->IsReferenced())
{
if (!OutputBuffer::Allocate(buffer))
if (!OutputBuffer::Allocate(buffer, false))
{
// No more space available, stop here
return;
Expand Down
11 changes: 6 additions & 5 deletions src/Platform/OutputMemory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ size_t OutputBuffer::cat(const char c) noexcept
{
// No - allocate a new item and copy the data
OutputBuffer *nextBuffer;
if (!Allocate(nextBuffer))
if (!Allocate(nextBuffer, false))
{
// We cannot store any more data
hadOverflow = true;
Expand Down Expand Up @@ -218,9 +218,10 @@ size_t OutputBuffer::cat(const char *_ecv_array src, size_t len) noexcept
{
if (last->dataLength == OUTPUT_BUFFER_SIZE)
{
// The last buffer is full
// Save at least some output buffers in case this buffer chain has to be sent via network.
// If we don't do this, the network responder may be unable to allocate enough for the header
OutputBuffer *nextBuffer;
if (!Allocate(nextBuffer))
if (!Allocate(nextBuffer, false))
{
// We cannot store any more data, stop here
hadOverflow = true;
Expand Down Expand Up @@ -359,13 +360,13 @@ bool OutputBuffer::WriteToFile(FileData& f) const noexcept
}

// Allocates an output buffer instance which can be used for (large) string outputs. This must be thread safe. Not safe to call from interrupts!
/*static*/ bool OutputBuffer::Allocate(OutputBuffer *_ecv_null &buf) noexcept
/*static*/ bool OutputBuffer::Allocate(OutputBuffer *_ecv_null &buf, bool allowReserved) noexcept
{
{
TaskCriticalSectionLocker lock;

buf = freeOutputBuffers;
if (buf != nullptr)
if (buf != nullptr && (allowReserved || OUTPUT_BUFFER_COUNT - usedOutputBuffers > RESERVED_OUTPUT_BUFFERS))
{
freeOutputBuffers = buf->next;
usedOutputBuffers++;
Expand Down
4 changes: 2 additions & 2 deletions src/Platform/OutputMemory.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ class OutputBuffer
// Initialise the output buffers manager
static void Init() noexcept;

// Allocate an unused OutputBuffer instance. Returns true on success or false if no instance could be allocated.
static bool Allocate(OutputBuffer *_ecv_null &buf) noexcept;
// Allocate an unused OutputBuffer instance, optionally from the reserve. Returns true on success or false if no instance could be allocated.
static bool Allocate(OutputBuffer *_ecv_null &buf, bool allowReserved = true) noexcept;

// Get the number of bytes left for allocation. If writingBuffer is not NULL, this returns the number of free bytes for
// continuous writes, i.e. for writes that need to allocate an extra OutputBuffer instance to finish the message.
Expand Down
4 changes: 2 additions & 2 deletions src/Platform/RepRap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2145,7 +2145,7 @@ GCodeResult RepRap::GetFileInfoResponse(c_string _ecv_null filename, OutputBuffe
return GCodeResult::notFinished;
}

if (!OutputBuffer::Allocate(response))
if (!OutputBuffer::Allocate(response, false))
{
return GCodeResult::notFinished;
}
Expand Down Expand Up @@ -2282,7 +2282,7 @@ void RepRap::AppendStringArray(OutputBuffer *buf, c_string _ecv_null name, size_
OutputBuffer *RepRap::GetModelResponse(const GCodeBuffer *_ecv_null gb, c_string _ecv_null key, c_string _ecv_null flags) const THROWS(GCodeException)
{
OutputBuffer *outBuf;
if (OutputBuffer::Allocate(outBuf))
if (OutputBuffer::Allocate(outBuf, false))
{
if (key == nullptr) { key = ""; }
if (flags == nullptr) { flags = ""; }
Expand Down
2 changes: 1 addition & 1 deletion src/Tools/Tool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -927,7 +927,7 @@ GCodeResult Tool::SetFirmwareRetraction(GCodeBuffer &gb, const StringRef &reply,
else
{
// Use an output buffer because M207 can report on all tools
if (outBuf == nullptr && !OutputBuffer::Allocate(outBuf))
if (outBuf == nullptr && !OutputBuffer::Allocate(outBuf, false))
{
return GCodeResult::notFinished;
}
Expand Down

0 comments on commit a3498d3

Please sign in to comment.