homebridge-plugins / homebridge-roomba2

Homebridge plugin to connect iRobot Roomba devices with Homebridge/HomeKit.
MIT License
146 stars 17 forks source link

New Option to Stop on Off #63

Closed rcoletti116 closed 3 years ago

rcoletti116 commented 3 years ago

Changes to two files in this PR:

  1. config.schema.json
  2. index.js

Today's Behavior

Today when you turn off the device in homekit the plugin pauses the job and sends the robot home. For some users this behavior is not ideal. Perhaps they are stopping it to make things quiet for a minute, and the command to dock turns it back on.

Changes in PR

This PR gives a new config option "Do not send home on stop."

N.B.: The reason I chose the command 'stop' instead of 'pause' is because I could not find a way to properly implement a 'resume' command. We would need to identify that the robot is in a paused state and choose to send a 'resume' command instead of 'clean,' when turned back on, as sending clean command on a paused job results in an error. This way we are ending the job, which is not ideal, but allows the controls in HomeKit to be more consistently.

Let me know if you have any questions on this.

karlvr commented 3 years ago

@rcoletti116 thank you, I have exactly this issue too. I wonder if we could also add an integration to empty the bin...

karlvr commented 3 years ago

@rcoletti116 I've reviewed your changes and I'm excited about these changes.

Firstly, I could merge the manufacturer change immediately, and the serial number change—I was just looking at that in the code, to discover that it seems someone got halfway with it! Are you able to open a separate PR for those two so we can get them out of the way while we tackle this new feature? They're not related to the feature, so it will be better practice to separate them anyway!

On to the feature!

I've posted some review feedback; a few things that need tidying up. I can tidy that sort of thing up if you don't have time, but if you're keen to stay the course that's great!

I hoped that there would be something in the status we get back from Roomba to tell us whether it's paused or not, so we would know to send a resume. Did you investigate what comes back from dorita980 to determine that there wasn't anything? Alternatively, I'd be happy for us to send "clean", receive the error, understand it, and then send a "resume" instead. Actually, that sounds like it will be more efficient than waiting for a status before commanding our Roombas. If you're happy to try that, please let me know how you get on, otherwise I am happy to take a look.

Thanks again!

rcoletti116 commented 3 years ago

I'll get on the code cleanup and separate PR.

I have been looking at dorita980 to get familiar with the functions, reading old issues and what documentation exists. I've actually opened a PR there so we can finish the identify feature. There are more commands available that could be added, if they prove useful.

I've been trying to see how we can tell if the robot is in a paused state and so far have had no success. I'm happy for you to look at that too.

As for ideal functionality, I agree a separate "button" or stateless switch to send home is probably a good idea.

rcoletti116 commented 3 years ago

@karlvr for further interest; I wasn't sure where to comment this. I've found that the there is a field "lastCommand" in the "getRobotState" which will show as 'pause' after a pause command is given. Maybe this could be used to key a Clean vs. Resume.

lastCommand: { command: "pause", time: 1632170499, initiator: "manual" },

I'm testing with the rest980 library which provides simple APIs for dorita980.

karlvr commented 3 years ago

@rcoletti116 thank you, that sounds interesting. Have you investigated just starting a clean and handling the error?

Also, and I apologise, I have just completed the port of the code to TypeScript. I think this will make it a lot easier to reason about, maintain and enhance the code. I'm happy to handle the merge conflict in your PRs.

I've included a new Building section in the README.

karlvr commented 3 years ago

@rcoletti116 OK... I am making some major changes to the plugin today to improve a variety of things. I have discovered how we can tell whether we should clean or result. The cycle property in the cleanMissionStatus is "clean" and the phase is "stop" when it is paused while cleaning, while the cycle property is "none" when it's properly stopped.

I will make that information available for you to use...

Although interestingly... if we send a clean and then a resume straight away, we don't seem to have any problems!

rcoletti116 commented 3 years ago

Also, and I apologise, I have just completed the port of the code to TypeScript.

That was a fast overhaul. It may tap me out for a while. I'll need to get my fork back in sync without losing the things I've been working on and then start navigating the new code.

Sounds good on the phase and cycle. I was testing and looking there expecting to find something but didn't see that pattern. Good catch.

karlvr commented 3 years ago

I'll adapt your PR in a branch. I know it's a massive change to the code… and I hope I'm right about this new polling approach being better. It just seems silly to me to be constantly asking Roomba's status all day every day every 30s or 5s when no one is looking at the Home app!

karlvr commented 3 years ago

@rcoletti116 here's your branch adapted and slightly modified (to use a pause instead):

https://github.com/karlvr/homebridge-roomba2/tree/rcoletti116-main

If you'd like to grab those changes into your branch, then we should have a good basis to continue.

karlvr commented 3 years ago

@rcoletti116 I've added a contact sensor to show whether Roomba is in Docking mode... I wonder if it should be a switch instead, so we can activate that mode. Perhaps similarly the Running contact sensor could be a switch.

rcoletti116 commented 3 years ago

I believe the reason the 'running' is set as a sensor is because sensors can generate notifications, whereas switches cannot. I think that should stay a sensor.

A switch to 'dock' makes sense. It would need to be stateless I think to run the dock command and turn back off.

rcoletti116 commented 3 years ago

This is ready for review again. Changes pushed through to my main branch are updated here.

karlvr commented 3 years ago

@rcoletti116 thank you; I've merged this in. I'm making quite a few changes at the moment, and of course a few of those from today weren't compatible. I'm going to knock this into shape now... please take a look at what I've done... I think perhaps we could consider using a stop() sometimes as well as a pause. More experimentation needed!

It's there now. I've published a beta (v1.3.0-beta.2), which you can install in Homebridge using the "previous versions" functionality (if you don't already know that!). I haven't tested it much (it's late and Roomba is noisy).

The stopping functionality is more advanced now... seems like a good idea to check what Roomba is actually doing before telling it to do something like stopping! We definitely need to reconcile your feature with wanting to dock Roomba sometimes... I feel like your change could become the default and we then add a separate "Go Home" button!! But it is late.

rcoletti116 commented 3 years ago

Got the beta installed. Thanks for cutting a beta release. So good news is the new functionality for noDockOnStop works correctly. The "bad" news is the connections seem unstable. I get a lot of no responses and commands that don't go through.

karlvr commented 3 years ago

@rcoletti116 thank you. Yes, I'm getting a similar experience. I've created an issue for it… I'll tag you in.