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.

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

[vallox] Vallox Binding initial contribution #6459

Closed
wants to merge 22 commits into from

Conversation

gitMiguel
Copy link

@gitMiguel gitMiguel commented Nov 27, 2019

Vallox Binding initial contribution.

Continuing https://github.com/openhab/openhab2-addons/pull/1560. See also discussion about different models of vallox units https://github.com/openhab/openhab2-addons/pull/2990.
This is a partial rewrite from #1560 and my goal is to have the Vallox MV binding combined to this so then we would have a single Vallox binding. Work is still in progress but as a beginner with java I would like a maintainers review/opinion and guide me to the right direction if needed.
@SuperOok can you take a look and give your comments as this is your original work.
Also tagging @bjoernbrings to get some comments about combining these two.

Download from jfrog: org.openhab.binding.vallox-2.5.7-SNAPSHOT.jar
Or install from marketplace: Exlipse Markeplace Vallox binding
Discussion: community.openhab.org/t/new-vallox-binding-wip/86425

Todo:

  • Readme
  • Integrate Vallox MV Binding
  • Test serial communication
  • Finalize serial communication.
  • Modify cache.
  • Provide download link for testing. Eclipse marketplace and direct link.

Signed-off-by: Miika Jukka miikajukka@gmail.com (github: gitMiguel)

Closes #1559

@gitMiguel gitMiguel requested a review from a team as a code owner November 27, 2019 12:27
@wborn wborn added new binding If someone has started to work on a binding. For a new binding PR. work in progress A PR that is not yet ready to be merged labels Nov 27, 2019
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/new-vallox-binding-wip/86425/1

@gitMiguel
Copy link
Author

gitMiguel commented Dec 1, 2019

Hi,

As you can see I made a new commit and I'm almost done. I finalized the serial connection part but haven't had a chance to test it properly. Getting serial port over ethernet to work has given me headaches. When setting serial port parameters the implementation is throwing UnsupportedCommOperationException. Maybe @Hilbrand you could take a look if it might be something in my code that's not working. The code is much based on DSMR binding.

/Miika

Edit:

Looks like serial port parameters are in the wrong order 🤦‍♂️
I'll also adjust threshold and timeout values.

@Hilbrand
Copy link
Member

Hilbrand commented Dec 2, 2019

I never implemented a fix in the DSMR binding. I stranded in a discussion on how this should be implemented eclipse-archived/smarthome#6280 I still think this should be handled in core. In another binding they simply wrapped the problematic methods. See #4372

@gitMiguel
Copy link
Author

I never implemented a fix in the DSMR binding. I stranded in a discussion on how this should be implemented eclipse/smarthome#6280 I still think this should be handled in core. In another binding they simply wrapped the problematic methods. See #4372

Hmm I got this working with: Vallox ->RS485 ->USB -> RPi3 -> ser2net -> My desktop running Win 10 with HW Virtual serial port VSP3 -> Eclipse IDE.

I'm quite unfamiliar with low level serial implementation in java but looks like it's using RxTx and both enableReceiveThreshold() and enableReceiveTimeout() are working as expected.

@gitMiguel
Copy link
Author

gitMiguel commented Dec 4, 2019

@bjoernbrings could you take a look at this and see if you approve the way that I combined these two bindings. I also formatted all channels and groups to lowerCamelCase. Should this also be done to MV channels or is it a breaking change? Check also the README and valloxmv_de.properties.

@gitMiguel gitMiguel force-pushed the master branch 2 times, most recently from 08bb923 to d49c873 Compare December 7, 2019 22:54
@gitMiguel gitMiguel changed the title [vallox][WIP] Vallox Binding initial contribution [vallox] Vallox Binding initial contribution Dec 9, 2019
@gitMiguel
Copy link
Author

gitMiguel commented Dec 9, 2019

I'm done so far and ready for reviews.

I'll leave UoM implementation for later.

Edit:
Squashed all commits and rebased onto master.

@bjoernbrings
Copy link
Contributor

Seems to me like a merge of a new binding (for the old system) with 'my' old binding (for the new system) without any change in or reuse of overlapping functionality. I'm totally fine with it.

Though I'm in principle a bit unsure what should should apply when there should be one binding or several

  • one per vendor => this merge is the way to go
  • only merge when there is overlapping functionality => we should keep it seperate

I'm personally happy with both solutions

@gitMiguel gitMiguel changed the base branch from master to 2.5.x January 9, 2020 08:30
@gitMiguel
Copy link
Author

The binding count only increases over time and merging these two keeps the count one less?

