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

Full set of mower commands #10

Closed
Jpsy opened this issue May 26, 2020 · 10 comments · Fixed by #11
Closed

Full set of mower commands #10

Jpsy opened this issue May 26, 2020 · 10 comments · Fixed by #11

Comments

@Jpsy
Copy link
Contributor

Jpsy commented May 26, 2020

Oskar, first and foremost a big THANK YOU for your work!

Over the last days I have tested your component with my Gardena mower (which is my only Gardena Smart component). With my limited knowledge of Python and of the deeper mechanics of HA I have compiled the following details, which are hopefully correct:

The Husqvarna/Gardena API provides 4 mower commands:

  1. START_SECONDS_TO_OVERRIDE - Manual operation, use 'seconds' attribute to define duration.
  2. START_DONT_OVERRIDE - Automatic operation.
  3. PARK_UNTIL_NEXT_TASK - Cancel the current operation and return to charging station.
  4. PARK_UNTIL_FURTHER_NOTICE - Cancel the current operation, return to charging station, ignore schedule.

These 4 are all implemented as methods in your py-smart-gardena2 lib.
But only 2 of them are exposed as HA services by your custom component:

  1. START_SECONDS_TO_OVERRIDE
    through services:
    vacuum.turn_on, and vacuum.start
  2. PARK_UNTIL_NEXT_TASK
    through services:
    vacuum.turn_off, vacuum.return_to_base, and vacuum.stop

My request is to expose all 4 commands.

The rationale is simply that they are needed. There is currently no service to stop the mower permanently (deactivate the schedule) and also no service that would activate the schedule without immediately starting the mower.

IMHO the best mapping would be this:

  1. START_SECONDS_TO_OVERRIDE = vacuum.start
  2. START_DONT_OVERRIDE = vacuum.turn_on
  3. PARK_UNTIL_NEXT_TASK = vacuum.return_to_base
  4. PARK_UNTIL_FURTHER_NOTICE = vacuum.stop and vacuum.turn_off

I tried implementing this myself to create a PR. But unfortunately I found that the turn_off and turn_on service calls never get through to the corresponding methods in vacuum.py of your component. I am far from capable of finding the reasons for that. Without the turn_on/off calls we would have only 3 services left to map 4 mower commands.

If in the end we would have to live with only 3 services, I would recommend the following mapping - which would provide the most important functions for automations:

  1. START_SECONDS_TO_OVERRIDE = vacuum.start
  2. PARK_UNTIL_NEXT_TASK = vacuum.return_to_base
  3. PARK_UNTIL_FURTHER_NOTICE = vacuum.stop

But it would be much more desirable to have all commands exposed.
Please drop a comment if I can help in any way.

@Jpsy
Copy link
Contributor Author

Jpsy commented May 27, 2020

I just fount that in the HA dev docs chapter 3.18 vacuum class StateVacuumEntity the methods async_turn_on and async_turn_off are marked as "not supported". Is this the reason why they do not trigger the custom integration.? But they are called async_* while the custom integration does not use the async_ prefix. My understanding is still much to thin.

@wijnandtop
Copy link
Contributor

Maybe introduce a switch for the mower called "follow schedule".

The mapping of state will get a bit complicated though. For example I don't know if START_SECONDS_TO_OVERRIDE will return to schedule after the override seconds expire.

@Jpsy
Copy link
Contributor Author

Jpsy commented May 27, 2020

Hi @wijnandtop, such a switch would be great, but I think it is extremely difficult and error prone to implement. The main reason is that the schedule on/off state of the mower depends on several factors and there seems to be no mower command that can only change that state without any side effects. Also the commands are not context free.

For example:
You would think that if the mower is parked and you call PARK_UNTIL_NEXT_TASK then you would reliably enable the schedule - without changing anything else. But in fact that command will change the schedule from today to tomorrow if the mower is already in scheduled state. So the stable way to enable a schedule when the mower is parked ist: PARK_UNTIL_FURTHER_NOTICE, wait for incoming status update, PARK_UNTIL_NEXT_TASK.

