Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[modbus] Validate poller count and write count according to modbus spec #8370

Merged
merged 5 commits into from
Sep 12, 2020
Merged

[modbus] Validate poller count and write count according to modbus spec #8370

merged 5 commits into from
Sep 12, 2020

Conversation

ssalonen
Copy link
Contributor

Related to discussion in #8228

Signed-off-by: Sami Salonen [email protected]

@ssalonen ssalonen added the work in progress A PR that is not yet ready to be merged label Aug 30, 2020
@ssalonen ssalonen requested a review from a team as a code owner August 30, 2020 18:04
@ssalonen
Copy link
Contributor Author

WIP. Needs to be tested with real setup as well. Especially data thing going offline/online properly when poller is invalid/valid.

@TravisBuddy
Copy link

Travis tests were successful

Hey @ssalonen,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@Hilbrand Hilbrand added the enhancement An enhancement or new feature for an existing add-on label Aug 31, 2020
@ssalonen @Hilbrand
[modbus] simplified array sorting
a05a187 
Signed-off-by: Sami Salonen <[email protected]>

Co-authored-by: Hilbrand Bouwkamp <[email protected]>
ssalonen referenced this pull request Aug 31, 2020
@Rossko57
Update README.md
68ed85b 
Small changes to 'length' and 'Discovery' descriptions.
@ssalonen ssalonen closed this Sep 6, 2020
@ssalonen ssalonen reopened this Sep 6, 2020
@ssalonen
Copy link
Contributor Author

ssalonen commented Sep 6, 2020

Travis failed due spotless issues, need to run the formatting.

@TravisBuddy
Copy link

Travis tests have failed

Hey @ssalonen,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@TravisBuddy
Copy link

Travis tests were successful

Hey @ssalonen,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@ssalonen
[modbus] spotless
d4c6f52 
Signed-off-by: Sami Salonen <[email protected]>
@ssalonen
[modbus] fix import
24004da 
Signed-off-by: Sami Salonen <[email protected]>
@ssalonen ssalonen force-pushed the modbus-read-count-validation branch from ab2526b to 24004da Compare September 7, 2020 17:29
@TravisBuddy
Copy link

Travis tests were successful

Hey @ssalonen,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Hilbrand
Hilbrand approved these changes Sep 7, 2020
View reviewed changes
Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Hilbrand
Copy link
Member

Hilbrand commented Sep 7, 2020

When you're done testing, please remove the work-in-progress and we can merge this pr.

@ssalonen
Copy link
Contributor Author

ssalonen commented Sep 11, 2020 •
edited
Loading

According to further testing and reading the spec. Limit is in fact 123 (not 125 like in the PR) registers with modbus tcp. Spec says with RTU it is 125. We might simply limit all to 123. Need to change and document this.

Online/offline thing handling seems to work well itself. Poller status is propagated correctly to data thing.

EDIT: Internet is full of mixed information on this. Following the official spec here.

@ssalonen
[modbus] documented where the limits come from.
ec096a1 
Internet is full of interpretation and variations. We follow the spec.

Signed-off-by: Sami Salonen <[email protected]>
@ssalonen ssalonen removed the work in progress A PR that is not yet ready to be merged label Sep 12, 2020
@ssalonen
Copy link
Contributor Author

This is good to merge once the tests pass.

@Hilbrand Hilbrand merged commit 314c283 into openhab:2.5.x Sep 12, 2020
@Hilbrand Hilbrand added this to the 2.5.9 milestone Sep 12, 2020
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020
@ssalonen @CSchlipp
[modbus] Validate poller count and write count according to modbus sp…
2bcb119 
…ec (openhab#8370)

* Related to discussion in openhab#8228
* Documented where the limits come from.
* Internet is full of interpretation and variations. We follow the spec.

Signed-off-by: Sami Salonen <[email protected]>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
@ssalonen @markus7017
[modbus] Validate poller count and write count according to modbus sp…
4dd626b 
…ec (openhab#8370)

* Related to discussion in openhab#8228
* Documented where the limits come from.
* Internet is full of interpretation and variations. We follow the spec.

Signed-off-by: Sami Salonen <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Oct 8, 2020
@ssalonen @andrewfg
[modbus] Validate poller count and write count according to modbus sp…
7335562 
…ec (openhab#8370)

* Related to discussion in openhab#8228
* Documented where the limits come from.
* Internet is full of interpretation and variations. We follow the spec.

Signed-off-by: Sami Salonen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hilbrand Hilbrand Hilbrand approved these changes

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
2.5.9
Development

Successfully merging this pull request may close these issues.
3 participants
@ssalonen @TravisBuddy @Hilbrand