Skip to content
This repository has been archived by the owner on Mar 8, 2022. It is now read-only.

Config entry and various changes #91

Merged
merged 28 commits into from
Apr 26, 2021
Merged

Config entry and various changes #91

merged 28 commits into from
Apr 26, 2021

Conversation

edenhaus
Copy link
Contributor

@edenhaus edenhaus commented Mar 9, 2021

Fixes #77, fixes #69, fixes #31, fixes #80 and in general all configuration.yaml mistakes made from users

Please be aware that even the library changed, https://github.com/And3rsL/Deebotozmo/tree/beta

Create this PR so I can give you feedback here

@And3rsL
Copy link
Owner

And3rsL commented Mar 9, 2021

To test it:

  1. Download everyting and replace "deebot" in custom components
  2. Remove every reference in configuration.yaml
  3. Integrate it by Settings -> Integrations

No need to save png anywhere.

Live map now uses camera component
@edenhaus
Copy link
Contributor Author

edenhaus commented Mar 9, 2021

I think we should only add translation files, where we actually have already translated the wording. If the file for the specify language doesn't exist, it will fallback automatically to English. This is also the case, when a key doesn't exist in the specific language file and reduce also the maintenance amount, when we introduce a new key. What do you think?

Therefore we would only have the enlgish, italian (which probably you will translate it) and german (which I will translate it)

@And3rsL
Copy link
Owner

And3rsL commented Mar 9, 2021

Totally agree

@edenhaus
Copy link
Contributor Author

edenhaus commented Mar 9, 2021

Are all sensor enabled by default? If yes I would disable all by default and the user can then enable the sensor, which he needs. I for example would disable last_clean_image and some stats sensors as I don't need them.

@edenhaus
Copy link
Contributor Author

edenhaus commented Mar 9, 2021

Logger: homeassistant.util.async_
Source: util/async_.py:129
First occurred: 19:56:30 (4 occurrences)
Last logged: 19:56:30

Detected I/O inside the event loop. This is causing stability issues. Please report issue to the custom component author for deebot doing I/O at custom_components/deebot/config_flow.py, line 49: ecovacs_api = EcoVacsAPI(
Detected I/O inside the event loop. This is causing stability issues. Please report issue to the custom component author for deebot doing I/O at custom_components/deebot/config_flow.py, line 57: return ecovacs_api.devices()

I get the following warnings

Can I also push some changes to this branch?

Copy link
Contributor Author

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Nice work. I will test it tomorrow :)

custom_components/deebot/sensor.py Outdated Show resolved Hide resolved
custom_components/deebot/camera.py Outdated Show resolved Hide resolved
custom_components/deebot/sensor.py Show resolved Hide resolved
@edenhaus
Copy link
Contributor Author

edenhaus commented Mar 10, 2021

I created the following PR:

This are requirements for hacs, which I found out,when I was trying to change the name their.

- Unload library on entity delete
- No more warnings on sync calls
@And3rsL
Copy link
Owner

And3rsL commented Mar 10, 2021

I created the following PR:

This are requirements for hacs, which I found out,when I was trying to change the name their.

i was just wondering how set a integration image! nice!

@edenhaus
Copy link
Contributor Author

I added that all sensors are disabled by default and the user can enable only the sensor, which he needs.

During this change I recognized that we don't setup a device. This would be really helpful, as the user gets a overview of the device with it's corresponding entities, automation's, etc.
See screenshot below from esphome:
grafik

What do you think?

else:
self.data[CONF_DEVICEID] = user_input
return self.async_create_entry(
title="Ecovacs vacuum custom integration", data=self.data
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest that we set as title the name of the vacuum. So the user can distinguish the vacuums on the integration side. Currently we create each entry with the same name. For example compare it with EspHome in the screenshot below (esp01, esp02, ... are the device names. I personally called them in this manner)
grafik
To get the title correct, we probably should only allow single select on the robots list or create of each selected robot a different entry.

Currently it is also possible to add the same vacuum twice. I think we should throw an error, when the vacuum is already set up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay I think I have a idea for the problem. Will give it a try tomorrow

Copy link
Owner

Choose a reason for hiding this comment

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

I don't know, i would like have it by account.. insert multiple times same user and password for different robots on same account is kinda confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The account is fine for me. Only we should add a OptionHandler, so a user can change room color and co without deleting and creating the entry again

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, i agree with that, maybe use first "form" to insert only Username,Password,Country,Continent -> Select robots and then from Options you should choose live map and room colors.. maybe add more options such as map color, line color etc..

Copy link
Owner

Choose a reason for hiding this comment

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

At the moment im honestly a bit scared from ecovacs api.. it seems like they started changes endpoints. i hope they dont change it too much

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment im honestly a bit scared from ecovacs api.. it seems like they started changes endpoints. i hope they dont change it too much

Not good et all.... Next week I hopefully set up bumper (selfhosted deebot server) so my vacuum is not anymore sending data to china

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@And3rsL Since Version 2 of the app ecovacs uses oauth. Probably that was the big api change.
Give a look my PR at bumper bmartin5692/bumper#116
There exists new endpoints like /newauth.do and api/appsvr/oauth_callback which we must probably use in the future.
After calling the oauth_callback each post request has the following in it's body

"auth": {
        "realm": "ecouser.net",
        "resource": "IASB481A03G8",
        "token": "TOKEN",
        "userid": "USERID",
        "with": "users"
    }

)

_LOGGER.debug("New vacbot found: " + device["name"])
vacbot.setScheduleUpdates()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should run this method later in a different thread. This method takes more then 10 seconds to complete.... Think you have more than one robot it can take more then 30 seconds or even more.
I think we should run this method later on in parallel. For testing I comment out this method and the conflig flow was quickly finished. Also I think the user has no problems if the robots is unavailable for a minutes after after creation.

Also setScheduleUpdates has problems, when the vacuum is offline during setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would probably remove this method. See And3rsL/Deebotozmo#27 (comment)

@And3rsL And3rsL marked this pull request as ready for review April 26, 2021 13:41
@And3rsL And3rsL merged commit 0b2ac39 into master Apr 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants