kr0ner / OneESP32ToRuleThemAll

17 stars 5 forks source link

Support more devices, e.g., TTF07 - how to collaborate? #44

Closed mkaiser closed 2 weeks ago

mkaiser commented 3 weeks ago

Hey,

I started a fork for my Tecalor TTF07cool heatpump (same as Stiebel Eltron WPC07c) for geothermal energy with deep drilling and must say, that I am REALLY HAPPY that you created such clean and easily extensible code base!

My main question is: How can I contribute to your project? The discord seems to be orphaned, and there is no "discussion" section available :)

There are a couple of things I had to adjust to get it working

Some things above are not easily merge-able and I would like to brainstorm a bit before I blindly program things ;)

Happy to hear from you!

Martin

kr0ner commented 3 weeks ago

Hi Martin, happy to hear that you find this project useful :) The discord was just created so that we could meet once and discuss some things. Never looked at it again tbh 😅 I had a look at your changes and it should be no problem to integrate them ... although it might be a bit of work.

The first thing I would put on my list is to make the repo easier to use. Actually this has been there for a while but I did not manage to tackle it yet. As you mentioned, a submodule would be the easiest way. Another idea would be to use remote packages so that you don't even have to clone the repo anymore https://esphome.io/components/packages#remote-git-packages

Since the majority of variants will be heatpumps, I'd propose to not move out all the related sensors to establish a common base, but to rather delete the unused ones using https://esphome.io/components/packages#remove in the ttf07c_wpf07cool.yaml Maybe this should anyways be the default. I'd add new features always directly to the base.yaml and based on feedback/PRs from 404, TTF etc exclude the files in their yamls.

Adding properties should be no big deal, since we make use of the preprocessor already

The CAN bus could make use of https://esphome.io/components/packages#extend moving the configuration up to the main yaml like the substitutions.

What do you think? Does this sound like a plan? 🤔

kr0ner commented 3 weeks ago

At least the CAN part was straight forward https://github.com/kr0ner/OneESP32ToRuleThemAll/pull/45

mkaiser commented 3 weeks ago

hi,

wow, this was a quick and extensive reply :)

thanks for the links - I was not aware how versatile packages can be used.

I'd propose to not move out all the related sensors to establish a common base, but to rather delete the unused ones using https://esphome.io/components/packages#remove in the ttf07c_wpf07cool.yaml Maybe this should anyways be the default. I'd add new features always directly to the base.yaml and based on feedback/PRs from 404, TTF etc exclude the files in their yamls.

I am not sure if this has no unwanted side-effects: What if someone adds a new sensor, which works for heatpump A, but not heatpump B? After fetching the newest package, the compilation would fail for B, until it is manually removed .

It would be perfect, if you could just add more and more sensors to a central "all in one yaml" and then actively select, which sensors you want to include (include it for heatpump A, but not B). This is kind of a "manual whitelisting" instead of "manual blacklisting) - but I don't see how to realize this, yet within yaml.

btw. tested #45, works great!

kr0ner commented 3 weeks ago

I am not sure if this has no unwanted side-effects: What if someone adds a new sensor, which works for heatpump A, but not heatpump B? After fetching the newest package, the compilation would fail for B, until it is manually removed .

Not sure if that would really be a problem if we pay attention to the workflow. Nevertheless it would make sense to finally add some tests and run them as pre-commit hook.

It would be perfect, if you could just add more and more sensors to a central "all in one yaml" and then actively select, which sensors you want to include (include it for heatpump A, but not B). This is kind of a "manual whitelisting" instead of "manual blacklisting) - but I don't see how to realize this, yet within yaml.

It is not possible to cherry-pick elements from a yaml. You either get all of the entities or none. So in order to not having to duplicate every sensor for every variant I'm still in favor of the explicit removal for the non-heat pumps. I'll prepare a PR an you can check it out.

kr0ner commented 3 weeks ago

Explicit removal does not work nicely, because the on_boot lambdas would still be there. Also some template-yamls create more than one sensor 🤷 @mkaiser please have a look at #46 ... should address almost all of you changes. Betriebstatus still contains the vent specific stuff, but at least it won't show up in your HA since those sensors are marked as internal now

mkaiser commented 2 weeks ago

Hey, thanks a lot! I was just able to compile a new version based on your current branch [split_yamls_again] --> mybranch

Re-reading the posts, I realized how sloppy I wrote this:

I am not sure if this has no unwanted side-effects: What if someone adds a new sensor, which works for heatpump A, but not heatpump B? After fetching the newest package, the compilation would fail for B, until it is manually removed .

It is not a compilation issue - it would compile, but it just won't work at run-time.

At the moment only some sensors are working fine for me - Temperature sensors and the "Leistungs / Ertrags" -sensors. I guess I will need a couple of weeks to figure all that out in detail (e.g. how to set the "Betriebszustand" stuff) then it should be easy to upstream all this into your repo :)

Discussion point A: My directory structure looks like this with a git-submodule with your repository.

/esphome/ttf07c.yaml /esphome/git_OneESP32ToRuleThemAll/

I had to move the esphome: includes: from common.yaml one level up to my ttf07-main.yaml

