MMU Serial Port issue. Unsafe write() function

[RogerInHawaii]

I'm looking at the serial port function for the MMU in the latest version of the MK3 Open Source code, and I think there's a coding problem with the write() function of the MMU serial port on my PRUSA MK3S., which is this:

void MMU2Serial::write(const uint8_t *buffer, size_t size)
{
while(size--){
fputc(*buffer, uart2io);
++buffer;
}
}

In the related uart2.c file it defines uart2io_ibuf in this way:

uint8_t uart2_ibuf[20] = {0, 0};

It doesn't look right to me. In particular the buffer is defined as 20 bytes but the write function writes into it with whatever is passed as the size parameter, meaning it could easily overwrite the buffer. Now, maybe there's some protocol or rules that the PRUSA programmers are supposed to follow (e.g. "No text more than 20 characters long to be used in calls to the write() function), but shouldn't the code itself do something if the text is too long? It just doesn't look like a very safe function.

In fact I have been making some modifications to the firmware and did indeed inadvertently write longer-than-20-characters to the write() function and the program crashes with an "FW crash detected" error.

I asked ChatGPT about it and it responded with this, which verified my concerns, but raised additional issues:

1. Size of uart2_ibuf:

The uart2_ibuf buffer is defined as uint8_t uart2_ibuf[20], which is only 20 bytes in size. If write tries to send more than 20 bytes at once, it could lead to a buffer overflow when accessing uart2io.
Confirm whether uart2io is properly configured to handle more data or whether fputc ensures safe handling of larger input.

2. Thread Safety:

If the write function is called from multiple threads or interrupts, and fputc or uart2io isn't thread-safe, race conditions could occur.

3. Blocking or Non-Blocking Behavior:

If fputc does not block until the byte is transmitted, it could overwrite previous data or lose some of it. Ensure fputc properly waits for the UART transmission buffer to be ready.

4. size Handling:

The function decrements size until it reaches 0, but there's no validation of the size against the capabilities of fputc or uart2io. If size is very large, performance could degrade or memory corruption might occur.Potential Problems:

[RogerInHawaii]

SERIOUSLY? No one cares about this?

[gudnimg]

Hi @RogerInHawaii I don't see what the problem is. The function MMU2Serial::write calls uart2_putchar() through fputc which sends one byte at time regardless of the buffer size. In fact uart2_ibuf isn't used at all here. uart2_ibuf is only used to store received data from the MMU.

int uart2_putchar(char c, _UNUSED FILE *stream)
{
	while (!uart2_txready);
	UDR2 = c; // transmit byte
//	while (!uart2_txcomplete); // wait until byte sent
//	UCSR2A |= (1 << TXC2); // delete TXCflag
	return 0;
}

Can you share how you are modifying the code? I'm not convinced sending more than 20 bytes to this function will cause a FW crash.

[RogerInHawaii]

I added a simple function to the MMU2 namespace:

void MMU2Serial::print(String TextToPrint)
{
char TheCharacter;
uint8_t TheCharacterInABuffer;
for (unsigned int i = 0; i < TextToPrint.length(); i++)
{
TheCharacter = TextToPrint.charAt(i);
TheCharacterInABuffer = (uint8_t)TheCharacter;
write(&TheCharacterInABuffer, 1);
}
}

And call it from a number of places in the Firmware.
Yes, it does seem like it never makes use of the 20-byte buffer when sending text out.

However, I was getting "FW crash detected", numerous times.

I did indeed have a number of text strings that I was sending via my MU2Serial::print() function which were greater than 20 bytes. It SEEMED to me that maybe that buffer size was the problem. So I sent a sequence of strings via my MMU2Serial::print() function, starting with just one character and working its way up to more-than-20. Everything was going fine but when it hit the string that was longer than 20 characters up popped the "FW crash detected" message again.

So I went through all my code and shortened any string that was being sent via my MMU2Serial::print() function to being less than 20 characters. And having done that, no more "FW crash detected" errors.

So somehow there does seem to be a 20-character limit, somehow.

You mention that "MMU2Serial::write calls uart2_putchar() through fputc which sends one byte at time regardless of the buffer size." And it seems that uart2_putchar WAITS for the uart2_txready. Does that mean that it waits for any previously sent character to be RECEIVED on the receiving end?

In that uart2_putchar() function you reference:

int uart2_putchar(char c, _UNUSED FILE *stream)
{
while (!uart2_txready);
UDR2 = c; // transmit byte
// while (!uart2_txcomplete); // wait until byte sent
// UCSR2A |= (1 << TXC2); // delete TXCflag
return 0;
}

there is also a commented-out couple of instructions, and it references uart2_txcomplete. Is there a difference between uart2_txready and uart2_txcomplete, such that uart2_txready doesn't really wait for the text to have been sent? Could that be the cause of the "FW crash detected" messages I'd been getting whenever one of the messages I was sending was greater than 20 characters?

The testing that I did sure did seem to indicate that there's a 20-character limit SOMEWHERE.

[gudnimg]

One thing I see immediately is String. I have had very bad experience using it in some older Arduino projects. I strongly recommend not using it. It uses the heap if I recall correctly and can easily cause unpredictable behaviour.

Better to use char pointers.

I havent read you entire response yet. I’ll take another look after work today :)