@gitMiguel gitMiguel force-pushed the master branch 2 times, most recently from 4d8166c to 1fab8f3 Compare January 9, 2020 10:01
@bjoernbrings bjoernbrings added (potentially) not backward compatible and removed work in progress A PR that is not yet ready to be merged labels Jan 9, 2020
@gitMiguel
Copy link
Author

@bjoernbrings what is the potential issue with backwards compatibility?

@Hilbrand
Copy link
Member

The thing id changes from valloxmv to vallox. Meaning users have to recreate the things. This means not backward compatible. Meaning if the valloxmv is going to be merged in here the original binding should be moved (see comment below) because it's not desired to have 2 versions of the same code here. hence the it's not backward compatible.

The additions between this binding and the valloxmv binding doesn't seem to share much code. So it's not conclusive if this should be combined. However if it's going to be shared I would prefer to first move the valloxmv binding to the new thing id and put valloxmv code in a specific valloxmv package and in a second commit or pull request put the se specific code in a valloxse (or us just vm and se) package to better separate which code belongs to what. With the current pr you basically let the reviewers review the complete original valloxmv for a second time, because it's very time consuming to figure out what is from the original code and what is added.

@gitMiguel
Copy link
Author

Understood but who makes the decision then? Might be better to leave these separated.

@openhab openhab deleted a comment from TravisBuddy Jan 13, 2020
@openhab openhab deleted a comment from TravisBuddy Jan 13, 2020
@openhab openhab deleted a comment from TravisBuddy Jan 13, 2020
@openhab openhab deleted a comment from TravisBuddy Jan 13, 2020
@openhab openhab deleted a comment from TravisBuddy Jan 13, 2020
@TravisBuddy
Copy link

Travis tests were successful

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

gitMiguel and others added 22 commits July 1, 2020 13:21
Signed-off-by: Miika Jukka <miikajukka@gmail.com>
Signed-off-by: Miika Jukka <miikajukka@gmail.com>
Signed-off-by: Miika Jukka <miikajukka@gmail.com>
Signed-off-by: Miika Jukka <miikajukka@gmail.com>
- Fix log statement and remove null check suppressions

Signed-off-by: Miika Jukka <miikajukka@gmail.com>
- ChannelMapper from Map to Enum (ChannelDescriptor)
- Remove excess null suppressions
- Remove ByteBuffer
- Remove leftover try-catch clauses
- Change cache map key from Byte to ChannelDescriptor
- Change runnables to private methods and apply method reference
- Convert to primitives where applicable
- Logging and thing status fixes
- Bump version to 2.5.4-SNAPSHOT

Signed-off-by: Miika Jukka <miikajukka@gmail.com>
Signed-off-by: Miika Jukka <miikajukka@gmail.com>
Signed-off-by: Miika Jukka <miikajukka@gmail.com>
... null check suppression from sendTelegram()

Signed-off-by: Miika Jukka <github@lantee.eu>
- Remove scheduler from connectors and use dedicated threads
- Switch to atomic booleans
- Remove efficiency channels. Calculation formula outdated.
- Address ChannelDescriptor with static maps
- Pass ValloxConnector from HandlerFactory and remove ConnectorFactory
- Remove unused methods and constants
- Ensure that Cache returns null where values are missing
- Other smaller fixes

Signed-off-by: Miika Jukka <github@lantee.eu>
...temperature, humidity and DC fan adjustment channels.

Signed-off-by: Miika Jukka <github@lantee.eu>
Signed-off-by: Miika Jukka <github@lantee.eu>
Signed-off-by: Miika Jukka <github@lantee.eu>
Signed-off-by: Miika Jukka <github@lantee.eu>
- Clean constants file

Signed-off-by: Miika Jukka <github@lantee.eu>
Signed-off-by: Miika Jukka <github@lantee.eu>
Signed-off-by: Miika Jukka <github@lantee.eu>
Signed-off-by: Miika Jukka <github@lantee.eu>
Signed-off-by: Miika Jukka <github@lantee.eu>
Signed-off-by: Miika Jukka <github@lantee.eu>
Signed-off-by: Miika Jukka <github@lantee.eu>
Signed-off-by: Miika Jukka <github@lantee.eu>
@TravisBuddy
Copy link

Travis tests were successful

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

@gitMiguel
Copy link
Author

Sorry but I don't have any time to finish this. Still hoping to get back and finish it somewhere in the future.

@gitMiguel gitMiguel closed this Aug 24, 2020
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/vallox-se-binding/86425/59

@gitMiguel gitMiguel deleted the master branch September 10, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR. (potentially) not backward compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vallox] Add Vallox Binding
8 participants