-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[aWATTar] Add aWATTar API class #17169
[aWATTar] Add aWATTar API class #17169
Conversation
4f57c66
to
6346d1b
Compare
What is the benefit of refactoring this to a new class? |
The main idea was to split the functionality for better testing the bridge. I figured it would be beneficial to move functionality into separate classes for better maintainability and testing later on. EDIT: if you want i can split the tests also with this PR, maybe then the intention would be more clear. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some comments.
bundles/org.openhab.binding.awattar/src/main/java/org/openhab/binding/api/AwattarApi.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.awattar/src/main/java/org/openhab/binding/api/AwattarApi.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.awattar/src/main/java/org/openhab/binding/api/AwattarApi.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.awattar/src/main/java/org/openhab/binding/api/AwattarApi.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.awattar/src/main/java/org/openhab/binding/api/AwattarApi.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.awattar/src/main/java/org/openhab/binding/api/AwattarApi.java
Outdated
Show resolved
Hide resolved
...awattar/src/main/java/org/openhab/binding/awattar/internal/handler/AwattarBridgeHandler.java
Outdated
Show resolved
Hide resolved
...awattar/src/main/java/org/openhab/binding/awattar/internal/handler/AwattarBridgeHandler.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.awattar/src/main/resources/OH-INF/i18n/awattar_de.properties
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.awattar/src/main/resources/OH-INF/i18n/awattar_de.properties
Outdated
Show resolved
Hide resolved
127e5f7
to
21dd81f
Compare
From my point of view extracting the API makes sense. As I would not call myself a experienced Java developer (anymore :-) ), I can't comment on nifty details, so take my approval for the general approach :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
One more idea that came to me yesterday evening: Encapsulating away the aWATTar API from the rest of the binding logic would make it much easier to add other energy price providers to the binding (e.g. entsoe), while reusing all the rest of the code. So the aWATTar binding could be extended to a general energy price binding. And as are about to add TimeSeries support also already most of the work for such an universal Energy address binding is already there soon. What do you think @tl-photography ? From my point of view we mainly would need to add an Interface to be implemented by the API providers to make this happen. Ok, configuration of access data (e.g. entsoe API requires an API key) will be needed also. Am I thinking too big now? |
While I do think it would be beneficial to reuse logic between energy price providers, I don't think this is the right approach. We already have different bindings: aWATTar, Energi Data Service, Tibber and an ENTSO-e. They are very different bindings integrating with different services. Merging them would be similar to merging e.g. the Hue, WLED and Govee Lan-API bindings, because they are all related to lighting. openhab/openhab-core#3478 proposes some kind of energy management component, and there is even a proposed architecture. Unfortunately no one has had time (and/or interest) to pick this up yet. |
Hi @jlaur , thanks for the pointer to the energy management component. I just scrolled over the discussion (did not read it in detail), but the idea looks very good to me. And, I have to admit that I am a little bit proud of the fact that the aWATTar binding seems to cover a fair part of the computational aspects (consecutive/nonconsecutive) bestprice time ranges that are discussed there :-). So my assumptions about the typical use cases seemed not to have been completely wrong, although of course further aspects might need to be considered also. The potential problem with picking this up might be that it describes a quite complex architecture that requires a lot of work, and it has to be part of the openHAB core which significantly reduces the number of people being able to do it, while it is much easier to provide one additional binding or modify an existing one. I was reminded of the discussion between OSI and TCP/IP networking in the 1990th, where OSI networking was carefully crafted to cover every networking aspect that might ever be needed by somebody, while TCP/IP just implemented what somebody needed at a certain moment. Guess who won :-) So on the one side we have the aWATTar binding which already exists and covers perhaps 50% of the use cases, but mainly lacks the functionality to work with different price providers. On the other hand, we have the elaborated architecture, which nobody picked up yet. How about this: We make a very simple interface between energy price providers and the calculation service and try to start with this? The calculation service could be taken from the aWATTar binding, and the interface for the price providers, as soon as it exists in the openHAB core, could then be implemented with acceptable effort by the different bindings that already exists. It could be some variant of a TimeSeries with objects like the one described above. |
The idea does not sound that bad thb. I also think that the proposed architecture does sound good and it is a good idea to have this inside openHAB, since this is the future in my opinion. But this is not a short term solution. We could, however, get in touch with the guys from the other bindings and propose an interim solution for this, with using a extensible API interface. I think it would make A LOT of sense to instead develop all separate in parallel together to get the best out of it. |
One more thing @J-N-K: I wanted to add tests to the exception and the "@test/...." part is not resolved there. If yes, is this actually testable with unit tests? |
I had a look at the code of these bindings and i would say, apart from Tibber (which offers also live data and has a little bit more complicated data model), all the others do use more or less the exact same data structure and have similar API architectures. Energi Data Service seems to be more advanced in general, but also quite complex. |
037cef7
to
4f5d21a
Compare
ok, this should be good now. @J-N-K We are going to have conflicts with your open PR. I checked the potential parts an i think i can merge them quickly afterwards. Or would you like to make the changes after my PR? |
Well, TCP/IP has a pretty nice architecture as well, so I would consider that statement a bit exaggerated. 😄
I would need to get back into the details of @kaikreuzer's proposed architecture, but if it's possible to implement a sub part of it without preventing a full implementation later, I don't see any problems with that. Perhaps this discussion should be continued in that issue?
I didn't check it in details, but I think some parts of the Energi Data Service binding might come in handy as well (at least conceptually), as I think it can do more advanced calculations - see for example https://www.openhab.org/addons/bindings/energidataservice/#thing-actions-example. |
You know that the "current" architecture with IP and TCP separated into two layers was actually the second approach, incorporating the learnings from the previous one (without layering)? 😉
Sounds good! My impression is that you are deeper in the topic already than I am, so it would be great if you could start that. From what I learned the last days regarding the discussions about desired legal changes for PV in 2025, this topic will become more and more important.
Checked it, it is more elaborated especially for non-linear loads. Also, the danish price model seems to have more components than the one in Germany, so the architecture should be able to provide an interface for calculation of the total prices out of the spot prices as well. Ok, things are starting to become complex. |
I second @jlaur here. The proposed architecture is not meant to be an initial first step. Rather the vision at the horizon. Naturally, we should go there step by step (and also adapt it on the way while learning). So instead of creating a "temporary solution between different bindings" (you all know that temporary solutions last longest...), all the binding developers should join the discussion on the core issue and help defining suitable interfaces to get started on the topic. I am personally still very much interested in driving it forward and the more activity there is on the issue, the more I'll be forced to involve myself as well. 😄 |
e9143b3
to
8b6d765
Compare
Signed-off-by: Thomas Leber <[email protected]>
8b6d765
to
8877997
Compare
Hi i think I did cover all requests and fixed the conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Signed-off-by: Thomas Leber <[email protected]>
Signed-off-by: Thomas Leber <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
Signed-off-by: Thomas Leber <[email protected]>
Signed-off-by: Thomas Leber <[email protected]>
This PR introduces an class for the aWATTar API for future refactoring and testing.
The class is intended to encapsualtes the API access.
All current tests will still be positive but can later be split into API and Bridge tests.