drivers/generic_modbus.{c,h}: fix compilation warnings#1107
drivers/generic_modbus.{c,h}: fix compilation warnings#1107jimklimov merged 6 commits intonetworkupstools:masterfrom
Conversation
…k before dereferencing
|
Also if possible, can you test the updated driver in master branch against your device, to make sure I did not add typos etc? |
|
I'm sorry for my delayed response. I didn't have the chance to review the committed changes due to my involvement in other business activities. I will check and give some feedback |
|
I have done some testing based on code from master branch. It seems to work fine, however I've made some improvements. Driver now handles connection errors (broken pipe) with modbus tcp server devices. I noticed also that there might be cases with modbus tcp devices, where invalid data are received (shifted data frames) due to tight time intervals between requests. So I've added some code to configure the response time out between requests and between frame bytes. The new code is under testing. When it's done should I commit these modifications within this PR? |
|
Yes, thanks, a PR would be great!
…On Thu, Dec 9, 2021, 18:02 dtsecon ***@***.***> wrote:
I have done some testing based on code from master branch. It seems to
work fine, however I've made some *improvements*. Driver now handles
connection errors (broken pipe) with modbus tcp server devices. I noticed
also that there might be cases with modbus tcp devices, where invalid data
are received (shifted data frames) due to tight time intervals between
requests. So I've added some code to configure the response time out
between requests and between frame bytes. The new code is under testing.
When it's done should I commit these modifications within this PR?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#1107 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMPTFHKEALZOXD2KPLG3NLUQDOL7ANCNFSM5EUMHCRA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
I have tested the code and committed the changes into the current fork (PR). Is that ok or should I open a new PR? |
|
If you mean #1239, that looks OK (further comments pursued there).
If the question is about methodology - you can make many PRs at once from
same fork using different branches, and re-use those branches to iterate on
a subject, but only after an initial PR from such branch got merged and
GitHub no longer tracks it as a source of contributions.
…On Thu, Dec 23, 2021, 17:26 dtsecon ***@***.***> wrote:
I have tested the code and committed the changes into the current fork
(PR). Is that ok or should I open a new PR?
—
Reply to this email directly, view it on GitHub
<#1107 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMPTFHAEZRC6UAJAICEXCLUSNERZANCNFSM5EUMHCRA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Follows up from #1052 to clean up compilation warnings.
CC @dtsecon - in original PR I asked whether generic types like "int" should become specific-bitness types to match wire protocol, etc.? If there is something to fix, please base that over this cleaned-up code :)
Also,
ser_parityis not sanity-checked (at least when received from config); should it be? Any others?UPDATE: CI likes the change so I'll merge it now; feel free to post further PRs based on NUT master branch.