esphome:
  name: ttf07c
  friendly_name: ttf07c
  platformio_options:
      ... 
  includes: 
    - git_OneESP32ToRuleThemAll/src/callback_handler.h
    - git_OneESP32ToRuleThemAll/src/communication.h
    - git_OneESP32ToRuleThemAll/src/custom_climate.h
    - git_OneESP32ToRuleThemAll/src/mapper.h
    - git_OneESP32ToRuleThemAll/src/mapper.cpp
    - git_OneESP32ToRuleThemAll/src/property.h
    - git_OneESP32ToRuleThemAll/src/simple_variant.h
    # - git_OneESP32ToRuleThemAll/src/sml_reader.h
    - git_OneESP32ToRuleThemAll/src/type.h
    - git_OneESP32ToRuleThemAll/src/type.cpp

more stuff including CAN config 

packages:
  ttf07: !include git_OneESP32ToRuleThemAll/yaml/ttf07.yaml

Would it be okay for your to move these includes in your esp32-poe-technik.yaml ?

Discussion point B: There are some minor issues I came across and made separate commits in my branch here.

What do you think? Should I make some PRs for this, or would you directly integrate it, as soon as the yaml-split is complete?

Discussion point C: Should we continue discussing these things in this thread, or move on to separate issues when the major restructuring is completed?

Discussion point D:

There are a couple of UNKNOWN messages in the log, e.g.,

[23:15:06][I][Communication:081]: Message received: Read/Write ID 0x60 0x79 for property UNKNOWN (0x005E) with raw value: 0x0000 (dec=0)
[23:15:06][I][CallbackHandler:036]: Callback not found for Kessel UNKNOWN (0x005e)
[23:15:06][I][Communication:081]: Message received: Read/Write ID 0x60 0x79 for property UNKNOWN (0x0053) with raw value: 0x0000 (dec=0)
[23:15:06][I][CallbackHandler:036]: Callback not found for Kessel UNKNOWN (0x0053)
[23:15:06][I][Communication:081]: Message received: Read/Write ID 0x60 0x79 for property UNKNOWN (0x0A00) with raw value: 0x0028 (dec=40)
[23:15:06][I][CallbackHandler:036]: Callback not found for Kessel UNKNOWN (0x0a00)
[23:15:06][I][Communication:081]: Message received: Read/Write ID 0x60 0x79 for property UNKNOWN (0x005A) with raw value: 0x0200 (dec=512)
[23:15:06][I][CallbackHandler:036]: Callback not found for Kessel UNKNOWN (0x005a)
[23:15:06][I][Communication:081]: Message received: Read/Write ID 0x60 0x79 for property UNKNOWN (0x000A) with raw value: 0x020B (dec=523)
[23:15:06][I][CallbackHandler:036]: Callback not found for Kessel UNKNOWN (0x000a)
[23:15:06][I][Communication:081]: Message received: Read/Write ID 0x60 0x79 for property UNKNOWN (0x0009) with raw value: 0x1517 (dec=5399)
[23:15:06][I][CallbackHandler:036]: Callback not found for Kessel UNKNOWN (0x0009)
[23:15:06][I][Communication:081]: Message received: Read/Write ID 0x60 0x79 for property UNKNOWN (0x0A20) with raw value: 0x0020 (dec=32)
[23:15:06][I][CallbackHandler:036]: Callback not found for Kessel UNKNOWN (0x0a20)`

[23:15:45][I][Communication:081]: Message received: Read/Write ID 0x66 0x79 for property UNKNOWN (0x00FE) with raw value: 0x0100 (dec=256)
[23:15:45][I][CallbackHandler:036]: Callback not found for Kessel UNKNOWN (0x00fe)
[23:15:46][I][Communication:081]: Message received: Read/Write ID 0xC7 0x01 for property UNKNOWN (0x00FE) with raw value: 0x0100 (dec=256)
[23:15:46][I][CallbackHandler:036]: Callback not found for HK1 UNKNOWN (0x00fe)
[23:15:46][I][Communication:081]: Message received: Read/Write ID 0x37 0x00 for property UNKNOWN (0x00FE) with raw value: 0x0100 (dec=256)
[23:15:46][I][CallbackHandler:036]: Callback not found for HK1 UNKNOWN (0x00fe)

How would you "reverse engineer" these values? I have an ElsterTable.h somewhere, which can be used to compare the address with the register name, but there is no register description (e.g. Which the single bits of "betriebszustand" mean)

kr0ner commented 2 weeks ago

It is not a compilation issue - it would compile, but it just won't work at run-time.

I think that is a minor inconvenience and can be fixed once found. I also don't expect these to happen very often.

Discussion point A:

I'm still working on this one. As proposed above, I'm investigating if/how exernal packages could solve this. If that does not work, we can still go with standard submodule.

Discussion point B:

I'm against this change. There is no reason why hex values should be printed here. The values in display are float or int and it does not help to know the hex value of the data. For property it is different, since we define it in hex.

Note on EL_AUFNAHMELEISTUNG_WW_TAG_WH vs EL_ENERGIEAUFNAHME I agree, feel free to open a PR for that.

Discussion point C:

I'd propose to open a new issue for every finding. Or at least for a group of findings that more or less belong together. Otherwise it is hard to keep track of what has been done and what still needs to be done.

Discussion point D:

Screw the ElsterTable ;) for me most of the things did not match at all and it was harder to find out which were working and which weren't. I would also not try to create a complete list and fix all UNKNOWN properties. Having sensor just for the sake of having a complete config and never looking at them makes no sense and just bloats everthing. Go through the display and check the values of interest. Try to map the value seen in the display e.g. 23.5° to 235 raw value in the logs to find the property ID. Check if they are already available in Property.h ... if yes and the value differs, guard it with a preprocessor statement for your model. If not, add it and if it is likely that the property value is only available for your device, guard it as well. If not, don't.