fix: reading multiple holding registers in modbus input plugin#8628
fix: reading multiple holding registers in modbus input plugin#8628helenosheaa merged 6 commits intoinfluxdata:masterfrom
Conversation
ivorybilled
left a comment
There was a problem hiding this comment.
Logic looks good to me
|
Looks good @garciaolais thanks for opening up another PR! |
srebhan
left a comment
There was a problem hiding this comment.
Looks good basically, but I'd prefer to have the checking for max grouped registers in an else-if to make it obvious that it is exclusive.
| 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?
srebhan
left a comment
There was a problem hiding this comment.
I think the register-range changed. Could you please check my comment in the code!?
|
@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! |
srebhan
left a comment
There was a problem hiding this comment.
Looks good to me assuming that you tested the code @garciaolais ... :-)
ssoroka
left a comment
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?
ssoroka
left a comment
There was a problem hiding this comment.
Actually, would still be great to get a test showing the expected metrics are generated properly.
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