yaacov/ArduinoModbusSlave

Responding to bogus frames

IanAber opened this issue · 3 comments

IanAber commented

I found that the slave is responding to bad packets when it sees a command it does not recognise. This is causing collisions on the bus and corruption when other slaves are present.
The problem is that the Validate function does not check the CRC first, only after it has decoded the message. If during the decoding the validate function finds a Modbus function code it does not handle it sends a STATUS_ILLEGAL_FUNCTION message back but this is done BEFORE the CRC is checked and so could very well be due to a packet not addressed to us at all. I am seeing errors from the master where a response was received from the WRONG slave.

Moving the CRC check and adding an address check to the beginning of the validate function fixes the problem and I see no more errors reported by the master..

`bool Modbus::validateRequest()
{
// IanA - Moved CRC check so we do not attempt to dscipher any invalid messages
// Check the crc, and if it isn't correct ignore the request.
uint16_t crc = readCRC(_requestBuffer, _requestBufferLength);
if (Modbus::calculateCRC(_requestBuffer, _requestBufferLength - MODBUS_CRC_LENGTH) != crc)
{
return false;
}
// Check that the message was addressed to us
if (!Modbus::relevantAddress(_requestBuffer[MODBUS_ADDRESS_INDEX]))
{
return false;
}
// IanA - End
// The minimum buffer size (1 x Address, 1 x Function, n x Data, 2 x CRC).
uint16_t expected_requestBufferSize = MODBUS_FRAME_SIZE;

// Check the validity of the data based on the function code.
switch (_requestBuffer[MODBUS_FUNCTION_CODE_INDEX])
{
case FC_READ_EXCEPTION_STATUS:
    // Broadcast is not supported, so ignore this request.
    if (isBroadcast())
    {
        return false;
    }
    break;

case FC_READ_COILS:             // Read coils (digital read).
case FC_READ_DISCRETE_INPUT:    // Read input state (digital read).
case FC_READ_HOLDING_REGISTERS: // Read holding registers (analog read).
case FC_READ_INPUT_REGISTERS:   // Read input registers (analog read).
    // Broadcast is not supported, so ignore this request.
    if (isBroadcast())
    {
        return false;
    }
    // Add bytes to expected request size (2 x Index, 2 x Count).
    expected_requestBufferSize += 4;
    break;

case FC_WRITE_COIL:     // Write coils (digital write).
case FC_WRITE_REGISTER: // Write registers (digital write).
    // Add bytes to expected request size (2 x Index, 2 x Count).
    expected_requestBufferSize += 4;
    break;

case FC_WRITE_MULTIPLE_COILS:
case FC_WRITE_MULTIPLE_REGISTERS:
    // Add bytes to expected request size (2 x Index, 2 x Count, 1 x Bytes).
    expected_requestBufferSize += 5;
    if (_requestBufferLength >= expected_requestBufferSize)
    {
        // Add bytes to expected request size (n x Bytes).
        expected_requestBufferSize += _requestBuffer[6];
    }
    break;

default:
    // Unknown function code.
    Modbus::reportException(STATUS_ILLEGAL_FUNCTION);
    return false;
}

// If the received data is smaller than what we expect, ignore this request.
if (_requestBufferLength < expected_requestBufferSize)
{
    return false;
}

// Set the length to be read from the request to the calculated expected length.
_requestBufferLength = expected_requestBufferSize;

// This is now done at the start of the function so it is no longer needed.
// Check the crc, and if it isn't correct ignore the request.
// uint16_t crc = readCRC(_requestBuffer, _requestBufferLength);
// if (Modbus::calculateCRC(_requestBuffer, _requestBufferLength - MODBUS_CRC_LENGTH) != crc)
// {
// return false;
// }
return true;
}
`

yaacov commented

Thank you for the issue !

can you make a pull request fixing that ?

IanAber commented
yaacov commented

thanks, have fun :-)