Return data of too large responses #81
Conversation
srebhan
force-pushed
the
fix_issue_52
branch
from
12a0467
to
5f8153e
Compare
|
@hsteidl anything I can do to get this merged? |
|
@gq0 or @tretmar is there anything I can do to get this PR going forward? |
|
Hello @srebhan , we are currently not able to take the time and assess the changes here thoroughly. It's still not forgotten though ;) We just need time to think about it as this might have direct impact on our product in the end. |
|
Any update here? We need this to fix an actual long-standing issue in Telegraf (influxdata/telegraf#11099) and the effect for existing code is zero if you did proper error checking before... |
|
@hsteidl or @tretmar any update? Another month passed by... This change is fully backward compatible and will NOT change the current behavior if you did error checking before... |
|
@srebhan do you want to re-open against https://github.com/evcc-io/modbus? Seems we'll need to go for a permanent fork anyway due to lack of feedback on #70. |
There was a problem hiding this comment.
Okay, I understand the idea here now.
Essentially, you have devices that return more bytes than given within the Byte count field of the Modbus Response and you want the library here to be more robust against such cases.
Two things that come into my mind here to improve this even further.
- Create a function that is doing this lenght/count check for us
- Currently, there is a lot of duplicate code here.
- Not sure, if this is easily doable without some refactoring here.
- In general, testing of the
clientis missing in this repo completely and it would be really helpful to understand the expectations :D
Still, as this PR doesn't bring any behavioral change other than a different Error type that is being returned, I'm fine with getting this merged first.
I'll then bring the refactoring/test addition into my team.
As already mentioned quite a lot by @andig , we are not really responsive in this repo and I'm sorry for that. Since the time this repo was started, we've got a lot of new people and never touched this repo at all. Thus, no one feels comfortable or even responsible for this currently.
Sure, this doesn't justify the lack of responsiveness ;)
|
@srebhan there is a still a conflict that has to be addressed within |
|
@tretmar resolved the confliict... Thanks for moving this forward. Hope to see some maintenance for this library in the future as it is a quite nice one... |
|
Thanks @srebhan . I'm working on it to get the team more active in here ;) |
|
@tretmar is there any plan for doing a release anytime soon? Otherwise I will pin to the git commit in Telegraf... |
|
you mean something like that: https://github.com/grid-x/modbus/releases/tag/v1.1? 🤔 We can do that if it helps you ;) |
Some devices send too much data in read responses which is not a grey-zone of the spec. However, as we do have enough bytes to return, this PR does so as proposed in the discussion . To be able to detect those malformed responses, the PR also introduces a custom error users can check against.
resolves #52