fix(inputs/modbus) influxdata#11105 no empty grouped requests

Simple modification to
* allow starting the field list with an omitted field (useful if many requests needed)
* avoids requests where all fields are omitted

As a result, the total number of request is generally lower, also improving performance

improve readme

readme linting

use empty instaed of len = 0

wip

wip

wip

plugins/inputs/modbus/README.md

@@ -53,6 +53,7 @@ Registers via Modbus TCP or Modbus RTU/ASCII.
   configuration_type = "register"
 
   ## --- "register" configuration style ---
+  ## Prefer the "request" configuarion style (See below, that offers more options and improvements)
 
   ## Measurements
   ##
@@ -322,6 +323,7 @@ the datasheet of your modbus device. This is a mandatory setting.
 
 For _coil_ and _discrete input_ registers this setting specifies the __bit__
 containing the value of the field.
+For _coil_ and _discrete input_ registers this setting specifies the **bit** containing the value of the field.
 
 ##### name
 
@@ -331,6 +333,7 @@ and can be omitted in this case.
 
 __Please note:__ There cannot be multiple fields with the same `name` in one
 metric identified by `measurement`, `slave_id` and `register`.
+**Please note:** There cannot be multiple fields with the same `name` in one metric identified by `measurement`, `slave_id` and `register`.
 
 ##### register datatype
 
@@ -357,6 +360,7 @@ cases.
 
 __Please note:__ The resulting field-type will be set to `FLOAT64` if no output
 format is specified.
+**Please note:** The resulting field-type will be set to `FLOAT64` if no output format is specified.
 
 ##### output datatype
 
@@ -391,13 +395,27 @@ requests. This way, you can fill "holes" in the addresses to construct
 consecutive address ranges resulting in a single request. Using a single modbus
 request can be beneficial as the values are all collected at the same point in
 time.
+When specifying `omit=true`, the corresponding field will be ignored when collecting the metric but is taken into account when constructing the modbus requests. This way, you can fill "holes" in the addresses to construct consecutive address ranges resulting in a single request. Using a single modbus request can be beneficial as the values are all collected at the same point in time.
+If your device can produce many fields, you can list all of them as `omit=false` (except of course the ones you are actually interested in) and let telegraf find the optimal number of requests needed.
 
 #### Tags definitions
 
 Each `request` can be accompanied by tags valid for this request.
 
 __Please note:__ These tags take precedence over predefined tags such as `name`,
 `type` or `slave_id`.
+**Please note:** These tags take precedence over predefined tags such as `name`, `type` or `slave_id`.
+
+#### Optimising the number of requests
+
+In the case where your modbus device can produce several hundreds of signals but you 
+are interested in logging only a few of them, you can be interested in optimising the
+number of requests sent to your device. In order to do so, list all the fields available
+in your device in your configuration, set all of them to `omit=true` except for the fields
+you want to log and set the `optimise_requests` to `true`.
+
+**Nota Bene** you should not use the `optimise_requests` options if your device requires
+the first address to always be queried.
 
 ---
 

plugins/inputs/modbus/configuration_register.go

