feat(inputs.modbus): Add workaround for unusual string-byte locations (#14764)

plugins/inputs/modbus/README.md

@@ -357,6 +357,16 @@ See the [CONFIGURATION.md][CONFIGURATION.md] for more details.
     ## coil registers. This is necessary for some devices see
     ## https://github.com/influxdata/telegraf/issues/8905
     # read_coils_starting_at_zero = false
+
+    ## String byte-location in registers AFTER byte-order conversion
+    ## Some device (e.g. EM340) place the string byte in only the upper or
+    ## lower byte location of a register see
+    ## https://github.com/influxdata/telegraf/issues/14748
+    ## Available settings:
+    ##   lower -- use only lower byte of the register i.e. 00XX 00XX 00XX 00XX
+    ##   upper -- use only upper byte of the register i.e. XX00 XX00 XX00 XX00
+    ## By default both bytes of the register are used i.e. XXXX XXXX.
+    # string_register_location = ""
 ```
 
 ## Notes

plugins/inputs/modbus/configuration_metric.go

@@ -43,6 +43,13 @@ func (c *ConfigurationPerMetric) SampleConfigPart() string {
 }
 
 func (c *ConfigurationPerMetric) Check() error {
+	switch c.workarounds.StringRegisterLocation {
+	case "", "both", "lower", "upper":
+		// Do nothing as those are valid
+	default:
+		return fmt.Errorf("invalid 'string_register_location' %q", c.workarounds.StringRegisterLocation)
+	}
+
 	seed := maphash.MakeSeed()
 	seenFields := make(map[uint64]bool)
 
@@ -302,7 +309,7 @@ func (c *ConfigurationPerMetric) newField(def metricFieldDefinition, mdef metric
 		return field{}, err
 	}
 
-	f.converter, err = determineConverter(inType, order, outType, def.Scale)
+	f.converter, err = determineConverter(inType, order, outType, def.Scale, c.workarounds.StringRegisterLocation)
 	if err != nil {
 		return field{}, err
 	}

plugins/inputs/modbus/configuration_register.go

@@ -35,6 +35,13 @@ func (c *ConfigurationOriginal) SampleConfigPart() string {
 }
 
 func (c *ConfigurationOriginal) Check() error {
+	switch c.workarounds.StringRegisterLocation {
+	case "", "both", "lower", "upper":
+		// Do nothing as those are valid
+	default:
+		return fmt.Errorf("invalid 'string_register_location' %q", c.workarounds.StringRegisterLocation)
+	}
+
 	if err := c.validateFieldDefinitions(c.DiscreteInputs, cDiscreteInputs); err != nil {
 		return err
 	}
@@ -165,7 +172,7 @@ func (c *ConfigurationOriginal) newFieldFromDefinition(def fieldDefinition, type
 			return f, err
 		}
 
-		f.converter, err = determineConverter(inType, byteOrder, outType, def.Scale)
+		f.converter, err = determineConverter(inType, byteOrder, outType, def.Scale, c.workarounds.StringRegisterLocation)
 		if err != nil {
 			return f, err
 		}

plugins/inputs/modbus/configuration_request.go

@@ -46,6 +46,13 @@ func (c *ConfigurationPerRequest) SampleConfigPart() string {
 }
 
 func (c *ConfigurationPerRequest) Check() error {
+	switch c.workarounds.StringRegisterLocation {
+	case "", "both", "lower", "upper":
+		// Do nothing as those are valid
+	default:
+		return fmt.Errorf("invalid 'string_register_location' %q", c.workarounds.StringRegisterLocation)
+	}
+
 	seed := maphash.MakeSeed()
 	seenFields := make(map[uint64]bool)
 
@@ -349,7 +356,7 @@ func (c *ConfigurationPerRequest) newFieldFromDefinition(def requestFieldDefinit
 		return field{}, err
 	}
 
-	f.converter, err = determineConverter(inType, order, outType, def.Scale)
+	f.converter, err = determineConverter(inType, order, outType, def.Scale, c.workarounds.StringRegisterLocation)
 	if err != nil {
 		return field{}, err
 	}

plugins/inputs/modbus/modbus.go

@@ -31,6 +31,7 @@ type ModbusWorkarounds struct {
 	CloseAfterGather        bool            `toml:"close_connection_after_gather"`
 	OnRequestPerField       bool            `toml:"one_request_per_field"`
 	ReadCoilsStartingAtZero bool            `toml:"read_coils_starting_at_zero"`
+	StringRegisterLocation  string          `toml:"string_register_location"`
 }
 
 // According to github.com/grid-x/serial

plugins/inputs/modbus/modbus_test.go

@@ -16,6 +16,7 @@ import (
 
 	"github.com/influxdata/telegraf"
 	"github.com/influxdata/telegraf/config"
+	"github.com/influxdata/telegraf/metric"
 	"github.com/influxdata/telegraf/plugins/inputs"
 	"github.com/influxdata/telegraf/plugins/parsers/influx"
 	"github.com/influxdata/telegraf/testutil"
@@ -570,3 +571,179 @@ func TestRequestsWorkaroundsReadCoilsStartingAtZeroRegister(t *testing.T) {
 	require.Equal(t, maxQuantityCoils, plugin.requests[1].coil[1].address)
 	require.Equal(t, uint16(1), plugin.requests[1].coil[1].length)
 }
+
+func TestWorkaroundsStringRegisterLocation(t *testing.T) {
+	tests := []struct {
+		name     string
+		location string
+		order    string
+		content  []byte
+		expected string
+	}{
+		{
+			name:  "default big-endian",
+			order: "ABCD",
+			content: []byte{
+				0x4d, 0x6f, 0x64, 0x62, 0x75, 0x73, 0x20, 0x53,
+				0x74, 0x72, 0x69, 0x6e, 0x67, 0x00,
+			},
+			expected: "Modbus String",
+		},
+		{
+			name:  "default little-endian",
+			order: "DCBA",
+			content: []byte{
+				0x6f, 0x4d, 0x62, 0x64, 0x73, 0x75, 0x53, 0x20,
+				0x72, 0x74, 0x6e, 0x69, 0x00, 0x67,
+			},
+			expected: "Modbus String",
+		},
+		{
+			name:     "both big-endian",
+			location: "both",
+			order:    "ABCD",
+			content: []byte{
+				0x4d, 0x6f, 0x64, 0x62, 0x75, 0x73, 0x20, 0x53,
+				0x74, 0x72, 0x69, 0x6e, 0x67, 0x00,
+			},
+			expected: "Modbus String",
+		},
+		{
+			name:     "both little-endian",
+			location: "both",
+			order:    "DCBA",
+			content: []byte{
+				0x6f, 0x4d, 0x62, 0x64, 0x73, 0x75, 0x53, 0x20,
+				0x72, 0x74, 0x6e, 0x69, 0x00, 0x67,
+			},
+			expected: "Modbus String",
+		},
+		{
+			name:     "lower big-endian",
+			location: "lower",
+			order:    "ABCD",
+			content: []byte{
+				0x00, 0x4d, 0x00, 0x6f, 0x00, 0x64, 0x00, 0x62,
+				0x00, 0x75, 0x00, 0x73, 0x00, 0x20, 0x00, 0x53,
+				0x00, 0x74, 0x00, 0x72, 0x00, 0x69, 0x00, 0x6e,
+				0x00, 0x67, 0x00, 0x00,
+			},
+			expected: "Modbus String",
+		},
+		{
+			name:     "lower little-endian",
+			location: "lower",
+			order:    "DCBA",
+			content: []byte{
+				0x4d, 0x00, 0x6f, 0x00, 0x64, 0x00, 0x62, 0x00,
+				0x75, 0x00, 0x73, 0x00, 0x20, 0x00, 0x53, 0x00,
+				0x74, 0x00, 0x72, 0x00, 0x69, 0x00, 0x6e, 0x00,
+				0x67, 0x00, 0x00, 0x00,
+			},
+			expected: "Modbus String",
+		},
+		{
+			name:     "upper big-endian",
+			location: "upper",
+			order:    "ABCD",
+			content: []byte{
+				0x4d, 0x00, 0x6f, 0x00, 0x64, 0x00, 0x62, 0x00,
+				0x75, 0x00, 0x73, 0x00, 0x20, 0x00, 0x53, 0x00,
+				0x74, 0x00, 0x72, 0x00, 0x69, 0x00, 0x6e, 0x00,
+				0x67, 0x00, 0x00, 0x00,
+			},
+			expected: "Modbus String",
+		},
+		{
+			name:     "upper little-endian",
+			location: "upper",
+			order:    "DCBA",
+			content: []byte{
+				0x00, 0x4d, 0x00, 0x6f, 0x00, 0x64, 0x00, 0x62,
+				0x00, 0x75, 0x00, 0x73, 0x00, 0x20, 0x00, 0x53,
+				0x00, 0x74, 0x00, 0x72, 0x00, 0x69, 0x00, 0x6e,
+				0x00, 0x67, 0x00, 0x00,
+			},
+			expected: "Modbus String",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			const addr = uint16(8)
+			length := uint16(len(tt.content) / 2)
+
+			expected := []telegraf.Metric{
+				metric.New(
+					"modbus",
+					map[string]string{
+						"name":     "Test",
+						"slave_id": "1",
+						"type":     "holding_register",
+					},
+					map[string]interface{}{
+						"value": tt.expected,
+					},
+					time.Unix(0, 0),
+				),
+			}
+
+			plugin := &Modbus{
+				Name:              "Test",
+				Controller:        "tcp://localhost:1502",
+				ConfigurationType: "request",
+				Log:               testutil.Logger{},
+				Workarounds:       ModbusWorkarounds{StringRegisterLocation: tt.location},
+				ConfigurationPerRequest: ConfigurationPerRequest{
+					Requests: []requestDefinition{
+						{
+							SlaveID:      1,
+							ByteOrder:    tt.order,
+							RegisterType: "holding",
+							Fields: []requestFieldDefinition{
+								{
+									Address:   addr,
+									Name:      "value",
+									InputType: "STRING",
+									Length:    length,
+								},
+							},
+						},
+					},
+				},
+			}
+			require.NoError(t, plugin.Init())
+
+			// Create a mock server and fill in the data
+			serv := mbserver.NewServer()
+			require.NoError(t, serv.ListenTCP("localhost:1502"))
+			defer serv.Close()
+
+			handler := mb.NewTCPClientHandler("localhost:1502")
+			require.NoError(t, handler.Connect())
+			defer handler.Close()
+			client := mb.NewClient(handler)
+			_, err := client.WriteMultipleRegisters(addr, length, tt.content)
+			require.NoError(t, err)
+
+			// Gather the data
+			var acc testutil.Accumulator
+			require.NoError(t, plugin.Gather(&acc))
+
+			// Compare
+			actual := acc.GetTelegrafMetrics()
+			testutil.RequireMetricsEqual(t, expected, actual, testutil.IgnoreTime())
+		})
+	}
+}
+
+func TestWorkaroundsStringRegisterLocationInvalid(t *testing.T) {
+	plugin := &Modbus{
+		Name:              "Test",
+		Controller:        "tcp://localhost:1502",
+		ConfigurationType: "request",
+		Log:               testutil.Logger{},
+		Workarounds:       ModbusWorkarounds{StringRegisterLocation: "foo"},
+	}
+	require.ErrorContains(t, plugin.Init(), `invalid 'string_register_location'`)
+}

plugins/inputs/modbus/sample_general_end.conf

@@ -39,3 +39,13 @@
     ## coil registers. This is necessary for some devices see
     ## https://github.com/influxdata/telegraf/issues/8905
     # read_coils_starting_at_zero = false
+
+    ## String byte-location in registers AFTER byte-order conversion
+    ## Some device (e.g. EM340) place the string byte in only the upper or
+    ## lower byte location of a register see
+    ## https://github.com/influxdata/telegraf/issues/14748
+    ## Available settings:
+    ##   lower -- use only lower byte of the register i.e. 00XX 00XX 00XX 00XX
+    ##   upper -- use only upper byte of the register i.e. XX00 XX00 XX00 XX00
+    ## By default both bytes of the register are used i.e. XXXX XXXX.
+    # string_register_location = ""

plugins/inputs/modbus/type_conversions.go

@@ -18,8 +18,19 @@ func determineUntypedConverter(outType string) (fieldConverterFunc, error) {
 	return nil, fmt.Errorf("invalid output data-type: %s", outType)
 }
 
-func determineConverter(inType, byteOrder, outType string, scale float64) (fieldConverterFunc, error) {
+func determineConverter(inType, byteOrder, outType string, scale float64, strloc string) (fieldConverterFunc, error) {
-	if scale != 0.0 && inType != "STRING" {
+	if inType == "STRING" {
+		switch strloc {
+		case "", "both":
+			return determineConverterString(byteOrder)
+		case "lower":
+			return determineConverterStringLow(byteOrder)
+		case "upper":
+			return determineConverterStringHigh(byteOrder)
+		}
+	}
+
+	if scale != 0.0 {
 		return determineConverterScale(inType, byteOrder, outType, scale)
 	}
 	return determineConverterNoScale(inType, byteOrder, outType)
@@ -85,8 +96,6 @@ func determineConverterNoScale(inType, byteOrder, outType string) (fieldConverte
 		return determineConverterF32(outType, byteOrder)
 	case "FLOAT64":
 		return determineConverterF64(outType, byteOrder)
-	case "STRING":
-		return determineConverterString(byteOrder)
 	}
 	return nil, fmt.Errorf("invalid input data-type: %s", inType)
 }

plugins/inputs/modbus/type_conversions_string.go

@@ -23,3 +23,41 @@ func determineConverterString(byteOrder string) (fieldConverterFunc, error) {
 		return string(s)
 	}, nil
 }
+
+func determineConverterStringLow(byteOrder string) (fieldConverterFunc, error) {
+	tohost, err := endiannessConverter16(byteOrder)
+	if err != nil {
+		return nil, err
+	}
+
+	return func(b []byte) interface{} {
+		// Swap the bytes according to endianness
+		var buf bytes.Buffer
+		for i := 0; i < len(b); i += 2 {
+			v := tohost(b[i : i+2])
+			buf.WriteByte(byte(v & 0xFF))
+		}
+		// Remove everything after null-termination
+		s, _ := bytes.CutSuffix(buf.Bytes(), []byte{0x00})
+		return string(s)
+	}, nil
+}
+
+func determineConverterStringHigh(byteOrder string) (fieldConverterFunc, error) {
+	tohost, err := endiannessConverter16(byteOrder)
+	if err != nil {
+		return nil, err
+	}
+
+	return func(b []byte) interface{} {
+		// Swap the bytes according to endianness
+		var buf bytes.Buffer
+		for i := 0; i < len(b); i += 2 {
+			v := tohost(b[i : i+2])
+			buf.WriteByte(byte(v >> 8))
+		}
+		// Remove everything after null-termination
+		s, _ := bytes.CutSuffix(buf.Bytes(), []byte{0x00})
+		return string(s)
+	}, nil
+}