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

Mapping Support #100

Closed
simbaja opened this issue Oct 24, 2021 · 6 comments
Closed

Mapping Support #100

simbaja opened this issue Oct 24, 2021 · 6 comments

Comments

@simbaja
Copy link

simbaja commented Oct 24, 2021

I've forked this component and the HA integration and have added initial mapping support. Was wondering if you are interested in getting this merged in? I can submit a PR, but it's fairly extensive (added some stuff from your upstream, added a new mapping module and assets) so didn't know how you wanted to approach. See:

SDK: https://github.com/simbaja/roombapy
HA: https://github.com/simbaja/ha_roomba

@pschmitt
Copy link
Owner

Hey,

Great work there! I'd love to see proper mapping support for Roombas in Home Assistant.

As to whether this should be a part of roombapy: I'm not particularly keen on seeing this implemented here. When I initially forked all the mapping stuff was one of the first things I ripped out. To me, roombapy was meant to be just a "simple" library to get (raw) data out of a Roomba and nothing more. I'd like to keep roombapy clean and concise, ergo not try to do too much stuff which might be applicable only for specific use cases. Ideally mapping would be done by a separate package which depends on roombapy for the acutal communication with the devices.

I'm not totally against adding some features which might help you or other projects, but I don't want to add any functionality that only is applicable to Home Assistant (or any other project for that matter). I'd gladly any PR though.

Thanks, and keep on hacking.

@simbaja
Copy link
Author

simbaja commented Oct 29, 2021

Yeah, I don't blame you for ripping out the mapping (and most of the other functionality) that was there, it was way too much. I ultimately did the same and re-wrote most of what's in the base fork related to mapping (though I kept some concepts like IconSets). I also pushed most of the mapping itself into the "mapping" module so that minimal changes are actually needed in the roomba class itself, primarily:

  1. a few mapping objects
  2. a few methods so that users can define and add maps, icons, etc.
  3. position/phase/state change history and tracking
  4. update hooks into the update state method so that the mapping is updated

Really, some form of 3/4 are what need to be in this library, 1/2 are there for convenience so I didn't have to have multiple libraries. I leveraged some of Nick's rewrite, so my fork shows more changes than are minimally required to support this functionality (it was just easier to do that than to try to pick and choose and be inconsistent). It'll require more work in the HA component to split into two libraries (will have to create a mapping object there instead of just creating a roomba with mapping), but it can be done if that's the only path forward here.

I'm at a point where this works for me as a drop-in replacement for the existing HA integration (and gives me map support, dock status support, and room-by-room cleaning support through service calls). I can work breaking out the mapping into its own library (shouldn't require much from the mapping side since it's already a module within roombapy), but I don't want it to be futile effort should I proceed. Are there any other reasons why mapping support (and some of those other features) are not already integrated into HA that would prevent these changes from getting into the HA component? The other challenge there is that if it is in two packages, there may be future concerns since these are going to inevitably be fairly tightly coupled. Do you have any concerns on future maintenance of the HA component if functionality is split into multiple packages?

@rafaborges
Copy link

I know it's an old thread, but let me leave here my 2p on this topic: I'm in favour of adding some mapping functionalities to Roombapy as it would allow further integration with HA automations. For instance, Roomba uses cameras to navigate and detect objects. A simple automation that turns the lights on for a given room when the robot is about to clean that room, would drastically improve cleaning quality. Also, integration with HA areas and schedule/calendars would take the integration to the next level.

@simbaja
Copy link
Author

simbaja commented Jan 16, 2023

Unfortunately, as far as I understand, iRobot removed the position information support from recent firmware releases, so it's not really possible to add the mapping. I think it's still available for some older devices, so feel free to take a look at the fork I wrote and see if it works for you. I don't really know if we can do it more generally any more due to those changes, unless someone has figured out how to get that information again.

@Orhideous
Copy link
Collaborator

Well, I guess this issue will be closed as out of scope of this project — especially when position information is no longer available.

@Orhideous
Copy link
Collaborator

@simbaja Turns out we can get map information from the cloud after #278 merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants