Apollo3zehn / FluentModbus

Lightweight and fast client and server implementation of the Modbus protocol (TCP/RTU).
MIT License
189 stars 71 forks source link

Malformed Modbus Message under certain Conditions #59

Open joshslark opened 2 years ago

joshslark commented 2 years ago

I'm seeing a weird issue while using this library with a simulator and I think it might be occurring with physical devices as well. To give some context, I'm continuously connecting to multiple devices using Modbus RTU (RS485) to have a close to live readout of values. Since this is a serial connection, I don't want to spend too much time trying to read one device if there is an error. I'm also using Retry Policy from Polly to retry ReadHoldingRegisters if there is an error or response timeout so that transient errors aren't displayed to the user.

When there are multiple errors/timeouts such that multiple devices exhaust their retries, I'm seeing the library send out malformed modbus messages to every device as if the buffer has somehow become corrupted or overflowed. However, I tried Closing and Reopening the connection to the client and even modifying the code so that it clears the buffer with zeroes, and somehow the library continues sending these malformed messages. Only closing the entire application or placing breakpoints in the debugger seems to clear up the issue.

This is the response I'm seeing from the simulator. image

Apollo3zehn commented 2 years ago

Hi joshlark, I'll have a look into it on Monday and com back to you then!

Apollo3zehn commented 2 years ago

Could you please give a code sample of how you communicate to the RTU devices? The only place I can image where something may become strange is the array from the ArrayPool<T> https://github.com/Apollo3zehn/FluentModbus/blob/39d819d7258fbaa48f73966b1a06e25a042361af/src/FluentModbus/ModbusFrameBuffer.cs#L13 which is returned to the pool, when the modbus client is disconnected. If its not returned properly, there may be strange effects. At least with a MemoryPool<T> I have seen such effects.

Does it help to explicitly call client.Disconnect() when the connection fails?

joshslark commented 2 years ago
private async Task<TempReading[]> ReadTemperatureValues(byte unitId)
{
    using (await _modbusLock.LockAsync())
    {
        if (!Connect())
        {
            return new [] {
                TempReading.FailedReading("Failed to connect to serial port", TemperatureType.Ts),
                TempReading.FailedReading("Failed to connect to serial port", TemperatureType.Tc)
            };
        }

        unitId = (byte)(unitId - _unitIdOffset + Globals.ControllerAddressOffset);
        TempReading[] readings = new TempReading[2];

        async Task<Memory<short>> ReadResponse()
        {
            using (await _clientLock.LockAsync())
            {
                var values = await _client.ReadHoldingRegistersAsync<short>(unitId, (ushort)_pvAddress, 2, _cts.Token);
                return values;
            }
        }

        PolicyResult<Memory<short>> result = await _retryPolicy
            .ExecuteAndCaptureAsync(ReadResponse);

        if (result.Outcome == OutcomeType.Successful)
        {
            var values = result.Result.ToArray();
            readings[0] = BuildPVTempReading(values[0]);
            readings[1] = BuildSVTempReading(values[1]);
        }
        else if (result.FinalException.Message == "The asynchronous read operation timed out.")
        {
            string message = $"Response timed out after {_responseTimeout}ms";
            readings[0] = TempReading.FailedReading(message, TemperatureType.Tc);
            readings[1] = TempReading.FailedReading(message, TemperatureType.Ts);
        }
        else
        {
            readings[0] = TempReading.FailedReading(result.FinalException.Message, TemperatureType.Tc);
            readings[1] = TempReading.FailedReading(result.FinalException.Message, TemperatureType.Ts);
        }

        return readings;
    }
}

I've tried putting _client.Close() and then reconnecting in the else if "asynchronous read" block, with no luck.