@@ -82,7 +82,7 @@ func (c *ConfigurationOriginal) initRequests(fieldDefs []fieldDefinition, maxQua
 	if err != nil {
 		return nil, err
 	}
-	return groupFieldsToRequests(fields, nil, maxQuantity), nil
+	return groupFieldsToRequests(fields, nil, maxQuantity, false), nil
 }
 
 func (c *ConfigurationOriginal) initFields(fieldDefs []fieldDefinition) ([]field, error) {

plugins/inputs/modbus/configuration_request.go

@@ -21,12 +21,13 @@ type requestFieldDefinition struct {
 }
 
 type requestDefinition struct {
-	SlaveID      byte                     `toml:"slave_id"`
-	ByteOrder    string                   `toml:"byte_order"`
-	RegisterType string                   `toml:"register"`
-	Measurement  string                   `toml:"measurement"`
-	Fields       []requestFieldDefinition `toml:"fields"`
-	Tags         map[string]string        `toml:"tags"`
+	SlaveID          byte                     `toml:"slave_id"`
+	ByteOrder        string                   `toml:"byte_order"`
+	RegisterType     string                   `toml:"register"`
+	Measurement      string                   `toml:"measurement"`
+	Fields           []requestFieldDefinition `toml:"fields"`
+	Tags             map[string]string        `toml:"tags"`
+	OptimiseRequests bool                     `toml:"optimise_requests"`
 }
 
 type ConfigurationPerRequest struct {
@@ -147,16 +148,16 @@ func (c *ConfigurationPerRequest) Process() (map[byte]requestSet, error) {
 
 		switch def.RegisterType {
 		case "coil":
-			requests := groupFieldsToRequests(fields, def.Tags, maxQuantityCoils)
+			requests := groupFieldsToRequests(fields, def.Tags, maxQuantityCoils, def.OptimiseRequests)
 			set.coil = append(set.coil, requests...)
 		case "discrete":
-			requests := groupFieldsToRequests(fields, def.Tags, maxQuantityDiscreteInput)
+			requests := groupFieldsToRequests(fields, def.Tags, maxQuantityDiscreteInput, def.OptimiseRequests)
 			set.discrete = append(set.discrete, requests...)
 		case "holding":
-			requests := groupFieldsToRequests(fields, def.Tags, maxQuantityHoldingRegisters)
+			requests := groupFieldsToRequests(fields, def.Tags, maxQuantityHoldingRegisters, def.OptimiseRequests)
 			set.holding = append(set.holding, requests...)
 		case "input":
-			requests := groupFieldsToRequests(fields, def.Tags, maxQuantityInputRegisters)
+			requests := groupFieldsToRequests(fields, def.Tags, maxQuantityInputRegisters, def.OptimiseRequests)
 			set.input = append(set.input, requests...)
 		default:
 			return nil, fmt.Errorf("unknown register type %q", def.RegisterType)

plugins/inputs/modbus/modbus_test.go

@@ -1923,4 +1923,47 @@ func TestRequestsStartingWithOmits(t *testing.T) {
 	require.NoError(t, modbus.Gather(&acc))
 	acc.Wait(len(expected))
 	testutil.RequireMetricsEqual(t, expected, acc.GetTelegrafMetrics(), testutil.IgnoreTime())
+	require.Len(t, modbus.requests[1].holding, 1)
+	require.Equal(t, uint16(0), modbus.requests[1].holding[0].address)
+}
+
+func TestOptimisingRequestNumber(t *testing.T) {
+	modbus := Modbus{
+		Name:              "Test",
+		Controller:        "tcp://localhost:1502",
+		ConfigurationType: "request",
+		Log:               testutil.Logger{},
+	}
+	modbus.Requests = []requestDefinition{
+		{SlaveID: 1,
+			ByteOrder:        "ABCD",
+			RegisterType:     "holding",
+			OptimiseRequests: true,
+			Fields: []requestFieldDefinition{
+				{
+					Name:      "holding-0",
+					Address:   uint16(0),
+					InputType: "INT16",
+					Omit:      true,
+				},
+				{
+					Name:      "holding-1",
+					Address:   uint16(1),
+					InputType: "UINT16",
+					Omit:      true,
+				},
+				{
+					Name:      "holding-2",
+					Address:   uint16(2),
+					InputType: "INT64",
+					Omit:      false,
+				},
+			},
+		},
+	}
+	require.NoError(t, modbus.Init())
+	require.NotEmpty(t, modbus.requests)
+	require.NotNil(t, modbus.requests[1])
+	require.Len(t, modbus.requests[1].holding, 1)
+	require.Equal(t, uint16(2), modbus.requests[1].holding[0].address)
 }

plugins/inputs/modbus/request.go

@@ -13,20 +13,17 @@ func newRequest(f field, tags map[string]string) request {
 	r := request{
 		address: f.address,
 		length:  f.length,
-		fields:  []field{},
+		fields:  []field{f},
 		tags:    map[string]string{},
 	}
-	if !f.omit {
-		r.fields = append(r.fields, f)
-	}
 	// Copy the tags
 	for k, v := range tags {
 		r.tags[k] = v
 	}
 	return r
 }
 
-func groupFieldsToRequests(fields []field, tags map[string]string, maxBatchSize uint16) []request {
+func groupFieldsToRequests(fields []field, tags map[string]string, maxBatchSize uint16, optimise bool) []request {
 	if len(fields) == 0 {
 		return nil
 	}
@@ -37,7 +34,9 @@ func groupFieldsToRequests(fields []field, tags map[string]string, maxBatchSize
 		addrJ := fields[j].address
 		return addrI < addrJ || (addrI == addrJ && fields[i].length > fields[j].length)
 	})
-
+	if optimise { // if it is OK to remove the leading omitted fields, try to optimise the number of requests
+		return groupFieldsToRequestsOptimised(fields, tags, maxBatchSize)
+	}
 	// Construct the consecutive register chunks for the addresses and construct Modbus requests.
 	// For field addresses like [1, 2, 3, 5, 6, 10, 11, 12, 14] we should construct the following
 	// requests (1, 3) , (5, 2) , (10, 3), (14 , 1). Furthermore, we should respect field boundaries
@@ -65,5 +64,48 @@ func groupFieldsToRequests(fields []field, tags map[string]string, maxBatchSize
 		current = newRequest(f, tags)
 	}
 	requests = append(requests, current)
+	// Filter out completely empty requests
+	nonEmptyRequests := make([]request, 0, len(requests))
+	for _, request := range requests {
+		if len(request.fields) != 0 {
+			nonEmptyRequests = append(nonEmptyRequests, request)
+		}
+	}
+	requests = nonEmptyRequests
 	return requests
 }
+
+func isNonEmptyRequest(r request) bool {
+	return !((len(r.fields) == 0) || ((len(r.fields) == 1) && (r.fields[0].address == r.address)))
+}
+
+func groupFieldsToRequestsOptimised(fields []field, tags map[string]string, maxBatchSize uint16) []request {
+	var requests []request
+	current := newRequest(fields[0], tags)
+	for _, f := range fields[1:] {
+		// Check if we need to interrupt the current chunk and require a new one
+		needInterrupt := f.address != current.address+current.length            // not consecutive
+		needInterrupt = needInterrupt || f.length+current.length > maxBatchSize // too large
+		if !needInterrupt {
+			// Still safe to add the field to the current request
+			current.length += f.length
+			if !f.omit {
+				// Omit adding the field but use it for constructing the request.
+				if isNonEmptyRequest(current) {
+					current.fields = append(current.fields, f)
+				} else {
+					current = newRequest(f, tags)
+				}
+			}
+		}
+		if isNonEmptyRequest(current) {
+			requests = append(requests, current)
+		}
+		current = newRequest(f, tags)
+	}
+	if isNonEmptyRequest(current) {
+		requests = append(requests, current)
+	}
+	return requests
+
+}

plugins/inputs/modbus/sample_request.conf

@@ -74,6 +74,26 @@
       machine = "impresser"
       location = "main building"
 
+  [[inputs.modbus.request]]
+    ## Holding example with request optimisation
+    ## The first field will not be added to the request.
+    slave_id = 1
+    byte_order = "DCBA"
+    register = "holding"
+    optimise_requests = "true"
+    fields = [
+      { address=0, name="voltage",      type="INT16",   omit=true  },
+      { address=1, name="current",      type="INT32",   scale=0.001 },
+      { address=3, name="power",        type="UINT32",  omit=true   },
+      { address=5, name="energy",       type="FLOAT32", scale=0.001, measurement="W" },
+      { address=7, name="frequency",    type="UINT32",  scale=0.1   },
+      { address=8, name="power_factor", type="INT64",   scale=0.01  },
+    ]
+
+    [[inputs.modbus.request.tags]]
+      machine = "impresser"
+      location = "main building"
+
   [[inputs.modbus.request]]
     ## Input example with type conversions
     slave_id = 1