Data loss from saturated input

[pentacular]

Please only submit bugs for latest main or devt branches. You can check the version number in the startup messgaes and compare it to the version in grbl.h

Please answer the following questions.

What version of the firmware are you using?

version 1.3a
build 20210424

Is the problem repeatable?

Yes

Under what conditions does the bug occur?

Whenever the i/o input rate exceeds the consumption rate on clients other than SERIAL.

The issue seems to be due to how clientCheckTask in Serial.cpp operates

        while ((client = getClientChar(&data)) != CLIENT_ALL) {
            if (is_realtime_command(data)) {
                execute_realtime_command(static_cast<Cmd>(data), client);
            } else {
                    // ...
                    client_buffer[client].write(data);
                    // ...
            }
        }  // if something available

The loop continually fetches client data and then attempts to write into a per client InputBuffer without considering available capacity.

This results in data loss when there is input available but no capacity for output.

The reason for this design seems to be to allow the extraction of realtime commands.

If we look at getClientChar

static uint8_t getClientChar(uint8_t* data) {
    int res;
#ifdef REVERT_TO_ARDUINO_SERIAL
    if (client_buffer[CLIENT_SERIAL].availableforwrite() && (res = Serial.read()) != -1) {
#else
    if (client_buffer[CLIENT_SERIAL].availableforwrite() && (res = Uart0.read()) != -1) {
#endif
        *data = res;
        return CLIENT_SERIAL;
    }
    // ...
}

We can see that CLIENT_SERIAL has special handling that declines to read unless there is corresponding output capacity.

Adding the same condition to the other client reads avoids the data loss issue in practice.
It seems to me that this would be the right thing to do in general.

The consequence would be that an input that was saturated would not be able to propagate realtime commands (as for CLIENT_SERIAL today), but a non-saturated input would be able to propagate realtime commands in parallel.

Given that any saturated input will cause catestrophic data loss today, saturated input cannot be an expected operating condition.
This means that adding the output capacity check should not affect current workflows.

People expecting to operate with saturated input (as in a file upload over a socket) would need to bear in mind that realtime commands would be delayed, and should either add support for inputs with out-of-band signaling or use an alternate client for realtime commands (such as WEBUI).

I have implemented and tested this fix, and can provide a PR.

Please let me know what you think.

[MitchBradley]

Please provide the PR

[MitchBradley]

The PR will be even more useful against FluidNC instead of Grbl_Esp32, since the vast majority of our development efforts have shifted to FluidNC. https://github.com/bdring/FluidNC