search

Bouni / luxtronik

Luxtronik integration for Home Assistant
MIT License
82 stars 14 forks source link

Merge project with BenPru/Luxtronik #51

Open AJediIAm opened 1 year ago

AJediIAm commented 1 year ago

https://github.com/BenPru/luxtronik is making a lot of good progress and seems to have surpassed this integration. Please consider merging the two projects so development effort be pulled together and users do not need to choose which one to use.

Bouni commented 1 year ago

@BenPru For me this could absolutely happen! the main issue is that you've for some reason not forked my project but uploaded it as a "new" one to GitHub. I've no idea how to get that to work out properly 🤷🏽

I lack the time to develop this further at the moment but your repo seems to have some traction.

Let me know what you think!

AJediIAm commented 1 year ago

Hi Bouni, I'm just a user who likes to make a small contribution once in a while. I was hoping that if @BenPru agrees, you could become a maintainer of the BenPru Luxtronik project, remove this one from HACS and update the community forum to refer to the BenPru project.

I hope that by joining forces and giving this a bit of a push from the community, your hard work can become part of HA.

BenPru commented 1 year ago

@AJediIAm Thanks for your ticket, great idea. I totally agree. @Bouni I can fork your project and transfer my code into this fork. But we have some things to plan:

How to handle breaking changes:

Stability, codebase and feature completeness

How do you think about this?

BenPru commented 1 year ago

I have tested the current version from bouni and I have seen that the sensor names uses a new scheme. E.g. "sensor.temperature_forerun". I think we can add a "legacy sensor name mode" for sensors created by the yaml configuration.

AJediIAm commented 1 year ago

Just a personal thought: Given the extend of the changes, this might be the right time to make a breaking change and switch to the new sensor names. If users are happy with the current setup, they do not need to upgrade. If users want keep up to date, share code, etc, using the new naming convention will benefit them in the long run.

Bouni commented 1 year ago

Hey, sorry for the long silence 😅

I totally agree with @AJediIAm we should defenitely go the "new sensor names" path as this allows for using the sensors in the energy dashboard for example.

I also would not take the effort of a legacy mode. If someone whants to keep their sensor they still can stay on their version.

My questions:

Should we keep a yaml config mode? I think a good GUI configurable integration is, especially given the thousends of parameter/calculations/visibilities superior to the yaml mode. Is this even possible, I've not yet looked really into the config side of HA integrations ... Selecting the entities one wants to have with a long list of checkboxes would be awesome!

kbabioch commented 1 year ago

At least I'm still in favor of YAML editing and kind of dis-like all of the GUI only integrations. So, if possible, I would definitely prefer to keep YAML editing around :-).

Also I don't see a reason why a dynamic Config Flow only mode should be established, all of the data points are well known and don't change. Also authentication is not an issue like with some other integrations, or am I missing something?

BenPru commented 1 year ago

a good GUI configurable integration Is this even possible

Yes, I think so. The Android TV Integration has something like this.

AJediIAm commented 1 year ago

There are many integrations which have the more complex sensors disabled by default. This is a very user friendly solution in my opinion. It shows you the most relevant stuff which is what 80% of the users need and it does not cluter your interface. More advanced users can still add the sensors by simply enabling them.

Bouni commented 1 year ago

@AJediIAm This is an approach you can only follow if you know what your sensors mean, in our case you have so man unknown sensors that might be important for some heatpump models that I don't think this is feasable for us (unfortunately).