Connectivity issues with one device impacts data gathering from all other devices

[marcb-dame]

🐞 Bug Report

Affected Services [REQUIRED]

The issue is located in: Device-modbus-go, driver.go, createDeviceClient()

Is this a regression?

Not aware

Description and Minimal Reproduction [REQUIRED]

If any device (ip) connection times out, or route not found, then no device data is gathered from other devices which are working fine.

🔥 Exception or Error

Typically: create device client failed. err: (from driver.go, createDeviceClient()). for example timeout, or route not found.

🌍 Your Environment

Deployment Environment:
Cluster
EdgeX Version [REQUIRED]:
4.0.0

Anything else relevant?
Have a fix if you want it.

[cloudxxx8]

since you have a fix, could you please help a PR for this?
https://lf-edgexfoundry.atlassian.net/wiki/spaces/FA/pages/11667615/Committing+Code+Guidelines

[sansmoraxz]

FYI I tested one thing the grid-x/modbus is safe for concurrent use. (ie guarded by it's own mutexes). Perhaps the addressMutex is no longer necessary.

[sansmoraxz]

@cloudxxx8 is this behaviour really needed?

device-modbus-go/internal/driver/driver.go

Lines 82 to 93 in c708da8

	// workingAddressCount used to check high-frequency command execution to avoid goroutine block
	count, ok := d.workingAddressCount[address]
	if !ok {
	d.workingAddressCount[address] = 1
	} else if count >= concurrentCommandLimit {
	d.addressMutex.Unlock()
	errorMessage := fmt.Sprintf("High-frequency command execution. There are %v commands with the same address in the queue", concurrentCommandLimit)
	d.Logger.Error(errorMessage)
	return fmt.Errorf("%s", errorMessage)
	} else {
	d.workingAddressCount[address] = count + 1
	}

[cloudxxx8]

@cloudxxx8 is this behaviour really needed?

device-modbus-go/internal/driver/driver.go

Lines 82 to 93 in c708da8

// workingAddressCount used to check high-frequency command execution to avoid goroutine block
count, ok := d.workingAddressCount[address]
if !ok {
d.workingAddressCount[address] = 1
} else if count >= concurrentCommandLimit {
d.addressMutex.Unlock()
errorMessage := fmt.Sprintf("High-frequency command execution. There are %v commands with the same address in the queue", concurrentCommandLimit)
d.Logger.Error(errorMessage)
return fmt.Errorf("%s", errorMessage)
} else {
d.workingAddressCount[address] = count + 1
}

We needed it before, but maybe not right now. If someone from the community can help verify on it, we can remove this lock.

[sansmoraxz]

Well it was working fine for me after removing. But have to stress test in the wild and see.

[sansmoraxz]

I did some more testing. The lib works find with the guards but not how we are locking the addresses.

For TCP it works well (so long as device can handle multiple connections, and most likely you would not probably be running multiple slaves on one port). But RTU most people do use multiple devices over a single serial port. And here's the issue:

device-modbus-go/internal/driver/driver.go

Lines 37 to 52 in 2c8c7f3

	func (d *Driver) createDeviceClient(info *ConnectionInfo) (DeviceClient, error) {
	d.clientMutex.Lock()
	defer d.clientMutex.Unlock()
	key := info.String()
	c, ok := d.clientMap[key]
	if ok {
	return c, nil
	}
	c, err := NewDeviceClient(info)
	if err != nil {
	driver.Logger.Errorf("create device client failed. err:%v \n", err)
	return nil, err
	}
	d.clientMap[key] = c
	return c, nil
	}

and

device-modbus-go/internal/driver/config.go

Lines 34 to 39 in 2c8c7f3

	func (info *ConnectionInfo) String() string {
	if info.Protocol == ProtocolTCP {
	return fmt.Sprintf("%s:%s:%d:%d", info.Protocol, info.Address, info.Port, info.UnitID)
	}
	return fmt.Sprintf("%s:%s:%d:%d:%d:%d:%d:%s", info.Protocol, info.Address, info.Port, info.UnitID, info.BaudRate, info.DataBits, info.StopBits, info.Parity)
	}

[sansmoraxz]

We should not have multiple clients for same port.

Probably we should update the handler slaveID, parity, etc. before making requests.

[sansmoraxz]

Otherwise we get timeouts.

Not sure if it's related to OP's issue. But have observed this from stress tests.