fix: reading multiple holding registers in modbus input plugin #8628
Conversation
|
Looks good @garciaolais thanks for opening up another PR! |
plugins/inputs/modbus/modbus.go
Outdated
| ii++ | ||
| registersRange = append(registersRange, registerRange{start, end - start + 1}) |
There was a problem hiding this comment.
Hmmm I'm pretty sure that the additional i++ here means we should change end-start+1 to end-start!? However, I would feel much more safe if we remove the additional i++ and keep the range calculation as is... :-/
There was a problem hiding this comment.
we can remove i++ but the other change end-start+1 to end-start brake some test
There was a problem hiding this comment.
Hi @srebhan
Do you have any other comments about this issue?
|
@garciaolais can you please comment on my question above regarding the new increment of |
|
@srebhan sorry for the delay but I really doesn't have free time to check this. Let me chance to retake it and I hope to test it this week. |
|
@garciaolais sure, just wanted to make sure it is not forgotten. Please let me know about the results of your tests! |
There was a problem hiding this comment.
Looks good to me assuming that you tested the code @garciaolais ... :-)
There was a problem hiding this comment.
This all looks good. One ask is that the tests don't validate the generated metric. It's possible that it runs without errors but doesn't give you the response you'd expect.
Does it make sense to add some lines to at least one of the tests to make sure the metric was generated as expected?
Required for all PRs:
Signed CLA.
Associated README.md updated.
Has appropriate unit tests.
Fix Error while reading multiple holding registers on modbus input plugin #7227
if you try to read multiple consecutive addresses , modbus return error "quantity 346 must be between 1 and 125".
this happens because we have a limitation with the number of registers that we can retrieve, 125 for holding registers and Input registers and 2000 for discrete inputs and coils