I am very much afraid we would invite evil with the trial to implement such a convenience function.

So my recommendation is to limit us to safely exposing the 4 existing commands if possible, or go with 3 commands if not. In fact your integration using the reverse engineer API also had only three commands exposed (through vacuum.start, .stop., .return_to_base).

What we should in fact do, is clearly document, which HA vacuum service calls which mower API command. So people will understand the difference between e.g. .start and .turn_on. I volunteer to write that documentation.

@osks
Copy link
Owner

osks commented May 28, 2020

IMHO the best mapping would be this:

1. `START_SECONDS_TO_OVERRIDE` = `vacuum.start`

2. `START_DONT_OVERRIDE` = `vacuum.turn_on`

3. `PARK_UNTIL_NEXT_TASK` = `vacuum.return_to_base`

4. `PARK_UNTIL_FURTHER_NOTICE` = `vacuum.stop` and `vacuum.turn_off`

That sounds like a good idea! I agree with you about how this should work.

I looked at the HA dev documentation you linked to and looked in the code for StateVacuumEntity in HA, and I found where it's says "Not supported" for async_turn_on, async_turn_off and async_toggle. I'm pretty sure that is only for the async version of those methods, and for for turn_on, turn_off, etc.

I haven't quite understood how a StateVacuumEntity is supposed to be controlled if it's not through the turn_on/turn_off methods (that you say didn't get through in your attempt). The dev documentation on the web (https://developers.home-assistant.io/docs/core/entity/vacuum) also doesn't say anything about the difference between VacuumEntity and StateVacuumEntity (both exist). I've also found the demo implementation for a vacuum (https://github.com/home-assistant/core/blob/dev/homeassistant/components/demo/vacuum.py), but haven't had time to think about it that much yet.

I found one difference though, the turn_on and turn_off methods in GardenaSmartMower doesn't have the **kwargs argument. Perhaps that is the problem?

Without knowing that you have already tested, I would try this:

class GardenaSmartMower(StateVacuumEntity):
    <... other code ...>

    def start(self):
        """Start cleaning or resume mowing."""
        duration = self.option_mower_duration * 60
        self._device.start_seconds_to_override(duration)

    def turn_on(self, **kwargs):
        """Start the mower."""
        self._device.start_dont_override()

    def return_to_base(self, **kwargs):
        """Set the lawn mower to return to the dock."""
        self._device.park_until_next_task()

    def stop(self, **kwargs):
        """Stop the lawn mower."""
        self.turn_off()

    def turn_off(self, **kwargs):
        """Stop mowing."""
        self.turn_off()

@Jpsy
Copy link
Contributor Author

Jpsy commented May 28, 2020

The changes that I proposed for a 3 command system are already running nicely in my local HA installation and with my mower. They are very similar to your code above.

I have also already tried to add the **kwargs param to turn_on and turn_off, but still no luck. They are not triggered by the corresponding service calls.

I am currently about to prepare a PR that will

  • add changes very similiar to yours above,
  • update the information texts and
  • add logging, so you can easily see which methods have been triggered.

Will need until tomorrow to get the PR up to your repo.

@Jpsy
Copy link
Contributor Author

Jpsy commented May 28, 2020

#11 (comment)

@Jpsy
Copy link
Contributor Author

Jpsy commented May 28, 2020

This PR implements the 3 command version, but the turn_on and turn_off methods are still preserved for further testing. **kwargs has been added to both.

@osks osks linked a pull request May 28, 2020 that will close this issue
@Jpsy
Copy link
Contributor Author

Jpsy commented May 29, 2020

TODO:
Check if there is a way to make vacuum.turn_on and vacuum.turn_off trigger the corresponding methods in vacuum.py.
If yes: Implement 4 command version.
If no: Stay with current version and close this issue.

@Jpsy
Copy link
Contributor Author

Jpsy commented May 30, 2020

Documentation for current 3 command version added in PR #19.

@osks
Copy link
Owner

osks commented May 31, 2020

Please continue in the new repository: py-smart-gardena/hass-gardena-smart-system#2

@osks osks closed this as completed May 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants