openenergysolutions / modbus-cpp

Modbus TCP master library in modern C++
https://aegis4ics.github.io/modbus-cpp/
Apache License 2.0
2 stars 0 forks source link

off by one for read holding registers #5

Closed jadamcrain closed 6 years ago

jadamcrain commented 6 years ago

If you configure a read holding register request, the received addresses are incremented one more than they should be, e.g. the request below results in the log below.

ReadHoldingRegistersRequest req{ 10, 59 };

What's being sent over the wire is correct, but the construction of the response has the all the indices incremented by one:

Start
a
[2018-02-20 11:55:49.010] [Example channel] [info] Sending request. UnitId: 0x01, TransactionId: 0x0000, Timeout: 3000 ms, Length: 5 bytes.
[2018-02-20 11:55:49.021] [Example channel - Connection] [info] Establishing connection to 172.168.1.248:502
[2018-02-20 11:55:49.032] [Example channel - Connection] [info] Connection established
[2018-02-20 11:55:49.032] [Example channel - Connection] [info] Sent 12 bytes
[2018-02-20 11:55:49.040] [Example channel - Connection] [info] Received 127 bytes
[2018-02-20 11:55:49.041] [Example channel] [info] Received response for pending request. UnitId: 0x01, TransactionId: 0x0000.
11: 1232
12: 1232
13: 1233
14: 1232
15: 0
16: 0
17: 0
18: 0
19: 0
20: 0
21: 0
22: 0
23: 0
24: 0
25: 600
26: 0
27: 0
28: 0
29: 0
30: 0
31: 0
32: 0
33: 0
34: 0
35: 0
36: 0
37: 0
38: 0
39: 0
40: 0
41: 0
42: 0
43: 0
44: 0
45: 0
46: 0
47: 0
48: 0
49: 0
50: 0
51: 0
52: 1000
53: 0
54: 1000
55: 0
56: 1000
57: 0
58: 1000
59: 0
60: 829
61: 0
62: 924
63: 0
64: 82
65: 0
66: 1
67: 0
68: 104
69: 0
jadamcrain commented 6 years ago

This gave me an excuse to read some code. Very easy to follow.

https://github.com/aegis4ics/modbus-cpp/blob/channel/lib/src/messages/ReadHoldingRegistersResponse.cpp#L41

Would be good during refactoring to see if we can remove the code duplication for holding/input registers:

https://github.com/aegis4ics/modbus-cpp/blob/channel/lib/src/messages/ReadInputRegistersResponse.cpp#L41

They're basically the same thing, right?

emgre commented 6 years ago

It was an interpretation of section 4.4 of the Modbus Application Protocol v1.1b3, where it says this:

The MODBUS application protocol defines precisely PDU addressing rules. In a MODBUS PDU each data is addressed from 0 to 65535. It also defines clearly a MODBUS data model composed of 4 blocks that comprises several elements numbered from 1 to n. In the MODBUS data Model each element within a data block is numbered from 1 to n. Afterwards the MODBUS data model has to be bound to the device application ( IEC-61131 object, or other application model). The pre-mapping between the MODBUS data model and the device application is totally vendor device specific.

To be honest, it doesn't make much sense to do that +1 offset just to start at 1 instead of 0. I'll remove that.

I'll also refactor the thing to avoid code duplication as ou suggested.