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.sbc] Initial contribution #9174

Merged
merged 13 commits into from
Feb 2, 2021
Merged

[modbus.sbc] Initial contribution #9174

merged 13 commits into from
Feb 2, 2021

Conversation

fwolter
Copy link
Member

@fwolter fwolter commented Nov 29, 2020

Signed-off-by: Fabian Wolter [email protected]

@fwolter fwolter added the new binding If someone has started to work on a binding. For a new binding PR. label Nov 29, 2020
@fwolter fwolter requested a review from ssalonen November 29, 2020 19:44
ssalonen
ssalonen requested changes Nov 30, 2020
View reviewed changes
Copy link
Contributor

@ssalonen ssalonen left a comment

Choose a reason for hiding this comment

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

Good job, very clean contribution and could act as a template for the simple read-only modbus bindings.

Some changes suggested.

bundles/org.openhab.binding.modbus.sbc/README.md Outdated Show resolved Hide resolved
...ab.binding.modbus.sbc/src/main/java/org/openhab/binding/modbus/sbc/internal/ALD1Handler.java Outdated Show resolved Hide resolved
...ab.binding.modbus.sbc/src/main/java/org/openhab/binding/modbus/sbc/internal/ALD1Handler.java Outdated Show resolved Hide resolved
....binding.modbus.sbc/src/main/java/org/openhab/binding/modbus/sbc/internal/ALD1Registers.java Outdated Show resolved Hide resolved
bundles/org.openhab.binding.modbus.sbc/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.modbus.sbc/src/main/resources/OH-INF/config/config.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.modbus.sbc/src/main/resources/OH-INF/config/config.xml Outdated Show resolved Hide resolved
@fwolter fwolter added the awaiting other PR Depends on another PR label Nov 30, 2020
@fwolter fwolter requested a review from a team as a code owner December 2, 2020 16:42
@fwolter fwolter force-pushed the sbc branch 2 times, most recently from 852005e to 3ec282d Compare December 2, 2020 16:59
@ssalonen
Copy link
Contributor

ssalonen commented Dec 4, 2020

Lgtm!

@cpmeister cpmeister removed the awaiting other PR Depends on another PR label Dec 5, 2020
cpmeister
cpmeister approved these changes Dec 5, 2020
View reviewed changes
Copy link
Contributor

@cpmeister cpmeister left a comment

Choose a reason for hiding this comment

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

LGTM

@cpmeister cpmeister added the cre Coordinated Review Effort label Dec 5, 2020
@fwolter
Copy link
Member Author

fwolter commented Dec 6, 2020

@openhab/add-ons-maintainers As @cpmeister reviewed this PR and I created it, we need another reviewer here.

andrewfg
andrewfg reviewed Dec 23, 2020
View reviewed changes
bundles/org.openhab.binding.modbus.sbc/src/main/resources/OH-INF/thing/thing-types.xml
<thing-type id="ald1Unidirectional">
<supported-bridge-type-refs>
<bridge-type-ref id="serial"/>
</supported-bridge-type-refs>
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to consider adding support for the RTU over TCP ( #9435 ) by adding the following here..

<bridge-type-ref id="tcp"/>

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. People can use serial/ethernet gateways (passing modbus rtu over tcp) or modbus gateways (converting modbus rtu to modbus tcp) with these devices

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't aware this is possible. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Is adding tcp bridge something that will be added here?

@andrewfg
Copy link
Contributor

FYI: The “RTU over TCP” revisions have now been merged. So this one should be able to build again.

@ssalonen ssalonen added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Dec 28, 2020
@fwolter fwolter added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Dec 28, 2020
@wborn wborn added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 22, 2021
Hilbrand
Hilbrand reviewed Feb 1, 2021
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.

Code looks ok. Just needs update of copyright year and maybe adding that tcp bridge?

bundles/org.openhab.binding.modbus.sbc/src/main/resources/OH-INF/thing/thing-types.xml
<thing-type id="ald1Unidirectional">
<supported-bridge-type-refs>
<bridge-type-ref id="serial"/>
</supported-bridge-type-refs>
Copy link
Member

Choose a reason for hiding this comment

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

Is adding tcp bridge something that will be added here?

@Hilbrand Hilbrand self-assigned this Feb 1, 2021
fwolter added 12 commits February 1, 2021 18:56
@fwolter
Incorporate review feedback #1
b79f818 
Signed-off-by: Fabian Wolter <[email protected]>
@fwolter
Clean address
b6b1421 
Signed-off-by: Fabian Wolter <[email protected]>
@fwolter
Adoptions to Modbus PR
c421e6b 
Signed-off-by: Fabian Wolter <[email protected]>
@fwolter
Change wording
e9239dd 
Signed-off-by: Fabian Wolter <[email protected]>
@fwolter
Refactor float/BigDecimal
b230f08 
Signed-off-by: Fabian Wolter <[email protected]>
@fwolter
Fix import
762f2b8 
Signed-off-by: Fabian Wolter <[email protected]>
@fwolter
Change wording
add8ed0 
Signed-off-by: Fabian Wolter <[email protected]>
@fwolter
Add ability for TCP bridge. Apply moving of modbus transport to core.…
6b66f40 
… Apply renaming of Units. Remove feature file.

Signed-off-by: Fabian Wolter <[email protected]>
@fwolter
Fix OH version
5483bd2 
Signed-off-by: Fabian Wolter <[email protected]>
@fwolter
Fixed signedness for the power factor
1cecdc4 
Signed-off-by: Fabian Wolter <[email protected]>
@fwolter
Add TCP bridge
76d3586 
Signed-off-by: Fabian Wolter <[email protected]>
@fwolter
Update license headers
bc20f17 
Signed-off-by: Fabian Wolter <[email protected]>
@fwolter
Copy link
Member Author

fwolter commented Feb 1, 2021

Forgot to add it to the unidirectional meter, too. Thanks! The license headers are also fixed.

Hilbrand
Hilbrand approved these changes Feb 2, 2021
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 Hilbrand merged commit 0be7f60 into openhab:main Feb 2, 2021
@Hilbrand Hilbrand added this to the 3.1 milestone Feb 2, 2021
@Hilbrand Hilbrand removed the cre Coordinated Review Effort label Feb 2, 2021
lucacalcaterra pushed a commit to lucacalcaterra/openhab-addons that referenced this pull request Feb 23, 2021
@fwolter @lucacalcaterra
[modbus.sbc] Initial contribution (openhab#9174)
a4e77b3 
Signed-off-by: Fabian Wolter <[email protected]>
Signed-off-by: Luca Calcaterra <[email protected]>
@fwolter fwolter deleted the sbc branch April 11, 2021 18:10
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
@fwolter @themillhousegroup
[modbus.sbc] Initial contribution (openhab#9174)
e7efd07 
Signed-off-by: Fabian Wolter <[email protected]>
Signed-off-by: John Marshall <[email protected]>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
@fwolter @thinkingstone
[modbus.sbc] Initial contribution (openhab#9174)
f264d7c 
Signed-off-by: Fabian Wolter <[email protected]>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
@fwolter
[modbus.sbc] Initial contribution (openhab#9174)
ba0cb35 
Signed-off-by: Fabian Wolter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewfg andrewfg andrewfg left review comments

@ssalonen ssalonen ssalonen approved these changes

@Hilbrand Hilbrand Hilbrand approved these changes

@cpmeister cpmeister cpmeister approved these changes

Assignees

@Hilbrand Hilbrand

Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.
6 participants
@fwolter @ssalonen @andrewfg @cpmeister @Hilbrand @wborn