Skip to content
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

Make a Home Assistant Custom Component for this #228

Closed
robbrad opened this issue Feb 26, 2023 · 53 comments
Closed

Make a Home Assistant Custom Component for this #228

robbrad opened this issue Feb 26, 2023 · 53 comments
Labels
idea Something that could happen, but isn't guaranteed

Comments

@robbrad
Copy link
Owner

robbrad commented Feb 26, 2023

@dp247 / @OliverCullimore FYI

I see a lot of people on the HA thread who might be struggling to connect the dots for the shell run of the python to generate the output and then create a rest sensor

One idea is could we make an actual Home Asistant Component that people just plug the values into

@robbrad robbrad added the idea Something that could happen, but isn't guaranteed label Feb 26, 2023
@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Jul 16, 2023

Some ideal requirements on this:

  • Make installable via HACS
  • Set up via UI
  • Allow for multiple instances (catering for multiple properties and also allowing us to more easily set up multiple for testing if there's issues with a specific Council)

Hopefully #56 will be achieved soon which will allow us to start looking into this one.

@robbrad
Copy link
Owner Author

robbrad commented Jul 18, 2023

long way to go but progress @OliverCullimore / @dp247

image

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Jul 19, 2023

@robbrad I've looked at splitting the config flow into 2 steps, the first you select your council, the second you enter the details that council requires (allowing us to only show the needed fields based on input.json)

I've just pushed this expecting it to create a ha_custom_comp branch under my fork of the repo but it looks to have just gone and pushed to yours instead, hope that's okay?

I've hit the following error with my changes when you submit the first step, I believe it's related to the validation of "vol.In()" which checks the option is one of the available councils.

Logs from running this in a HA dev container with the HA core dev branch (so this could possibly work fine on production instances and is just a bug in the dev branch):

2023-07-19 20:59:08.353 ERROR (MainThread) [aiohttp.server] Error handling request
Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/aiohttp/web_protocol.py", line 433, in _handle_request
    resp = await request_handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/aiohttp/web_app.py", line 504, in _handle
    resp = await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/aiohttp/web_middlewares.py", line 117, in impl
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/components/http/security_filter.py", line 85, in security_filter_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/components/http/forwarded.py", line 100, in forwarded_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/components/http/request_context.py", line 28, in request_context_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/components/http/ban.py", line 80, in ban_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/components/http/auth.py", line 236, in auth_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/components/http/view.py", line 148, in handle
    result = await handler(request, **request.match_info)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/components/config/config_entries.py", line 181, in post
    return await super().post(request, flow_id)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/components/http/data_validator.py", line 72, in wrapper
    result = await method(view, request, data, *args, **kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/helpers/data_entry_flow.py", line 118, in post
    result = self._prepare_result_json(result)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/components/config/config_entries.py", line 185, in _prepare_result_json
    return _prepare_config_flow_result_json(result, super()._prepare_result_json)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/components/config/config_entries.py", line 122, in _prepare_config_flow_result_json
    return prepare_result_json(result)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/helpers/data_entry_flow.py", line 44, in _prepare_result_json
    data["data_schema"] = voluptuous_serialize.convert(
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/voluptuous_serialize/__init__.py", line 120, in convert
    if issubclass(schema, Enum):
       ^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: issubclass() arg 1 must be a class

I'll pick trying to resolve this error back up when I next get chance but wanted to document it now I've pushed these changes to be part of yours (again sorry hope that's okay, feel free to revert if this isn't where we were heading with this).

@robbrad
Copy link
Owner Author

robbrad commented Jul 19, 2023

It’s all good I’ll look into it. Basically I made that branch so we could collaborate

@robbrad
Copy link
Owner Author

robbrad commented Jul 20, 2023

I have a feeling its something to do with this

https://github.com/robbrad/UKBinCollectionData/blob/ha_custom_comp/custom_components/uk_bin_collection/config_flow.py#L84

I think async_show_form doesnt like not getting a vol.Schema

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Jul 20, 2023

The get_council_schema returns a vol.Schema though? Just isn't defined as a constant.

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Jul 21, 2023

@robbrad thanks for finding and fixing that.

I've implemented a few more fixes which is showing promise with it trying to run collect_data now.

But running into this error next, will try and take a look at what we need to do to for that sometime tomorrow:

RuntimeError: Blocking calls must be done in the executor or a separate thread; Use `await hass.async_add_executor_job()`; at custom_components/uk_bin_collection/__init__.py, line 43: json_string = await collect_data.main(args)

Also added a "name" field to the first config flow step which we can hopefully use as part of the entity name at some point to allow multiple councils to be set up at once.

@robbrad
Copy link
Owner Author

robbrad commented Jul 22, 2023

https://developers.home-assistant.io/docs/asyncio_working_with_async/ - im studying this

Bit of a learning curve for me here

I wonder if the setup in init.py should be under say sensor.py (which isnt used currently)

@robbrad
Copy link
Owner Author

robbrad commented Jul 22, 2023

I think we may need to pivot slightly - and go after something like this https://developers.home-assistant.io/docs/integration_fetching_data

@OliverCullimore
Copy link
Collaborator

Looking at https://github.com/home-assistant/core/blob/dev/homeassistant/components/adguard/__init__.py for example (I find browsing the components folder of core is helpful for examples) it looks like the AdGuardHome package must have async functions as they just call then with await.

Is that maybe what we need to do? (make collect data an async function)

@robbrad
Copy link
Owner Author

robbrad commented Jul 22, 2023

Looking at https://github.com/home-assistant/core/blob/dev/homeassistant/components/adguard/__init__.py for example (I find browsing the components folder of core is helpful for examples) it looks like the AdGuardHome package must have async functions as they just call then with await.

Is that maybe what we need to do? (make collect data an async function)

I went down the same route this morning!

The thing is - is requests is a syncronous lib and thus blocking.

We could look to convert the whole repo to use aiohttp or find a way to make it work as is

@robbrad
Copy link
Owner Author

robbrad commented Jul 22, 2023

@robbrad
Copy link
Owner Author

robbrad commented Jul 22, 2023

@OliverCullimore actually @RobertYoung has made a very nice example of how this might work for his council - https://github.com/RobertYoung/manchester-city-council-bin-collection/tree/main/custom_components/bin_collection_manchester_council - maybe we should use this approach to base off (MIT License) ?

@OliverCullimore
Copy link
Collaborator

Interesting, I've actually just been looking at https://github.com/home-assistant/core/blob/52ab6b0b9d2534b2200abc11d840bf29e6dd54b8/homeassistant/components/iss/__init__.py after searching for "import requests" which also uses a coordinator approach

@robbrad
Copy link
Owner Author

robbrad commented Jul 22, 2023

I think part of the challenge is that collect_data.py has no class that just instanciates itself - it blocks

So to collect data we run collect_data.main(args) - this is the only path. So when HA runs it gets blocked

The iss implimentation https://github.com/HydrelioxGitHub/pyiss/blob/master/pyiss/__init__.py has calls seperate to the flow

raised #306

@OliverCullimore
Copy link
Collaborator

@robbrad I've actually managed to get it calling collect_data.main successfully without those changes, would they be an improvement to how things are done anyway or would it be better to revert if we don't need to change that?

@OliverCullimore
Copy link
Collaborator

I've just pushed what I've got so far, it's returning the JSON in the logs now but I think we need to convert it over to being a platform like https://github.com/RobertYoung/manchester-city-council-bin-collection/tree/main/custom_components/bin_collection_manchester_council uses to give us the flexibility of creating multiple entities

@robbrad
Copy link
Owner Author

robbrad commented Jul 22, 2023

@robbrad I've actually managed to get it calling collect_data.main successfully without those changes, would they be an improvement to how things are done anyway or would it be better to revert if we don't need to change that?

Not sure its an improvment as such - just makes it more modular. eg instanciate class, set args vs CLI (which implments the same in my changes)

@OliverCullimore
Copy link
Collaborator

Yeah imagine it'll follow a better design pattern to instantiate the class rather than calling main, I'll merge 😃

@robbrad
Copy link
Owner Author

robbrad commented Jul 22, 2023

@OliverCullimore Thanks mate

Do you mind If I push my changes over yours ? - I have it working with the new class approach and non blocking (btw yours and my approaches are 99% the same 😄

@OliverCullimore
Copy link
Collaborator

Sure, go for it 😃

@OliverCullimore
Copy link
Collaborator

@robbrad not seeing any updates so assume you haven't got around to pushing those yet?

@robbrad
Copy link
Owner Author

robbrad commented Jul 22, 2023

Im having issues ! lol this is a proper rabbit hole

I dont know enough about the different approaches or home assistant

I decided to bin off what I had - just to get the sensor working - So I took your code as a base

Im now looking at how https://github.com/home-assistant/core/blob/d708c159e748fde3e2a13333c21afbff4b455602/homeassistant/components/demo/__init__.py works

If you have an idea please commit your code

@OliverCullimore
Copy link
Collaborator

No worries, sounded like you had it working so I shelved my changes in prep to pull your updates and then merge anything applicable from mine back in after.

Let me unshelve those changes and see if I can finish them up to get it working.

@robbrad
Copy link
Owner Author

robbrad commented Jul 22, 2023

Sorry @OliverCullimore

I thought I was so close

Will take a break for a bit. I think one challenge will be mapping the various json outputs to sensor items

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Jul 22, 2023

@robbrad no worries at all, I found myself in a rabbit hole of looking at the docs and other components while trying to get the shelved changes I had to work earlier also. Still got the references I was using open so will pick back up and try and get a sensor working.

I did those updates for all councils to output a common JSON format in #296 to prep for that so hopefully that will make the next steps easier.

I think I also have some ideas on how we can map it all from what I currently have set up with a command line HA sensor populating template entities so I'm happy to take a look at that once we manage to get 1 sensor working.

@robbrad
Copy link
Owner Author

robbrad commented Jul 22, 2023

I think I have found it a challenge knowing the difference between async_setup and async_setup_entity

Setup platform and basically passing in config from the config flow rather than config file

I have realised I'm really hampered without the debugger in VSCode and think there must be a better way to dev than stopping and starting hass every change

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Jul 22, 2023

Yeah I'm still not 100% sure what's what with those sorta bits.

I'm running Docker desktop on Windows with the dev container in VS code as per the docs advice, I don't use VS code as my main IDE though and by default you can't get to the dev container's files via file explorer nor paste files in so I'm copy-pasting the file contents from my main IDE over to it whenever I'm ready to test/copying small tweaks back.
With that I'm still restarting hass for every change to show but at least it only has the few default integrations so is pretty quick compared to if I restarted my "production" hass and it automatically stops and shows the logs in the terminal when it errors.

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Jul 22, 2023

Just learnt you can add

,
"mounts": [
  "source=${localEnv:USERPROFILE}\\Projects\\UKBinCollectionData\\custom_components\\uk_bin_collection,target=${containerWorkspaceFolder}/config/custom_components/uk_bin_collection,type=bind"
]

into devcontainer.json if you set a VS code dev container up BTW so now I can just restart as it's referencing my main IDE's code

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Jul 22, 2023

@robbrad I believe I've made some progress with this (which I've just pushed), it's now creating an entity but I can't seem to get it to have a state/value and also oddly the _LOGGER doesn't seem to want to output from within sensor.py which is making this hard to debug.

image

I've also laid out some code that I hope will allow us to make it automatically create a next_collection sensor per bin type (e.g. next_collection_refuse) as well as the "global" next_collection entity for the next collection across all bin types.

Calling it a day with this now so wanted to leave an update for the next one of us to look at this to pick up from 😄

@robbrad
Copy link
Owner Author

robbrad commented Jul 23, 2023

@OliverCullimore good progress. I tried it out and the logging seams to hang right after

_LOGGER.info(LOG_PREFIX + "Args: %s", self.args)

I added some more logging on my local copy and the logs hang on

json_string = await self.ukbcd.run()

@OliverCullimore
Copy link
Collaborator

@robbrad I think I'd somewhat assumed that couldn't be the case as the entities were being created. Thank you for finding that.

I imagine if we re-wrap that call in what I was doing before for main it might resolve it at a guess, I'll give that a try shortly and report back.

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Jul 23, 2023

@robbrad slight improvement from wrapping it in that but may have found the culprit, I don't think run() is returning hence is blocking so submitted #308 to hopefully fix that

@robbrad
Copy link
Owner Author

robbrad commented Jul 23, 2023

@robbrad slight improvement from wrapping it in that but may have found the culprit, I don't think run() is returning hence is blocking so submitted #308 to hopefully fix that

https://media4.giphy.com/media/v1.Y2lkPTc5MGI3NjExbmo4aG9oejJkanVteWVxNXg2MjVuaTljb3g1bXVxbnJyaTAwMG02OCZlcD12MV9pbnRlcm5hbF9naWZfYnlfaWQmY3Q9Zw/QKpjzdA6W3ndS/giphy.gif

@robbrad
Copy link
Owner Author

robbrad commented Jul 23, 2023

@OliverCullimore - I think the issue still exists. I have been looking into this a little and this could be due to the way the whole uk_bin_collection lib is written

What I am trying is not use HA for now, but trying to get it working with just python eg make a test.py as follows and iterate

Im not having much luck though

from uk_bin_collection.uk_bin_collection import collect_data
import asyncio

testapp = collect_data.UKBinCollectionApp()

testapp.set_args([
    "CheshireEastCouncil",
    "https://online.cheshireeast.gov.uk/MyCollectionDay/SearchByAjax/GetBartecJobList?uprn=100012791226&onelineaddress=3%20COBBLERS%20YARD,%20SK9%207DZ&_=1689413260149"
])

async def test():
    return await testapp.run()

# Use asyncio.run() to run the asynchronous test function and await its result
data = asyncio.run(test())

print(data)

This could be a complete rabbit whole and in reality prob just a lack of a return like you say

@robbrad
Copy link
Owner Author

robbrad commented Jul 23, 2023

Looking at manchester-city-council-bin-collection/custom_components and the iss one are both syncronous - so it should be possible without overall lib changes ?

We just need to makesure all returns work

@OliverCullimore
Copy link
Collaborator

@robbrad slight improvement from wrapping it in that but may have found the culprit, I don't think run() is returning hence is blocking so submitted #308 to hopefully fix that

https://media4.giphy.com/media/v1.Y2lkPTc5MGI3NjExbmo4aG9oejJkanVteWVxNXg2MjVuaTljb3g1bXVxbnJyaTAwMG02OCZlcD12MV9pbnRlcm5hbF9naWZfYnlfaWQmY3Q9Zw/QKpjzdA6W3ndS/giphy.gif

I approved and merged so I'll take some of the blame for missing that too 😄

@OliverCullimore
Copy link
Collaborator

Looking at manchester-city-council-bin-collection/custom_components and the iss one are both syncronous - so it should be possible without overall lib changes ?

We just need to makesure all returns work

Yeah I'll give using it outside of HA a go also and see if we can debug any more missing returns

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Jul 23, 2023

@robbrad a few updates, testing with a test.py calling the run function outside of HA also it was clear it wasn't blocking anymore after the return was added in collect data.

So I think my implementation of the try-catch in init was likely the issue as I don't think it was getting to the return after that.
I think fixing that seems to have allowed the entity to now be populated but can't confirm which test achieved that, but I've pushed my changes as it's hopefully progress in the right direction.

I'm still not seeing it get to the "Council Data:" logging line but as I'm seeing the output from the library printing the JSON straight after "Collecting data for council:" I'm thinking it could be a potential cause so I've submitted a PR to amend where we print the JSON to allow us to rule that out.

@robbrad
Copy link
Owner Author

robbrad commented Jul 23, 2023

Just tried it - Great work ! I can see the date for Glasgow in Home Assistant

When I try it for CheshireEast I get 2023-07-23 19:40:54,053 root ERROR Error Connecting: ('Connection aborted.', ConnectionResetError(104, 'Connection reset by peer')) however - ill see if I can take a look at this tomrrow night

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Jul 23, 2023

@robbrad Glad to hear.

Do you mean it's putting that error into the entity? Should probably make sure it returns none instead if so and also likely should uncomment https://github.com/robbrad/UKBinCollectionData/blob/ha_custom_comp/custom_components/uk_bin_collection/sensor.py#L35 again which I'd commented out as a test previously.

A few councils including that one that are failing the tests at the moment https://robbrad.github.io/UKBinCollectionData/3.11/827/#suites/98d3104e051c652961429bf95fa0b5d6/3bd2be2855be5102/ (just checking you're not thinking that one's just failing via HA)

Some I've found are hit-and-miss and have worked sometimes locally for me but didn't manage to find any fixes for the SSL errors etc yet when I looked the other day.

@robbrad
Copy link
Owner Author

robbrad commented Jul 26, 2023

@OliverCullimore : I have made some restructuring changes and am getting some positive results

For now until the get data is sorted I have hardcoded some bin data - this will allow us to iterate on the way the sensor looks and is and displayed. Im 99.99% confident the actual data get will work but to stop me breaking the council infra while im testing - I commented it

@robbrad
Copy link
Owner Author

robbrad commented Jul 26, 2023

I think we are going to need a way for the user - maybe in the config_flow to map colours to their bins ? maybe a flow could read the output folder for their council and we could use the const file to pull that mapping from the config flow - thus allowing the sensor to map the correct colour

@robbrad
Copy link
Owner Author

robbrad commented Jul 26, 2023

I think we are going to need a way for the user - maybe in the config_flow to map colours to their bins ? maybe a flow could read the output folder for their council and we could use the const file to pull that mapping from the config flow - thus allowing the sensor to map the correct colour

Or we just let the front end handle it

@robbrad
Copy link
Owner Author

robbrad commented Jul 27, 2023

Trying to fix the options flow when you press configure - should scan intervel be in there?

@OliverCullimore
Copy link
Collaborator

I think we are going to need a way for the user - maybe in the config_flow to map colours to their bins ? maybe a flow could read the output folder for their council and we could use the const file to pull that mapping from the config flow - thus allowing the sensor to map the correct colour

Or we just let the front end handle it

I think that might be best as long as we allow it to retain any front-end entity customization of icons when we update entities which I imagine it will, will give it a test.

Then end-users can set the colour and the icon themselves if desired (to mdi:recycle or mdi:tree etc for recycling and garden bins)?

@OliverCullimore
Copy link
Collaborator

I think we are going to need a way for the user - maybe in the config_flow to map colours to their bins ? maybe a flow could read the output folder for their council and we could use the const file to pull that mapping from the config flow - thus allowing the sensor to map the correct colour

Or we just let the front end handle it

I think that might be best as long as we allow it to retain any front-end entity customization of icons when we update entities which I imagine it will, will give it a test.

Then end-users can set the colour and the icon themselves if desired (to mdi:recycle or mdi:tree etc for recycling and garden bins)?

Tested and you can only change the icon from the UI, not seeing red icons as you might expect from setting the colour so may not be the right attribute to return to control that.

@dp247
Copy link
Collaborator

dp247 commented Jul 27, 2023

This is fantastic work guys - as a HA noob, is there anything I can read to be of any use?

@robbrad
Copy link
Owner Author

robbrad commented Jul 27, 2023

This is fantastic work guys - as a HA noob, is there anything I can read to be of any use?

It's been a real learning journey I'm sure @OliverCullimore would agree

Best thing you can do is clone down the branch and try it out. Uncomment the data refresh and comment the hard coded data on the sensor.py. Try it out and find the bugs/issues. 90% of what we have is pretty good but I'm sure there is more that could be solved.

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Jul 27, 2023

Yeah would definitely agree but it feels like we're getting there with it all.

@dp247 if you don't have HA set up at all this might be a good option if you have/don't mind installing VS code and docker desktop. See this also for how I'm then pulling the cloned repo into that for testing.

@robbrad
Copy link
Owner Author

robbrad commented Jul 27, 2023

@OliverCullimore / @dp247 - just fixed some things on the data pull

@danielcecil
Copy link
Contributor

Hey folks, I've been trying this out with HA and so far so good, top work! I have made a few extra aggregate entities to help some automations including:

  1. 'Next Collection' with the earliest date across all sensors as the state, and a list of which bins are being collected as an attribute.
  2. 'Bins Today' and 'Bins Tomorrow' binary sensor.

What do you think to adding these in code?
bin_sensors.yaml.txt

@OliverCullimore
Copy link
Collaborator

Thanks to everyone who helped make this happen!

@robbrad I think this one can likely be closed now and any new ideas etc opened as separate issues?

@robbrad robbrad closed this as completed Sep 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea Something that could happen, but isn't guaranteed
Projects
None yet
Development

No branches or pull requests

4 participants