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

Add GetMapSetV2 command #372

Merged
merged 18 commits into from
Feb 15, 2024
Merged

Conversation

MVladislav
Copy link
Contributor

@MVladislav MVladislav commented Dec 12, 2023

Update map usage for newer bot's.

Updates differences:

  • GetCachedMapInfo is using GetMapSet to draw "virtual walls" or "no map zones"
    • for get it to work newer models use new API call over GetMapSetV2 which can be choose by GetCachedMapInfo on init by set version = 1 or 2
  • drawing the floor has an update, as the new bot's have more pixel_type which needs to be colored
    • over app they can be colored in different colors or also only in one color (here now only update for use one color)
    • 3 new types are added for coloring:
      • 0x04 => floor which could be related to a room but could not be reached by bot
      • 0x05 => any devices existing in a room like a couch, desk, ...
      • and every number not in 0-5, is now a room, which could make it possible to color each room in a different color, but is here now color each room into a single color
      • possible better colors to be choose

Problems to be fixed:

  • to be done is fix the event call for GetMapSetV2
    • the direct call of the function GetMapSetV2 is working
    • but if the event is triggered by bot or app, it still use the old event and calls GetMapSet instead the new version
  • test case not working

Related issue:

MVladislav and others added 2 commits December 12, 2023 19:26
- in function "GetMapSubSet" add check for compressed values, as newer bot's return value base64 7z compressed
  - but "GetMapSubSet" will not used for newer bot's now, as the new api will instead used "GetMapSetV2"
- update "GetCachedMapInfo" to possible choose between GetMapSet v1 or v2
- add new command "GetMapSetV2" as the old api "GetMapSet" not responses working result for "vw/mw"
  - TODO: problem on event call for new "GetMapSetV2", on an event it still calls "GetMapSet" instead "GetMapSetV2"
- deebot T10 update to use new map version
- moved function "decompress_7z_base64_data" into "utils" file
- update map coloring as new map use more different type to create a map
- add/updates some test cases
deebot_client/commands/json/map.py Outdated Show resolved Hide resolved
deebot_client/commands/json/map.py Outdated Show resolved Hide resolved
deebot_client/commands/json/map.py Outdated Show resolved Hide resolved
deebot_client/commands/json/map.py Outdated Show resolved Hide resolved
deebot_client/commands/json/map.py Outdated Show resolved Hide resolved
deebot_client/util.py Outdated Show resolved Hide resolved
deebot_client/util.py Outdated Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/commands/json/map.py Outdated Show resolved Hide resolved
@edenhaus edenhaus added the pr: new-feature PR, which adds a new feature label Dec 13, 2023
@edenhaus edenhaus mentioned this pull request Dec 24, 2023
edenhaus added a commit that referenced this pull request Dec 26, 2023
Co-authored-by: mvladislav <dev@mvladislav.online>
@MVladislav
Copy link
Contributor Author

It seams the map is in general updated with #373 for coloring and also improved for svg.
In this issue is the changes/updated included for map command v2.
I have checked all your comments and suggestions from above.

This is my quick example main.py which i used for test the map (with self hosted bumper):

"""Main."""
from __future__ import annotations

import asyncio
import logging
from pathlib import Path
import time
from typing import TYPE_CHECKING

import aiohttp

from deebot_client.api_client import ApiClient
from deebot_client.authentication import Authenticator, create_rest_config
from deebot_client.device import Device
from deebot_client.events import CachedMapInfoEvent, MapChangedEvent
from deebot_client.mqtt_client import MqttClient, create_mqtt_config
from deebot_client.util import md5

if TYPE_CHECKING:
    from deebot_client.models import DeviceInfo

_LOGGER = logging.getLogger(__name__)

# - getMapSubSet
#   - Räumeinfos(Name,typ, ...),
#   - virtual walls
#   - no mop zones
# - GetMajorMap
#   - crc32 für alle chucks
# - GetMinorMap
#   - Raumstruktur (Wände, Teppich)
#   - geänderten chunks


class Main:
    """Class main."""

    device_id = md5(str(time.time()))
    account_id = "abc@xy.local"
    password_hash = md5("yourPassword")
    continent = "eu"
    country = "de"

    _authenticator = None
    _deebot_config = None
    _devices: list[DeviceInfo] | None = None
    _bot: Device | None = None

    def __init__(self) -> None:
        logging.basicConfig(level=logging.INFO)

    async def _play_ground(self) -> None:
        _LOGGER.info(f"==> try get map for bot '{self._bot.device_info.name}'...")

        async def on_info(event: CachedMapInfoEvent) -> None:
            _LOGGER.info(f"map info :: {event.name}")

        async def on_changed(event: MapChangedEvent) -> None:
            _LOGGER.info(f"map changed :: {event.when}")
            a = self._bot.map.get_svg_map()
            self._save_svg_img_to_file(a)

        self._bot.events.subscribe(CachedMapInfoEvent, on_info)
        self._bot.events.subscribe(MapChangedEvent, on_changed)

    def _save_svg_img_to_file(
        self, svg_string: str | None, output_filename: str = "image.svg"
    ) -> None:
        if svg_string is not None:
            with Path.open(output_filename, "w") as file:
                file.write(svg_string)

    async def main(self) -> None:
        """Call main function."""
        my_session = aiohttp.ClientSession(
            connector=aiohttp.TCPConnector(verify_ssl=False)
        )
        self._devices = await self._retrieve_devices(my_session)
        await self._base_connect()
        await self._play_ground()

    async def _base_connect(self, bot_index: int = 0) -> None:
        self._bot = Device(self._devices[bot_index], self._authenticator)

        mqtt_config = create_mqtt_config(device_id=self.device_id, country=self.country)
        mqtt = MqttClient(mqtt_config, self._authenticator)
        await self._bot.initialize(mqtt)

    async def _retrieve_devices(
        self, my_session: aiohttp.ClientSession
    ) -> list[DeviceInfo]:
        self._deebot_config = create_rest_config(
            my_session,
            device_id=self.device_id,
            alpha_2_country=self.country.upper(),
        )

        self._authenticator = Authenticator(
            self._deebot_config,
            self.account_id,
            self.password_hash,
        )

        api_client = ApiClient(self._authenticator)

        return await api_client.get_devices()


if __name__ == "__main__":
    loop = asyncio.get_event_loop()
    loop.create_task(Main().main())
    loop.run_forever()

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (8f75ebf) 82.72% compared to head (0257b3c) 83.10%.

Files Patch % Lines
deebot_client/commands/json/map.py 79.54% 6 Missing and 3 partials ⚠️
deebot_client/map.py 33.33% 2 Missing ⚠️
deebot_client/messages/json/map.py 81.81% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #372      +/-   ##
==========================================
+ Coverage   82.72%   83.10%   +0.37%     
==========================================
  Files          72       73       +1     
  Lines        2895     2942      +47     
  Branches      515      528      +13     
==========================================
+ Hits         2395     2445      +50     
+ Misses        450      443       -7     
- Partials       50       54       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MVladislav
Copy link
Contributor Author

This is an image snippet how it looks with the SVG, color and map v2 changes with a T10 bot:

current dev version with map v2 command
image image

@edenhaus
Copy link
Contributor

edenhaus commented Feb 9, 2024

I have made some minor improvements. I hope it is fine to push then directly on the PR

image

Why are the rectangles filled with black?

- with current color set to #f00 background was black, needs to set to full hex name
- reduce the alpha channel to be more transparent
- add check if "subsets" is not give to not get an error and possible handle event/atr call
- add HandlingResult if it is called by event/atr
@MVladislav
Copy link
Contributor Author

I have made some minor improvements. I hope it is fine to push then directly on the PR

I like it, thanks :).
You can always directly push code changes into my PRs, i have no problems with that.

Why are the rectangles filled with black?

This is because of used color as MapSetType.VIRTUAL_WALLS: "#f00"
I have update it and also adjust alpha channel for a better look.

image

@MVladislav
Copy link
Contributor Author

By side i checked how v2 is performing and found some "problems"/points I would need some help/decisions:

  • The new version is named as getMapSet_V2, but i found a line which overwrites _V2 inside deebot_client/messages/__init__.py in line 34-35
    • This was a hell to understand why the events always use getMapSet instead of getMapSet_V2
    • # T8 series and newer
      if converted_name.endswith("_V2"):
          converted_name = converted_name[:-3]
    • How should we proceed with that?
      • A possible solution could be, instead of create getMapSet_V2, we can combine it inside getMapSet with checks what data ins included, but not sure if this will work.
      • Or make a whitelist, for which command the V2 should not be removed.
  • I have update getMapSet_V2 a bit:
    • When it is called to use getMapSet_V2 directly, over GetCachedMapInfo, it is working as expected.
    • But when it is called by onMapSet_V2-"event/atr" it has less info and the map will not be updated
      • This is intercepted by json/map.py in line 301 by check if subsets is given
      • I would somehow call GetMapSetV2 again to collect the needed information, like it is also done in GetMapTrace
        • where it calls itself inside _handle_response
        • but i'm not sure how this logic works
        • current when GetMapSetV2 is called by GetCachedMapInfo it works and the map is updated
        • but when it is called by event/atr it will not be updated
    • We could also complete ignore that topic, to handle update the map by event/atr calls, as this also not works for older models where it should be updated by GetMapSubSet
      • changed virtual walls will than only load on inital calls not on events

@MVladislav
Copy link
Contributor Author

MVladislav commented Feb 10, 2024

What do you think about, by extending the map with names, or maybe also with ID info?
(I not pushed this change now)
(Here i test the change: MVladislav@cd47c6f)

As Example:

only name name with id
image image

@MVladislav
Copy link
Contributor Author

MVladislav commented Feb 11, 2024

For the new bots in V2 API the walls are separate drawn, for that the event onMapInfo_V2 needs to be added.

In the app, the walls can be activated and deactivated, which will looks like:

activated walls deactivated walls
image image

Not sure if this is really needed, i have local a version which is half ready, but i get some errors which i can currently not fix. If this is something required, i can try to finish it, else i would remove it.
(Here i test that change: MVladislav@cd47c6f)

It would be looks as follows:

without wall with walls
image image

- add notify 'MapSetType' into  'GetMapSetV2'
@edenhaus
Copy link
Contributor

What do you think about, by extending the map with names, or maybe also with ID info?

Please don't add it in this PR. We should discuss this first via Discord

@edenhaus
Copy link
Contributor

* When it is called to use `getMapSet_V2` directly, over `GetCachedMapInfo`, it is working as expected.

This is how it should be

* But when it is called by `onMapSet_V2`-"event/atr" it has less info and the map will not be updated

Please implement onMapSet_V2 like the onBattery in https://github.com/DeebotUniverse/client.py/blob/dev/deebot_client/messages/json/battery.py
See also https://github.com/DeebotUniverse/client.py/blob/dev/deebot_client/commands/json/battery.py to avoid duplicated code

By implementing the message if converted_name.endswith("_V2"): is no problem anymore and the way to go... I want to remove soon or later all the conventions fallbacks

@edenhaus
Copy link
Contributor

n the app, the walls can be activated and deactivated, which will looks like:

We should keep this PR as small as possible and not add any more features... These features can be added in follow up PRs

@edenhaus edenhaus changed the title Update command: "Map" Add GetMapSetV2 command Feb 11, 2024
@MVladislav
Copy link
Contributor Author

What do you think about, by extending the map with names, or maybe also with ID info?

Please don't add it in this PR. We should discuss this first via Discord

n the app, the walls can be activated and deactivated, which will looks like:

We should keep this PR as small as possible and not add any more features... These features can be added in follow up PRs

I will let that changes be backup tracked in issue for titles and walls, if the code will be needed.

- move main code into message and use message in command
- add note for future live map update todo
- removed not needed lines
- extend and cleanup test cases
deebot_client/commands/json/map.py Outdated Show resolved Hide resolved
deebot_client/commands/json/map.py Outdated Show resolved Hide resolved
deebot_client/map.py Outdated Show resolved Hide resolved
deebot_client/messages/json/map.py Outdated Show resolved Hide resolved
tests/commands/json/test_map.py Show resolved Hide resolved
- split get and on logic into the correct function-class
- comment fix
- include removed test_getMapSubSet_customName back
deebot_client/commands/json/map.py Outdated Show resolved Hide resolved
MVladislav and others added 2 commits February 15, 2024 18:54
- create function getMapSet v1 and v2 to combine same function and easier handle
- let getMapSet v2 call getMapSubSet instead of its own room defintion
  - getMapSubSet as room boarder defined
- update test cases
@edenhaus edenhaus merged commit 3433f28 into DeebotUniverse:dev Feb 15, 2024
9 checks passed
@MVladislav MVladislav deleted the feature/CommandMap branch February 15, 2024 21:26
@tobiasbrage
Copy link

Is a fix in the works? I'm really regretting buying an Ecovacs and not a Roborock at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: new-feature PR, which adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants