From 4934756a1731ea1ceda90091c9301d6d343cbae5 Mon Sep 17 00:00:00 2001 From: Antonio Garcia Date: Wed, 30 Dec 2020 11:18:30 -0600 Subject: [PATCH 1/6] add validation for range --- plugins/inputs/modbus/modbus.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/plugins/inputs/modbus/modbus.go b/plugins/inputs/modbus/modbus.go index 21bd8a977da7b..6c99411afc829 100644 --- a/plugins/inputs/modbus/modbus.go +++ b/plugins/inputs/modbus/modbus.go @@ -217,19 +217,29 @@ func (m *Modbus) InitRegister(fields []fieldContainer, name string) error { sort.Slice(addrs, func(i, j int) bool { return addrs[i] < addrs[j] }) ii := 0 + maxQuantity := 1 var registersRange []registerRange + if name == cDiscreteInputs || name == cCoils { + maxQuantity = 2000 + } + + if name == cInputRegisters || name == cHoldingRegisters { + maxQuantity = 125 + } // Get range of consecutive integers // [1, 2, 3, 5, 6, 10, 11, 12, 14] // (1, 3) , (5, 2) , (10, 3), (14 , 1) for range addrs { + quantity := 1 if ii < len(addrs) { start := addrs[ii] end := start - for ii < len(addrs)-1 && addrs[ii+1]-addrs[ii] == 1 { + for ii < len(addrs)-1 && addrs[ii+1]-addrs[ii] == 1 && quantity < maxQuantity { end = addrs[ii+1] ii++ + quantity++ } ii++ registersRange = append(registersRange, registerRange{start, end - start + 1}) From e17603e40d2a13335d0395b2f69e062169fbeffa Mon Sep 17 00:00:00 2001 From: Antonio Garcia Date: Wed, 30 Dec 2020 11:19:15 -0600 Subject: [PATCH 2/6] add test for range validation --- plugins/inputs/modbus/modbus_test.go | 60 ++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/plugins/inputs/modbus/modbus_test.go b/plugins/inputs/modbus/modbus_test.go index 07af3369a66ec..570491cc3f958 100644 --- a/plugins/inputs/modbus/modbus_test.go +++ b/plugins/inputs/modbus/modbus_test.go @@ -1,6 +1,7 @@ package modbus import ( + "fmt" "testing" m "github.com/goburrow/modbus" @@ -655,6 +656,65 @@ func TestHoldingRegisters(t *testing.T) { } } +func TestReadMultipleCoilLimit(t *testing.T) { + serv := mbserver.NewServer() + err := serv.ListenTCP("localhost:1502") + assert.NoError(t, err) + defer serv.Close() + + fcs := []fieldContainer{} + for i := 0; i <= 2005; i++ { + fc := fieldContainer{} + fc.Name = fmt.Sprintf("coil-%v", i) + fc.Address = []uint16{uint16(i)} + fcs = append(fcs, fc) + } + + modbus := Modbus{ + Name: "TestReadCoils", + Controller: "tcp://localhost:1502", + SlaveID: 1, + Coils: fcs, + } + + err = modbus.Init() + assert.NoError(t, err) + var acc testutil.Accumulator + err = modbus.Gather(&acc) + assert.NoError(t, err) +} + +func TestReadMultipleHoldingRegisterLimit(t *testing.T) { + serv := mbserver.NewServer() + err := serv.ListenTCP("localhost:1502") + assert.NoError(t, err) + defer serv.Close() + + fcs := []fieldContainer{} + for i := 0; i <= 200; i++ { + fc := fieldContainer{} + fc.Name = fmt.Sprintf("HoldingRegister-%v", i) + fc.ByteOrder = "AB" + fc.DataType = "INT16" + fc.Scale = 1.0 + fc.Address = []uint16{uint16(i)} + fcs = append(fcs, fc) + } + + modbus := Modbus{ + Name: "TestHoldingRegister", + Controller: "tcp://localhost:1502", + SlaveID: 1, + HoldingRegisters: fcs, + } + + err = modbus.Init() + assert.NoError(t, err) + var acc testutil.Accumulator + err = modbus.Gather(&acc) + assert.NoError(t, err) +} + func TestRetrySuccessful(t *testing.T) { retries := 0 maxretries := 2 From 4fec465d4d10f340b32530687bdd9b078f0770ba Mon Sep 17 00:00:00 2001 From: Antonio Garcia Date: Sun, 3 Jan 2021 18:15:28 -0600 Subject: [PATCH 3/6] Change 'if' to 'else if' --- plugins/inputs/modbus/modbus.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plugins/inputs/modbus/modbus.go b/plugins/inputs/modbus/modbus.go index 6c99411afc829..d5e7918307cc9 100644 --- a/plugins/inputs/modbus/modbus.go +++ b/plugins/inputs/modbus/modbus.go @@ -221,9 +221,7 @@ func (m *Modbus) InitRegister(fields []fieldContainer, name string) error { var registersRange []registerRange if name == cDiscreteInputs || name == cCoils { maxQuantity = 2000 - } - - if name == cInputRegisters || name == cHoldingRegisters { + } else if name == cInputRegisters || name == cHoldingRegisters { maxQuantity = 125 } From 0648d4a11d8e2c55a4ce760300f7262cf916f0cc Mon Sep 17 00:00:00 2001 From: Antonio Garcia Date: Tue, 5 Jan 2021 22:08:18 -0600 Subject: [PATCH 4/6] improved logic --- plugins/inputs/modbus/modbus.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/plugins/inputs/modbus/modbus.go b/plugins/inputs/modbus/modbus.go index d5e7918307cc9..af8d2f25e6ea6 100644 --- a/plugins/inputs/modbus/modbus.go +++ b/plugins/inputs/modbus/modbus.go @@ -229,19 +229,20 @@ func (m *Modbus) InitRegister(fields []fieldContainer, name string) error { // [1, 2, 3, 5, 6, 10, 11, 12, 14] // (1, 3) , (5, 2) , (10, 3), (14 , 1) for range addrs { + if ii >= len(addrs) { + break + } quantity := 1 - if ii < len(addrs) { - start := addrs[ii] - end := start - - for ii < len(addrs)-1 && addrs[ii+1]-addrs[ii] == 1 && quantity < maxQuantity { - end = addrs[ii+1] - ii++ - quantity++ - } + start := addrs[ii] + end := start + + for ii < len(addrs)-1 && addrs[ii+1]-addrs[ii] == 1 && quantity < maxQuantity { + end = addrs[ii+1] ii++ - registersRange = append(registersRange, registerRange{start, end - start + 1}) + quantity++ } + ii++ + registersRange = append(registersRange, registerRange{start, end - start + 1}) } m.registers = append(m.registers, register{name, registersRange, fields}) From 21ac610f0c31735950e80ca6f877c856627441ab Mon Sep 17 00:00:00 2001 From: garciaolais Date: Tue, 19 Jan 2021 21:31:27 -0600 Subject: [PATCH 5/6] remove unnecessary increment --- plugins/inputs/modbus/modbus.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/inputs/modbus/modbus.go b/plugins/inputs/modbus/modbus.go index af8d2f25e6ea6..4959fc260f7bc 100644 --- a/plugins/inputs/modbus/modbus.go +++ b/plugins/inputs/modbus/modbus.go @@ -241,7 +241,7 @@ func (m *Modbus) InitRegister(fields []fieldContainer, name string) error { ii++ quantity++ } - ii++ + registersRange = append(registersRange, registerRange{start, end - start + 1}) } From b27316dac9c56028262ddc841016d01e255e6f84 Mon Sep 17 00:00:00 2001 From: garciaolais Date: Sat, 20 Feb 2021 23:43:49 -0600 Subject: [PATCH 6/6] add validation for generated metrics --- plugins/inputs/modbus/modbus.go | 3 +- plugins/inputs/modbus/modbus_test.go | 41 ++++++++++++++++++++++++++-- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/plugins/inputs/modbus/modbus.go b/plugins/inputs/modbus/modbus.go index 4959fc260f7bc..ef2a32afbd5aa 100644 --- a/plugins/inputs/modbus/modbus.go +++ b/plugins/inputs/modbus/modbus.go @@ -241,6 +241,7 @@ func (m *Modbus) InitRegister(fields []fieldContainer, name string) error { ii++ quantity++ } + ii++ registersRange = append(registersRange, registerRange{start, end - start + 1}) } @@ -444,7 +445,7 @@ func (m *Modbus) getFields() error { for bitPosition := 0; bitPosition < 8; bitPosition++ { bitRawValues[address] = getBitValue(readValue, bitPosition) address = address + 1 - if address+1 > rr.length { + if address > rr.address+rr.length { break } } diff --git a/plugins/inputs/modbus/modbus_test.go b/plugins/inputs/modbus/modbus_test.go index 570491cc3f958..651749308ac31 100644 --- a/plugins/inputs/modbus/modbus_test.go +++ b/plugins/inputs/modbus/modbus_test.go @@ -662,12 +662,26 @@ func TestReadMultipleCoilLimit(t *testing.T) { assert.NoError(t, err) defer serv.Close() + handler := m.NewTCPClientHandler("localhost:1502") + err = handler.Connect() + assert.NoError(t, err) + defer handler.Close() + client := m.NewClient(handler) + fcs := []fieldContainer{} - for i := 0; i <= 2005; i++ { + writeValue := uint16(0) + for i := 0; i <= 4000; i++ { fc := fieldContainer{} fc.Name = fmt.Sprintf("coil-%v", i) fc.Address = []uint16{uint16(i)} fcs = append(fcs, fc) + + t.Run(fc.Name, func(t *testing.T) { + _, err = client.WriteSingleCoil(fc.Address[0], writeValue) + assert.NoError(t, err) + }) + + writeValue = 65280 - writeValue } modbus := Modbus{ @@ -682,6 +696,14 @@ func TestReadMultipleCoilLimit(t *testing.T) { var acc testutil.Accumulator err = modbus.Gather(&acc) assert.NoError(t, err) + + writeValue = 0 + for i := 0; i <= 4000; i++ { + t.Run(modbus.registers[0].Fields[i].Name, func(t *testing.T) { + assert.Equal(t, writeValue, modbus.registers[0].Fields[i].value) + writeValue = 1 - writeValue + }) + } } func TestReadMultipleHoldingRegisterLimit(t *testing.T) { @@ -690,8 +712,14 @@ func TestReadMultipleHoldingRegisterLimit(t *testing.T) { assert.NoError(t, err) defer serv.Close() + handler := m.NewTCPClientHandler("localhost:1502") + err = handler.Connect() + assert.NoError(t, err) + defer handler.Close() + client := m.NewClient(handler) + fcs := []fieldContainer{} - for i := 0; i <= 200; i++ { + for i := 0; i <= 400; i++ { fc := fieldContainer{} fc.Name = fmt.Sprintf("HoldingRegister-%v", i) fc.ByteOrder = "AB" @@ -699,6 +727,11 @@ func TestReadMultipleHoldingRegisterLimit(t *testing.T) { fc.Scale = 1.0 fc.Address = []uint16{uint16(i)} fcs = append(fcs, fc) + + t.Run(fc.Name, func(t *testing.T) { + _, err = client.WriteSingleRegister(fc.Address[0], uint16(i)) + assert.NoError(t, err) + }) } modbus := Modbus{ @@ -713,6 +746,10 @@ func TestReadMultipleHoldingRegisterLimit(t *testing.T) { var acc testutil.Accumulator err = modbus.Gather(&acc) assert.NoError(t, err) + + for i := 0; i <= 400; i++ { + assert.Equal(t, int16(i), modbus.registers[0].Fields[i].value) + } } func TestRetrySuccessful(t *testing.T) {