simonvetter / modbus

Go modbus stack (client and server)
MIT License
280 stars 89 forks source link

Feature request: modbus error response codes #4

Closed wz2b closed 3 years ago

wz2b commented 3 years ago

I am building a modbus proxy using this library as the base. The proxy speaks modbus TLS on one interface and normal Modbus TCP on a second interface. The purpose is to add TLS capabilities to devices that can't themselves support it.

When I make a request to a client, e.g.:

values, err := h.target.ReadRegisters(req.Addr, req.Quantity, modbus.HOLDING_REGISTER)

I would like to be able to get the actual error response from the target device (on the insecure side) so I can return that same response code to the secure client. I can think of a few possible ways to do this:

  1. Have these methods return something that implements the error interface but includes an extra field with more information about the failure. (Can this be done without type assertion?)
  2. Implement a method GetLastModbusException() that can be called in a synchronous manner before making the next request
  3. Extend the API with new methods that return more complete error information
  4. Modify the result strings that come back to include the response code, then my software parses thay out of the string (yuck)

Since this library is synchronous as a client I am leaning toward a GetLastModbusException() approach. As it is, I already make all my requests with (my own) mutex because I need to set a per-request unit ID. Most of the current request-handling code looks like this:

case res.functionCode == (req.functionCode | 0x80):
    if len(res.payload) != 1 {
        err = ErrProtocolError
        return
    }
    err = mapExceptionCodeToError(res.payload[0])
default:
    err = ErrProtocolError
    mc.logger.Warningf("unexpected response code (%v)", res.functionCode)
}

so it's minimal effort to just store the received function code in ModbusClient. I don't find that very elegant, but it may have the least impact on existing code.

Being able to tell the difference between "Target didn't respond" and "Target responded with an error" is also important.

I don't really want to flat-out fork this library, I'd rather have a conversation about ideas and see what you think and put my effort into something that could be a PR. Thoughts?

simonvetter commented 3 years ago

Assuming you're using a server instance (with a tcp+tls url), incoming modbus requests are serviced by your custom handler, where you're using a client to forward the request to the regular tcp or rtu target. Is that correct?

Here's how I'm usually writing proxies/tcp to rtu gateways.

func (mph *ModbusProxyHandler) HandleHoldingRegisters(req *modbus.HoldingRegistersRequest) (res []uint16, err error) {
        // add any required role or host-based auth here

    // acquire a lock to avoid concurrency issues.
    eh.lock.Lock()
    // release the lock upon return
    defer eh.lock.Unlock()

        // tell the modbus client to target the same slave id as that of the incoming request
        eh.client.SetUnitId(req.UnitId)

    if req.IsWrite {
            // the incoming request was a write: use client.WriteRegisters() to forward that write request
            // to the target device
            err = eh.client.WriteRegisters(req.Addr, req.Args)
        } else {
           // the request was a read: use client.ReadRegisters() to read the requested registers from the
           // target device.
           res, err = eh.client.ReadRegisters(req.Addr, req.Quantity, modbus.HOLDING_REGISTER)
        }

        return

Once the request is done, one of the following will happen:

See https://github.com/simonvetter/modbus/blob/1ef4e52208145ee64a6dc84276edf3b72813deb6/modbus.go#L95 and its call sites in server.go for more details.

Would this approach work for your use case? You may also remap the errors before returning from the handler, do role-based auth before calling into the client, etc. but for a simple proxy you should be all set unless I missed anything.

simonvetter commented 3 years ago

Going to close this for now, feel free to reopen if needed.