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

[tacmi] Support JSON-Api #14119

Merged
merged 17 commits into from
Mar 22, 2025
Merged

[tacmi] Support JSON-Api #14119

merged 17 commits into from
Mar 22, 2025

Conversation

cmorty
Copy link
Contributor

@cmorty cmorty commented Dec 30, 2022 •
edited
Loading

This implementation is based on the JSON-API provided by TA.

The API is provided here: https://wiki.ta.co.at/C.M.I._JSON-API

@cmorty cmorty force-pushed the tacmi-json branch 6 times, most recently from f18651a to 58053e3 Compare December 30, 2022 17:04
@cmorty
Copy link
Contributor Author

cmorty commented Dec 30, 2022

@Wolfgang1966, @marvkis : Seems like I finally tricked the Null-Checker into working. The one running in the CI is even more broken then my local one ( 2022-03 ), which was happy with just reassigning to a local variable. I'm looking forward to a code review.

@cmorty cmorty changed the title [tacmi][WIP] Support JSON-Api [tacmi] Support JSON-Api Dec 30, 2022
@lolodomo lolodomo added the enhancement An enhancement or new feature for an existing add-on label Dec 31, 2022
@Wolfgang1966
Copy link

Hi @cmorty , a great thank you for adding the JSON API! Just this morning I fell into a value scaling problem with the CoE interface after updating the firmware of my CMI (KW now are read ten times smaller than they really are, I correct this by a rule at the moment).

Do you have a test version of your binding somewhere to download? I would really like to switch to JSON with my UVR1611 and hence would like to beta-test your binding.

In parallel I start reviewing your PR. But as I would not consider myself a good Java programmer or too familiar with the coding standards of openHAB, so I would be glad if someone else with better knowledge in this topics would review it also.

@cmorty
Copy link
Contributor Author

cmorty commented Jan 1, 2023

@Wolfgang1966 : The build-log says that it should be available under https://openhab.jfrog.io/openhab/libs-pullrequest-local/org/openhab/addons/bundles/org.openhab.binding.tacmi/4.0.0-SNAPSHOT/org.openhab.binding.tacmi-4.0.0-SNAPSHOT.jar - I'm missing a PR-ID in that URL though. If it doesn't work: Ping me and I'll provide you with a jar.

You were automatically added as reviewer (not my choice). I'm a C++ developer. But the CI does quite a lot of checks, thus the most important issue probably is to make sure the code is sane. ;-)

Wolfgang1966
Wolfgang1966 reviewed Jan 3, 2023
View reviewed changes
Copy link

@Wolfgang1966 Wolfgang1966 left a comment

Choose a reason for hiding this comment

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

Dear @cmorty, overall it looks good to me. I found only some small potential improvements.

Thanks again for your efforts

@cmorty cmorty force-pushed the tacmi-json branch 3 times, most recently from ed8f2c8 to 6322086 Compare January 4, 2023 12:50
Wolfgang1966
Wolfgang1966 approved these changes Jan 4, 2023
View reviewed changes
Copy link

@Wolfgang1966 Wolfgang1966 left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@cmorty cmorty mentioned this pull request Jan 4, 2023
Open
lolodomo
lolodomo requested changes Jan 29, 2023
View reviewed changes
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Review part 1 of 2.
Remains doc, handler, Value and tests

@lolodomo lolodomo added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 29, 2023
@Elgin1
Copy link

Elgin1 commented Jan 30, 2023

Hi @cmorty , nice that your're integrating the JSON API into the binding!

Having recently fought with my CMI to get all the values into openHAB with a lengthy Javascript rule myself, I was wondering:
Does your code take into account that you can only query one node per minute and then the next node has to be queried a minute later and so on? I ended up with an interval of 65 sec between reading out each one of my three nodes to get things stable for several days in the end without the CMI complaining about too many requests.

I haven't spotted anything in that direction during the (admittedly short) look I have given your code, hence my question...

@lolodomo lolodomo added the awaiting feedback Awaiting feedback from the pull request author label Mar 7, 2023
@cmorty
Copy link
Contributor Author

cmorty commented Oct 29, 2023

Hi @cmorty , nice that your're integrating the JSON API into the binding!

Having recently fought with my CMI to get all the values into openHAB with a lengthy Javascript rule myself, I was wondering: Does your code take into account that you can only query one node per minute and then the next node has to be queried a minute later and so on? I ended up with an interval of 65 sec between reading out each one of my three nodes to get things stable for several days in the end without the CMI complaining about too many requests.

I haven't spotted anything in that direction during the (admittedly short) look I have given your code, hence my question...

Yes it does. Even has a configuration option. As I updated OpenHab it looks like I actually want to look into this again. ;)

@cmorty cmorty requested a review from a team as a code owner November 1, 2023 20:50
@cmorty cmorty force-pushed the tacmi-json branch 2 times, most recently from 749055a to f0093a2 Compare November 4, 2023 16:52
@cmorty cmorty requested a review from lolodomo November 5, 2023 12:33
@lsiepel
Copy link
Contributor

lsiepel commented Jan 5, 2025 •
edited
Loading

@lsiepel : As far as I understand main ist against 5.0.0-Snapshot, while I rabased against 4.3.x so I can test. Rebasing that branch into main worked without issue, but I don't have a 5.0.0-Instance to test.

This PR is against main (so yes it is towards 5.0.0) You might be able to build against another version as your feature branch is 100+ commits behind. No problem though.

@cmorty
Copy link
Contributor Author

cmorty commented Jan 5, 2025

The last push also fixes some null-checks. Seems like Eclipse evaluates assert (and complains if I add an additional null-check), while OpenHab is compiled without (and complains about a possible null). I rewrote a bit of the code, but didn't test it yet. The code should be trivial enough, but I've thought that before. :) Will test it tomorrow.

lsiepel
lsiepel requested changes Jan 5, 2025
View reviewed changes
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks for moving this forward! Looked at the remaining files, think this is the last review.

cmorty added 5 commits January 6, 2025 15:06
@cmorty
[tacmi] Support JSON-Api
a9fcd73 
This implementation is based on the JSON-API provided by TA.

Signed-off-by: Moritz 'Morty' Strübe <[email protected]>
@cmorty
[tacmi] Add pattern to schema-numeric-ro channel-type
161e75f 
Signed-off-by: Moritz 'Morty' Strübe <[email protected]>
@cmorty
[tacmi] Make verify happy
19a7b15 
Signed-off-by: Moritz 'Morty' Strübe <[email protected]>
@cmorty
[tacmi] Reorganize documentation
9f661c0 
Reorganize the documentation into main topics and separate Schema and
CoE sections (instead of intermingling the to topics).
No modifications but minor adjustments to the headings were made.

Signed-off-by: Moritz 'Morty' Strübe <[email protected]>
@cmorty
[tacmi] Add JSON API documentation
85394e9 
Add documentation on how to use the JSON API.

Signed-off-by: Moritz 'Morty' Strübe <[email protected]>
lsiepel
lsiepel requested changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Have some found left overs and two new comments. Otherwise LGTM.

@lsiepel
Copy link
Contributor

lsiepel commented Feb 19, 2025

@cmorty we are almost there, are you able to fix the last few comments?

lsiepel added 4 commits March 22, 2025 23:09
@lsiepel
Update bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/bi…
abf2323 
…nding/tacmi/internal/json/TACmiJsonHandler.java

Signed-off-by: lsiepel <[email protected]>
@lsiepel
Update bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/bi…
c404dfe 
…nding/tacmi/internal/json/TACmiJsonHandler.java

Signed-off-by: lsiepel <[email protected]>
@lsiepel
Update bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/bi…
bbd4022 
…nding/tacmi/internal/json/TACmiJsonHandler.java

Signed-off-by: lsiepel <[email protected]>
@lsiepel
Update bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/bi…
501380a 
…nding/tacmi/internal/json/TACmiJsonHandler.java

Signed-off-by: lsiepel <[email protected]>
lsiepel
lsiepel approved these changes Mar 22, 2025
View reviewed changes
Copy link
Contributor

@lsiepel lsiepel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Added 4 commits to resolve my last own comments. Removing logging etc.
@jlaur are you able to verify the last changes i made. I just don't want to merge my own changes, even though they are very small.

Edit: arg i ddidn;t expect these new license header issues. Let me fix that too.

@jlaur
Copy link
Contributor

jlaur commented Mar 22, 2025

@jlaur are you able to verify the last changes i made. I just don't want to merge my own changes, even though they are very small.

The last four commits so far LGTM.

lsiepel
lsiepel reviewed Mar 22, 2025
View reviewed changes
Copy link
Contributor

@lsiepel lsiepel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Fixing the headers (the fork is 2500 commits behind)

lsiepel added 8 commits March 22, 2025 23:28
@lsiepel
Update bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/bi…
ff3672c 
…nding/tacmi/internal/json/obj/Data.java

Signed-off-by: lsiepel <[email protected]>
@lsiepel
Update bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/bi…
2c5ae97 
…nding/tacmi/internal/json/Config.java

Signed-off-by: lsiepel <[email protected]>
@lsiepel
Update bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/bi…
3958bae 
…nding/tacmi/internal/json/TACmiJsonHandler.java

Signed-off-by: lsiepel <[email protected]>
@lsiepel
Update bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/bi…
cacb61b 
…nding/tacmi/internal/json/obj/IO.java

Signed-off-by: lsiepel <[email protected]>
@lsiepel
Update bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/bi…
eb2d9db 
…nding/tacmi/internal/json/obj/Value.java

Signed-off-by: lsiepel <[email protected]>
@lsiepel
Update bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/bi…
d6b6dc0 
…nding/tacmi/internal/json/obj/JsonResponse.java

Signed-off-by: lsiepel <[email protected]>
@lsiepel
Update bundles/org.openhab.binding.tacmi/src/main/java/org/openhab/bi…
d75a59a 
…nding/tacmi/internal/json/obj/Header.java

Signed-off-by: lsiepel <[email protected]>
@lsiepel
Update bundles/org.openhab.binding.tacmi/src/test/java/org/openhab/bi…
f7f9c31 
…nding/tacmi/internal/json/obj/test/DeSer.java

Signed-off-by: lsiepel <[email protected]>
@lsiepel lsiepel merged commit 5a3a8bb into openhab:main Mar 22, 2025
2 checks passed
@lsiepel lsiepel added this to the 5.0 milestone Mar 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsiepel lsiepel lsiepel approved these changes

@Wolfgang1966 Wolfgang1966 Wolfgang1966 approved these changes

@marvkis marvkis Awaiting requested review from marvkis marvkis is a code owner

@lolodomo lolodomo Awaiting requested review from lolodomo

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

Successfully merging this pull request may close these issues.
7 participants
@cmorty @Wolfgang1966 @Elgin1 @lsiepel @jlaur @lolodomo @marvkis