ginkage / MHI-AC-Ctrl-ESPHome

ESPHome integration for MHI-AC-Ctrl project
MIT License
101 stars 35 forks source link

[Feature Request] Make this available as a custom component #55

Closed Kurisutian closed 2 weeks ago

Kurisutian commented 1 year ago

Since the files provided seem to be changing and get updates every now and then I was wondering if it would be possible to convert the repo to be used as custom components within ESPHome. I know that https://github.com/tfyoung/esphome-MHIHeatPump has already done this with another version. However I thought it makes sense maybe here as well so everyone using will get the latest version without the need of manually update all files every time.

Thanks also for the great work making this available in ESPHome. ❤️

RobertJansen1 commented 1 year ago

I have fiddled around with this but i can't seem to get it working. if you know how, please supply a pull request to https://github.com/RobertJansen1/MHI-AC-Ctrl-ESPHome/tree/esphome_component and i will try it

nbeernink commented 11 months ago

Also still interested in this. So I gave it a try and failed. :)

I don't quite understand why the compile works fine if the source .h and .cpp files from this project are next to the yaml file, but as soon as you try to put it in an external dir, and use it as an external component it errors out. Docs seem to suggest that we need custom __init.py and climate.py files in order for esphome to generate additional code for this to work?

https://github.com/thegroove/esphome-custom-component-examples#basic-structure-of-a-custom-component (would be nice if they had an example for than __init__.py file, but I suppose an unofficial example can be seen in tfyoungs version)

I tried to set the files up like this:

.
├── components
│   └── mhi_ac_ctrl
│       ├── MHI-AC-Ctrl-core.cpp
│       ├── MHI-AC-Ctrl-core.h
│       ├── mhi_ac_ctrl.h
└── mhi-custom.yml

I then remove the include and add a clause like this:

  external_components:
    - source:
        type: local
        path: components/mhi_ac_ctrl

esphome compile mhi-custom.yml then errors out like this:

Compiling .pioenvs/mhi-ac-test/src/main.cpp.o
mhi-custom.yml: In lambda function:
mhi-custom.yml:47:30: error: expected type-specifier before 'MhiAcCtrl'
mhi-custom.yml:49:26: error: could not convert '{mhi_ac_ctrl}' from '<brace-enclosed initializer list>' to 'std::vector<esphome::climate::Climate*>'
mhi-custom.yml: In lambda function:
mhi-custom.yml:67:16: error: 'MhiAcCtrl' was not declared in this scope
mhi-custom.yml:67:26: error: expected primary-expression before ')' token
mhi-custom.yml:67:27: error: expected ')' before 'mhi_ac_test'
mhi-custom.yml: In lambda function:
mhi-custom.yml:77:16: error: 'MhiAcCtrl' was not declared in this scope
mhi-custom.yml:77:26: error: expected primary-expression before ')' token
mhi-custom.yml:77:27: error: expected ')' before 'mhi_ac_test'
mhi-custom.yml: In lambda function:
mhi-custom.yml:87:16: error: 'MhiAcCtrl' was not declared in this scope
mhi-custom.yml:87:26: error: expected primary-expression before ')' token
mhi-custom.yml:87:27: error: expected ')' before 'mhi_ac_test'
mhi-custom.yml: In lambda function:
mhi-custom.yml:106:16: error: 'MhiAcCtrl' was not declared in this scope
mhi-custom.yml:106:26: error: expected primary-expression before ')' token
mhi-custom.yml:106:27: error: expected ')' before 'mhi_ac_test'
*** [.pioenvs/mhi-ac-test/src/main.cpp.o] Error 1
majkrzak commented 6 months ago

I usually sneak peek the sources of other similar components. Like here, the harier one might be good to peak https://github.com/esphome/esphome/tree/dev/esphome/components/haier

The *.py files generaly describes how esphome should process the yaml config files to generate the cpp sources out of it.

I believe the error you have is due to you left garbage like bellow in the config file. This should be ditched, and only the climate: part should be left in example config file.

  services:
    # Call the set_api_room_temperature service from HA to override the room temperature
    # If a new value has not been received after room_temp_api_timeout seconds, it will fall back to internal sensor
    - service: set_api_room_temperature
      variables:
        value: float
      then:
        - lambda: |-
            return ((MhiAcCtrl*)id(${deviceid}))->set_room_temperature(value);

It would be good to push it forward as, despite being awesome, this project is kinda useless in current form.

arpiecodes commented 6 months ago

That 'garbage' is there because of limitations in the ESPHome primitives/natives not offering official ways to do things we need to be able to do, such as setting the temp on the unit with a custom value or using the extra vane positions/settings which the Climate component doesn't support (and also has no real way to expand properly). Also, I don't agree it's useless in its current form. It just requires some batteries/instruction to be followed. Which you can learn. Or, you know, just follow instructions. It is not that hard.

Can it be made better? Hell yes. For starters we shouldn't have to copy in library files by hand. Obviously would be great if we can make it into its own component, based on the climate base class. All easier said than done though, as missing support like I mentioned above make it hard to push the pieces of the puzzle into the template given by the ESPHome project.

majkrzak commented 6 months ago

While converting it to the custom component, the example I have cited should get wrapped into an actions like: https://github.com/esphome/esphome/blob/dev/esphome/components/haier/automation.h and exposed with @automation.register_action decorator. Although I'm not sure if this particular does not fit into the actions available on the climate component.

arpiecodes commented 6 months ago

You seem to be at home in ESPHome component layout. Maybe you could help us out and give it a try?

devedse commented 1 month ago

Hey all, it would be amazingly useful if the component could be converted to an external component. I hope you guys can get this implemented.

RobertJansen1 commented 1 month ago

99 should fix this, anyone eager to test this, please do before i merge and break all setups :)