Skip to content

Add CCL Electronics integration #130281

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

Open
wants to merge 41 commits into
base: dev
Choose a base branch
from
Open

Add CCL Electronics integration #130281

wants to merge 41 commits into from

Conversation

fkiscd
Copy link

@fkiscd fkiscd commented Nov 10, 2024

Proposed change

Adds the CCL Electronics integration, allowing local weather stations with expandable modules to push sensor data.

Link to library: https://github.com/CCL-Electronics-Ltd/aioccl

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @fkiscd

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

@mikey0000
Copy link
Contributor

mikey0000 commented Nov 10, 2024

Need to only PR one platform at a time.

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @fkiscd

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @fkiscd

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @fkiscd

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

entry.runtime_data = CCLDevice(entry.data["passkey"])

CCLServer.add_copy(entry.runtime_data)
await CCLServer.run()
Copy link
Member

Choose a reason for hiding this comment

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

you are launching your own webserver here which is not something we want. There is a built in webhook component that can be used to allow push style HTTP callbacks, take a look at tedee for how those are registered. Also, is there any way you can push the instance details to CCL, instead of the user having to go to the app to configure that manually?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the advice. It is now changed to listen for requests using webhook api.
Here is the test command I input to the zsh terminal in dev container (about half of them are binary sensors which are not included in this PR):
curl localhost:8123/api/webhook/c2507495 -X POST -H 'Content-Type: application/json' -d '{"serial_no": "1061616322","mac_address": "48:31:B7:06:D5:59","model":"HA100","fw_ver":"v1.00","datetime":"2024-12-26+02:47:46","abar":1014.1,"rbar":1012.9,"t1dew":16.1,"t1feels":22.9,"t1chill":22.9,"inhum":63,"intem":23.0,"t1solrad":0,"t1hum":66,"t1tem":22.9,"t1rainra":0,"t1raindy":0,"t1rainhr":0,"t1rainmth":0,"t1rainwy":0,"t1rainyr":0,"t1uvi":0,"t1wdir":276,"t1wgust":0,"t1ws":0,"t1ws10mav":0,"inbat":1,"t1bat":0,"t234c1cn":0,"t234c2cn":0,"t234c3cn":0,"t234c4cn":0,"t234c5cn":0,"t234c6cn":0,"t234c7cn":0,"t11cn":0,"t10cn":0,"t9cn":0,"t6c1cn":0,"t6c2cn":0,"t6c3cn":0,"t6c4cn":0,"t6c5cn":0,"t6c6cn":0,"t6c7cn":0,"t5lscn":0,"t1cn":1,"t8cn":0,"api_ver":"v1.00"}'

@home-assistant home-assistant bot marked this pull request as draft December 17, 2024 14:31
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

There are still a couple of comments not addressed

@home-assistant home-assistant bot marked this pull request as draft May 14, 2025 18:54
@fkiscd fkiscd marked this pull request as ready for review May 18, 2025 21:27
@home-assistant home-assistant bot requested a review from joostlek May 18, 2025 21:27
@home-assistant home-assistant bot marked this pull request as draft May 22, 2025 09:35
@fkiscd fkiscd marked this pull request as ready for review May 25, 2025 18:44
@home-assistant home-assistant bot requested a review from joostlek May 25, 2025 18:44
@fkiscd
Copy link
Author

fkiscd commented Jun 15, 2025

@joostlek Is there any improvement I could make? Since our customers have been waiting for a long time. Sorry for bothering.

@joostlek
Copy link
Member

Yea so I was discussing this PR with @zweckj and we kinda come to the conclusion that the current setup could work in the end, but with how it works now, it's very complex and I kinda think that this is the moment to push back and look how we can improve this so we can both have a maintainable integration with is also easily extendable.

Currently some things are vague, like the library has a few too much tasks. Why should the library care about what sensors or binary sensors it creates? In theory the library should just process and return data, and HA should create the right entities from that.

@joostlek joostlek marked this pull request as draft June 16, 2025 21:57
@fkiscd fkiscd marked this pull request as ready for review June 22, 2025 05:51
@fkiscd
Copy link
Author

fkiscd commented Jun 22, 2025

Yea so I was discussing this PR with @zweckj and we kinda come to the conclusion that the current setup could work in the end, but with how it works now, it's very complex and I kinda think that this is the moment to push back and look how we can improve this so we can both have a maintainable integration with is also easily extendable.

Currently some things are vague, like the library has a few too much tasks. Why should the library care about what sensors or binary sensors it creates? In theory the library should just process and return data, and HA should create the right entities from that.

Thank you, moved that feature out from the library. Now the integration will be the one to check if a sensor is binary.

Comment on lines 26 to 33
test-before-configure:
status: exempt
comment: |
The integration only receives and reads data from devices, no connection is needed.
test-before-setup:
status: exempt
comment: |
The integration only receives and reads data from devices, no connection is needed.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite happy with that tbh. How would we know a user did not make a mistake when entering the things in the app? Isn't the app pushing something immediately after first setup at least?

Comment on lines +51 to +54
log-when-unavailable:
status: exempt
comment: |
The integration has no initiative access to the device.
Copy link
Member

Choose a reason for hiding this comment

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

or rather than in the entities, maybe we raise UpdateFailed from the coordinator if we didn't receive an update for more than 5 minutes

Copy link
Author

Choose a reason for hiding this comment

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

The integration now tests if data is pushed with async_config_entry_first_refresh(). I also tried to set the coordinator update interval to 10 mins to raise UpdateFailed on time, but somehow polling is not working. _async_update_data() is only triggered on set-up and never again even manually, which I have no idea at all.

Copy link
Member

Choose a reason for hiding this comment

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

and never again even manually without looking at the code: that happens when we have no entities subscribing to the coordiantor as listeners

CCLSensorTypes.UVI: SensorEntityDescription(
key="UVI",
state_class=SensorStateClass.MEASUREMENT,
translation_key="uvi",
Copy link
Member

Choose a reason for hiding this comment

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

still valid

@home-assistant home-assistant bot marked this pull request as draft June 23, 2025 19:04
@fkiscd fkiscd marked this pull request as ready for review June 30, 2025 19:40
@home-assistant home-assistant bot requested a review from zweckj June 30, 2025 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants