fix on merge #66 (#67)

* fix: Responding to bogus frames #61

as per @IanAber prevent sending STATUS_ILLEGAL_FUNCTION on a bogus frame:
- `validateRequest()`: move CRC and validate address to start of procedure

Some extras for extra snappy performance optimalisations:
- `relevantAddress()`: keep the check it local in , since we provide the `unitAddress` to check for broadcast.
- `validateRequest()`: keep is_Broadcast check local , it's all here i.s.o. calling isBroadcast() => `calling readUnitAddress()` : inline with `createResponse()`
- `executeCallback`: remove bool `isbroadcast` and avoid thus extra branch check and memory usage

* fix on merge #66 for issue #61

Better fix #61 :
Move CRC check back to the end of checking in `Modbus::validateRequest()` and use a variable for the `reportException(STATUS_ILLEGAL_FUNCTION);` so the report can be done after the CRC check.

* fix it

Author: dgoo2308

src/ModbusSlave.cpp

@@ -614,65 +614,60 @@ bool Modbus::relevantAddress(uint8_t unitAddress)
  */
 bool Modbus::validateRequest()
 {
-    // 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;
     }
     // The minimum buffer size (1 x Address, 1 x Function, n x Data, 2 x CRC).
     uint16_t expected_requestBufferSize = MODBUS_FRAME_SIZE;
-
+    bool report_illegal_function=false;
+    
     // 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 (_requestBuffer[MODBUS_ADDRESS_INDEX] == MODBUS_BROADCAST_ADDRESS)
-        {
-            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 (_requestBuffer[MODBUS_ADDRESS_INDEX] == MODBUS_BROADCAST_ADDRESS)
-        {
-            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 regosters (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;
+        case FC_READ_EXCEPTION_STATUS:
+            // Broadcast is not supported, so ignore this request.
+            if (_requestBuffer[MODBUS_ADDRESS_INDEX] == MODBUS_BROADCAST_ADDRESS)
+            {
+                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 (_requestBuffer[MODBUS_ADDRESS_INDEX] == MODBUS_BROADCAST_ADDRESS)
+            {
+                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 regosters (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;
+        default:
+            // Unknown function code.
+             report_illegal_function=true;
     }
 
     // If the received data is smaller than what we expect, ignore this request.
@@ -681,9 +676,23 @@ bool Modbus::validateRequest()
         return false;
     }
 
+    // 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;
+    }
+    
+    // report_illegal_function after the CRC check, cheaper
+    if (report_illegal_function)
+    {
+        Modbus::reportException(STATUS_ILLEGAL_FUNCTION);
+        return false;
+    }
+    
     // Set the length to be read from the request to the calculated expected length.
     _requestBufferLength = expected_requestBufferSize;
-
+    
     return true;
 }