[gudnimg]

String is specific to the Arduino platform. Its better to use native C strings. Mainly for reliability IMO

[gudnimg]

You mention that "MMU2Serial::write calls uart2_putchar() through fputc which sends one byte at time regardless of the buffer size." And it seems that uart2_putchar WAITS for the uart2_txready. Does that mean that it waits for any previously sent character to be RECEIVED on the receiving end?

No there is no handshake in UART (no flow control) between the printer and MMU.
The ATmega2560 datasheet explains how this work better than I could describe:

[gudnimg]

Is there a difference between uart2_txready and uart2_txcomplete, such that uart2_txready doesn't really wait for the text to have been sent?

Yes, as the image I shared above describes it in detail. When sending >20 bytes of data, we don't really care when the transmission is complete. All we need to know is when the MCU is ready to buffer more data in its transmission buffer. The MCU then automatically takes care of transmitting the data for us. In other applications it could be useful to know exactly when each UART transmission is complete, but for this application (send data to MMU) it would only slow things down with no benefits I think.

From the datasheet:

The TXCn Flag can be used to check that the Transmitter has completed all transfers, and the RXC Flag can be used to check that there are no unread data in the receive buffer. Note that the TXCn Flag must be cleared before each transmission (before UDRn is written) if it is used for this purpose.

This flag isn't always useful and really depends on the use case. Since the transmit buffer is only 1 byte large, you would have to check this TXCn flag for each byte and also clear it. This slows things down.

Could that be the cause of the "FW crash detected" messages I'd been getting whenever one of the messages I was sending was greater than 20 characters?

No I don't see how (at least its not obvious to me). It possible if it takes a long time to transmit that the hardware watchdog is tripping. That will cause a FW crash message. But it takes a long time for that happen, I don't remember exactly how long but at least a few seconds.

[gudnimg]

I am very suspicious of this function you shared:

void MMU2Serial::print(String TextToPrint)
{
    char TheCharacter;
    uint8_t TheCharacterInABuffer;
    for (unsigned int i = 0; i < TextToPrint.length(); i++)
    {
        TheCharacter = TextToPrint.charAt(i);
        TheCharacterInABuffer = (uint8_t)TheCharacter;
        write(&TheCharacterInABuffer, 1);
    }
}

One possibility for what is causing the FW crash is you are running out of stack memory.

Does the crash happen if you comment out the write line?

void MMU2Serial::print(String TextToPrint)
{
    char TheCharacter;
    uint8_t TheCharacterInABuffer;
    for (unsigned int i = 0; i < TextToPrint.length(); i++)
    {
        TheCharacter = TextToPrint.charAt(i);
        TheCharacterInABuffer = (uint8_t)TheCharacter;
        //write(&TheCharacterInABuffer, 1);
    }
}

[RogerInHawaii]

Thank you so much for taking the time to look through this issue.
I'll get back to you real quick (couple of days) with some additional thoughts.
ANd I'll try what you suggested about leaving out the actual call to the write() function to see if it still crashes, which would point more to my use of Strig objects than the write() function.

[gudnimg]

FYI: We just merged some minor cleanup on the Uart code #4816

Hopefully less code “noise” :)